Skip to content

[SYCL][Driver] Remove temporary string usage in HandleXarchArgs #13248

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 5, 2024
Merged

[SYCL][Driver] Remove temporary string usage in HandleXarchArgs #13248

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe96c9a
[SYCL][Driver] Remove temporary string usage in HandleXarchArgs
jinge90 Apr 2, 2024
b10386f
Merge remote-tracking branch 'upstream/sycl' into fix_msan_bug_for_ha…
jinge90 Apr 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang/lib/Driver/Compilation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any way we can capture the bad behavior in a test other than running through the sanitizer?

Copy link
Contributor Author

@jinge90 jinge90 Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (NeedHandle) {
Expand Down