Disable "UseOdrIndicator" ASan instrumentation mode by default. #35074

danliew-apple · 2020-12-12T02:23:23Z

Previously Swift enabled the "UseOdrIndicator" ASan instrumentation mode
and gave no option to disable this. This probably wasn't intentional but
happened due to the fact the
createModuleAddressSanitizerLegacyPassPass() function has a default
value for the UseOdrIndicator parameter of true and in Swift we
never specified this parameter explicitly.

Clang disables the "UseOdrIndicator" mode by default but allows it to be
enabled using the -fsanitize-address-use-odr-indicator flag.
Having "UseOdrIndicator" off by default is probably the right
default choice because it bloats the binary. So this patch changes the
Swift compiler to match Clang's behavior.

This patch disables the "UseOdrIndicator" mode by default but adds a
hidden driver and frontend flag (-sanitize-address-use-odr-indicator)
to enable it. The flag is hidden so that we can remove it in the future
if needed.

A side of disabling "UseOdrIndicator" is that by we will no longer use
private aliases for poisoning globals. Private aliases were introduced
to avoid crashes (google/sanitizers#398) due
to ODR violations with non-instrumented binaries. On Apple platforms the
use of two-level namespaces probably means that using private aliases
wasn't ever really necessary to avoid crashes. On platforms with a flat
linking namespace (e.g. Linux) using private aliases might matter more
but should users actually run into problems they can either:

Fix their environment to remove the ODR, thus avoiding the crash.
Instrument the previously non-instrumented code to avoid the crash.
Use the new -sanitize-address-use-odr-indicator flag

rdar://problem/69335186

danliew-apple · 2020-12-12T02:29:11Z

@swift-ci Please test

danliew-apple · 2020-12-12T02:29:46Z

@swift-ci Please smoke test

swift-ci · 2020-12-12T03:47:09Z

Build failed
Swift Test OS X Platform
Git Sha - 9856863

yln

I am happy with this patch with one small ask (check that we get an unused warning without ASan) and one small nit (code for checking existence of boolean command line option).

This patch does the following two things:

Add a Swift driver frontend flag to allow opting into ASan's UseOdrIndicator.
Switch the default behavior from UseOdrIndicator=true to UseOdrIndicator=false.

I agree with Dan's reasoning that it being true for Swift was most likely just an oversight and aligning it with clang's behavior seems like the right thing to do.
@kubamracek: do you agree?

Previously Swift enabled the "UseOdrIndicator" ASan instrumentation mode
and gave no option to disable this. This probably wasn't intentional but
happened due to the fact the
createModuleAddressSanitizerLegacyPassPass() function has a default
value for the UseOdrIndicator parameter of true and in Swift we
never specified this parameter explicitly.

Clang disables the "UseOdrIndicator" mode by default but allows it to be
enabled using the -fsanitize-address-use-odr-indicator flag.
Having "UseOdrIndicator" off by default is probably the right
default choice because it bloats the binary. So this patch changes the
Swift compiler to match Clang's behavior.

@danliew-apple: maybe we can also get one approval from the Swift team?

yln · 2020-12-14T18:47:40Z

lib/IRGen/IRGen.cpp

-                                                    recover));
+  PM.add(createModuleAddressSanitizerLegacyPassPass(
+      /*CompileKernel=*/false, recover, /*UseGlobalsGC=*/true,
+      useODRIndicator));


Just note to myself: here is the place we are switching the default. We are now using an explicit value determined by the driver instead of the default specified in the header (which is true).

include/swift/Option/Options.td

lib/Frontend/CompilerInvocation.cpp

danliew-apple · 2020-12-14T19:22:32Z

@brentdax Adding you as a reviewer because it looks like you recently changed stuff in the driver.

danliew-apple · 2020-12-14T19:23:36Z

@DougGregor Do the driver changes here cause any problems for the new swift swift driver?

beccadax

As long as this change won't break any builds from before this new flag was introduced, it looks fine from the driver's perspective.

