[SYCL][Driver] Remove temporary string usage in HandleXarchArgs #13248
Conversation
Signed-off-by: jinge90 <[email protected]>
| @@ -99,7 +99,7 @@ static void HandleXarchArgs(DerivedArgList *OffloadArgList, const Driver &D, | |||
| OffloadArgList->MakeArgStringRef(XarchVecs[Index])); | |||
| } | |||
| } else | |||
| XarchValueRefs.push_back(StringRef(XarchV)); | |||
| XarchValueRefs.push_back(OffloadArgList->MakeArgString(XarchV)); | |||
There was a problem hiding this comment.
Any way we can capture the bad behavior in a test other than running through the sanitizer?
There was a problem hiding this comment.
Hi, @mdtoguchi
The sycl-xarch.cpp test has covered this line of code already but it just passed due to good luck. This behavior is a general problem, a temporary value is still used when it is out of scope. Sometimes, the memory of this value is not touched even when it is out of scope, this is why this test happened to pass. In some platform, the memory of this temporary value may be modified, then we will see the lit test fails.
It is very difficult to catch such kind of errors because we can't make the temporary value's memory to be changed when it is out of scope and memory sanitizer is invented to detect such bugs via shadow memory mechanism.
Thanks very much.
|
Hi, @intel/llvm-gatekeepers |
|
@srividya-sundaram Can you please approve this PR if you're content with the resolution of your comment? |
|
Hi, @intel/llvm-gatekeepers |
We should use string which is valid during the whole compilation phase instead of a temporary string.