[Sanitizers] Add new sanitize-stable-abi flag for libsanitizers. #69943
Conversation
usama54321
requested review from
yln,
kubamracek,
devincoughlin,
rsundahl and
wrotki
usama54321
requested review from
artemcm,
tshortli,
hborla,
slavapestov and
xedin
as code owners
include/swift/Option/Options.td
Outdated
| @@ -1394,6 +1394,10 @@ def sanitize_coverage_EQ : CommaJoined<["-"], "sanitize-coverage=">, | |||
| HelpText<"Specify the type of coverage instrumentation for Sanitizers and" | |||
| " additional options separated by commas">; | |||
|
|
|||
| def sanitize_stable_abi_EQ : Flag<["-"], "sanitize-stable-abi">, | |||
| Flags<[FrontendOption, NoInteractiveOption]>, | |||
| HelpText<"ABI instrumentation for sanitizer runtime.">; | |||
There was a problem hiding this comment.
Please improve the help text here, I don't think anyone (who doesn't already know) can understand what the option does based on this description.
There was a problem hiding this comment.
I initially copied the HelpText from the same option in clang. I have updated it to make it a bit more helpful
| if (context.OI.SanitizerUseStableABI) | ||
| addLinkSanitizerLibArgsForDarwin(context.Args, Arguments, "asan_abi", | ||
| *this, false); | ||
| else | ||
| addLinkSanitizerLibArgsForDarwin(context.Args, Arguments, "asan", *this); |
There was a problem hiding this comment.
Could use something like this to reduce diff/duplication.
auto good_name = context.OI.SanitizerUseStableABI ? "asan_abi" : "asan";
There was a problem hiding this comment.
There is an extra boolean argument in one of the cases, so this would require two ternary operators, so using an if feels cleaner here.
| @@ -0,0 +1,5 @@ | |||
| // REQUIRES: asan_runtime | |||
| // RUN: %swiftc_driver -driver-print-jobs -sanitize=address -sanitize-stable-abi %s 2>&1 | %FileCheck %s | |||
There was a problem hiding this comment.
When I read "-sanitize-stable-abi` as "sanitize stable ABI" it's not the right meaning. Compare "Sanitize address" which describes pretty well what it does.
The variable names for this are SanitizerUseStableABI, so we just forgot the r here?
I think the "sanitizer" prefix here is a noun vs. "sanitize" (verb). If we want to include a verb, we could spell the option with "use".
I would pick one of these:
-sanitizer-stable-abi
-sanitizer-use-stable-abi
There was a problem hiding this comment.
I matched the convention used in clang where we use -fsanitize-stable-abi flag. In the last team meeting, @devincoughlin suggested that the naming should be consistent in clang and swift. We currently are not planning to change this in clang
|
@swift-ci test |
This patch adds a new flag sanitize-stable-abi to support linking against the Sanitizers stable ABI added recently in compiler-rt. The patch also passes extra options for the ASan pass when using this flag to outline instrumentation code and remove version check. rdar://112915278
usama54321
force-pushed
the
eng/112915278
branch
from
d437c86 to
8282bed
Compare
|
@swift-ci test |
| @@ -317,8 +317,13 @@ toolchains::Darwin::addSanitizerArgs(ArgStringList &Arguments, | |||
| // Linking sanitizers will add rpaths, which might negatively interact when | |||
| // other rpaths are involved, so we should make sure we add the rpaths after | |||
| // all user-specified rpaths. | |||
| if (context.OI.SelectedSanitizers & SanitizerKind::Address) | |||
| addLinkSanitizerLibArgsForDarwin(context.Args, Arguments, "asan", *this); | |||
| if (context.OI.SelectedSanitizers & SanitizerKind::Address) { | |||
There was a problem hiding this comment.
Will the rpath still be added? If it's not necessary is it just a NOP or could it cause a problem?
|
@swift-ci test |
|
@swift ci test Linux |
|
@swift-ci test Linux |
1 similar comment
|
@swift-ci test Linux |
|
@swift-ci test Linux |
|
@swift-ci Please smoke test Linux platform |
|
Cloned this commit to #70508 , and will be merging from there (on behalf of Usama who's not available at the moment). So closing this one |
This patch adds a new flag sanitize-stable-abi to support linking against the stable ABI introduced in libsanitizers. The patch also passes the appropriate options for the ASan pass when using this flag.
This change depends on the llvm-project change here: llvm/llvm-project#72439
rdar://112915278