I believe analogous work will be necessary in the new driver. Rather than asking Doug to review, I'll just mention @artemcm and @nkcsgexi here so they know to schedule that.

beccadax

(Sorry, I was trying to remove Doug and it somehow got two others, too. GitHub for iOS bug, maybe?)

artemcm · 2020-12-14T19:53:50Z

I believe analogous work will be necessary in the new driver. Rather than asking Doug to review, I'll just mention @artemcm and @nkcsgexi here so they know to schedule that.

Yes, the Driver component of this PR will need to be implemented in https://github.com/apple/swift-driver also, to propagate the new flag to swift-frontend jobs. It looks like a fairly simple change.

danliew-apple · 2020-12-14T22:46:15Z

@yln Can you check you're okay with the new patch?

danliew-apple · 2020-12-14T22:46:27Z

@swift-ci Please smoke test

danliew-apple · 2020-12-14T22:46:35Z

@swift-ci Please test

danliew-apple · 2020-12-14T22:49:01Z

@gottesmm @compnerd I touched IRGen so you should probably check that you're okay with this change.

swift-ci · 2020-12-14T23:46:01Z

Build failed
Swift Test Linux Platform
Git Sha - adfbd11

lib/Option/SanitizerOptions.cpp

compnerd · 2020-12-15T00:16:17Z

lib/Option/SanitizerOptions.cpp

@@ -258,6 +258,22 @@ OptionSet<SanitizerKind> swift::parseSanitizerRecoverArgValues(
  return sanitizerRecoverSet;
 }

