-
Notifications
You must be signed in to change notification settings - Fork 738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SYCL][ClangLinkerWrapper] Fix read of invalid memory #15472
Conversation
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
// the file. | ||
StringRef UnbundledArchiveList = | ||
Args.MakeArgString((*ObjList)->getBuffer()); | ||
UnbundledArchiveList.split(ObjectFilePaths, '\n', /*MaxSplit=*/-1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where will this variable be used? Not a blocking question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review. It's used here, we immediately split it into the ObjectFilePaths
vector .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah..of course..I was naive and was looking for this variable on the RHS of an assignment statement. :-(. Thinking more, may be not so naive. I was just wondering where else we would be using UnbundledArchiveList that we needed to store it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good as something that will be used in upcoming patches.
@intel/llvm-gatekeepers |
The problem was the buffer gets freed when we go out of the
if
because theunique_ptr
is created there, so just make a copy that will stay alive as long as we need it. Confirmed fix with valgrind.This is already tested by
sycl/test-e2e/NewOffloadDriver/multisource.cpp
and it fails sporadically which led me to this fix.