-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Determine whether a given sanitizer is available based on the presenc… #14919
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
Determine whether a given sanitizer is available based on the presenc… #14919
Conversation
…e of the library. rdar://37192887
@swift-ci please test |
lib/Option/SanitizerOptions.cpp
Outdated
Diags.diagnose(SourceLoc(), diag::error_unsupported_option_argument, | ||
A->getOption().getPrefixedName(), A->getValue(i)); | ||
} else if (sanitizerRuntimeLibExists(toFileName(*kind), isShared) | ||
&& (*kind != SanitizerKind::Thread || Triple.isArch64Bit())) { |
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.
Can we keep the helper function "isTSanSupported"? Even if it's just a test for .isArch64bit. This condition is very hard to read.
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.
@kubamracek keeping the function does not make sense, as we now check existence of all sanitizers. I've tried to rewrite the condition to make it more readable.
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.
Can we then just be a bit more verbose here? Something like
bool sanitizerSupported = true;
if (*kind == Thread) {
if (!Triple.is64bit() sanitizerSupported = false; // TSan requires 64-bit architecture
}
...
Besides the comment, this LGTM. Thanks! |
Build failed |
@swift-ci please smoke test |
LGTM. Thanks! |
…e of the library.
rdar://37192887
Basically, if the library is there, the sanitizer is available. IF not, then it's not available.