+bool swift::parseSanitizerAddressUseODRIndicator(
+    const llvm::opt::Arg *A, const OptionSet<SanitizerKind> &enabledSanitizers,


Given that we don't expect this to be NULL (see below), I think that it would be better to change the signature to take the Arg by reference (const llvm::opt::Arg &A).

This is a valid criticism but the use of llvm::opt::Arg* here is deliberate. It's being used so that this new function follows a similar design as the other functions in include/swift/Option/SanitizerOptions.h.

I'd prefer to keep the APIs as uniform as possible. Note I'm aware there are some oddities already in this file (inconsistent argument order) but we shouldn't add to them if we don't have to.

I don't think its adding any inconsistency. The LLVM style actually encourages this - pass by reference unless you need to be able to pass in nullptr as a reference to nullptr is not well-formed.

I wasn't talking about inconsistency with respect to LLVM's recommended style. I'm talking about inconsistency with respect to the APIs that already exist in SanitizerOptions.h. They all take a llvm::opt::Arg*. All of these could actually be Arg& because they all assume that Arg* A is not null. Deliberately adding an API that doesn't match the existing APIs is an unnecessary inconsistency. We could change all of the functions in the header but that's really out-of-scope for this PR. If you really want this that let's do that in a separate PR.

I suspect the reason these APIs take a llvm::opt::Arg* right now is that is the type available at the use sites and so passing it directly was the most convenient thing to do.

shahmishal · 2020-12-15T00:54:30Z

@swift-ci test Linux

Previously Swift enabled the "UseOdrIndicator" ASan instrumentation mode and gave no option to disable this. This probably wasn't intentional but happened due to the fact the `createModuleAddressSanitizerLegacyPassPass()` function has a default value for the `UseOdrIndicator` parameter of `true` and in Swift we never specified this parameter explicitly. Clang disables the "UseOdrIndicator" mode by default but allows it to be enabled using the `-fsanitize-address-use-odr-indicator` flag. Having "UseOdrIndicator" off by default is probably the right default choice because it bloats the binary. So this patch changes the Swift compiler to match Clang's behavior. This patch disables the "UseOdrIndicator" mode by default but adds a hidden driver and frontend flag (`-sanitize-address-use-odr-indicator`) to enable it. The flag is hidden so that we can remove it in the future if needed. A side effect of disabling "UseOdrIndicator" is that by we will no longer use private aliases for poisoning globals. Private aliases were introduced to avoid crashes (google/sanitizers#398) due to ODR violations with non-instrumented binaries. On Apple platforms the use of two-level namespaces probably means that using private aliases wasn't ever really necessary to avoid crashes. On platforms with a flat linking namespace (e.g. Linux) using private aliases might matter more but should users actually run into problems they can either: * Fix their environment to remove the ODR, thus avoiding the crash. * Instrument the previously non-instrumented code to avoid the crash. * Use the new `-sanitize-address-use-odr-indicator` flag rdar://problem/69335186

danliew-apple · 2020-12-15T19:13:21Z

@swift-ci Please smoke test

danliew-apple · 2020-12-16T21:05:52Z

@gottesmm Ping.

danliew-apple · 2020-12-17T03:25:21Z

@compnerd Thanks for approving.

danliew-apple requested review from yln, kubamracek and ravikandhadai December 12, 2020 02:23

yln approved these changes Dec 14, 2020

View reviewed changes

danliew-apple requested a review from beccadax December 14, 2020 19:20

danliew-apple requested a review from DougGregor December 14, 2020 19:23

beccadax approved these changes Dec 14, 2020

View reviewed changes

beccadax requested review from beccadax and yln and removed request for DougGregor December 14, 2020 19:36

beccadax approved these changes Dec 14, 2020

View reviewed changes

delcypher force-pushed the dliew/rdar-69335186 branch from 9856863 to adfbd11 Compare December 14, 2020 22:44

danliew-apple requested review from gottesmm and compnerd December 14, 2020 22:47

yln approved these changes Dec 14, 2020

View reviewed changes

compnerd reviewed Dec 15, 2020

View reviewed changes

delcypher force-pushed the dliew/rdar-69335186 branch from adfbd11 to 9208b52 Compare December 15, 2020 19:11

compnerd approved these changes Dec 17, 2020

View reviewed changes

danliew-apple merged commit 8e95189 into main Dec 17, 2020

danliew-apple deleted the dliew/rdar-69335186 branch December 17, 2020 03:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable "UseOdrIndicator" ASan instrumentation mode by default. #35074

Disable "UseOdrIndicator" ASan instrumentation mode by default. #35074

danliew-apple commented Dec 12, 2020

danliew-apple commented Dec 12, 2020

danliew-apple commented Dec 12, 2020

swift-ci commented Dec 12, 2020

yln left a comment

yln Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

beccadax left a comment

beccadax left a comment

artemcm commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

swift-ci commented Dec 14, 2020

compnerd Dec 15, 2020

danliew-apple Dec 15, 2020 •

edited

Loading

compnerd Dec 15, 2020

danliew-apple Dec 15, 2020

shahmishal commented Dec 15, 2020

danliew-apple commented Dec 15, 2020

danliew-apple commented Dec 16, 2020

danliew-apple commented Dec 17, 2020

Disable "UseOdrIndicator" ASan instrumentation mode by default. #35074

Disable "UseOdrIndicator" ASan instrumentation mode by default. #35074

Conversation

danliew-apple commented Dec 12, 2020

danliew-apple commented Dec 12, 2020

danliew-apple commented Dec 12, 2020

swift-ci commented Dec 12, 2020

yln left a comment

Choose a reason for hiding this comment

yln Dec 14, 2020

Choose a reason for hiding this comment

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

beccadax left a comment

Choose a reason for hiding this comment

beccadax left a comment

Choose a reason for hiding this comment

artemcm commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

danliew-apple commented Dec 14, 2020

swift-ci commented Dec 14, 2020

compnerd Dec 15, 2020

Choose a reason for hiding this comment

danliew-apple Dec 15, 2020 • edited Loading

Choose a reason for hiding this comment

compnerd Dec 15, 2020

Choose a reason for hiding this comment

danliew-apple Dec 15, 2020

Choose a reason for hiding this comment

shahmishal commented Dec 15, 2020

danliew-apple commented Dec 15, 2020

danliew-apple commented Dec 16, 2020

danliew-apple commented Dec 17, 2020

danliew-apple Dec 15, 2020 •

edited

Loading