[asan] Set abort_on_error=1 by default on OS X

[kubamracek]

Summary

This sets the default ASan flags to abort_on_error=1 on OS X. For unit tests and lit tests we set ASAN_OPTIONS back to abort_on_error=0 before running the tests (to avoid crashing). I added two tests that intentionally don't respect the default ASAN_OPTIONS to test the behavior of an empty ASAN_OPTIONS (on OS X we should crash, on Linux we should exit()).

[kcc]

When possible, we should avoid differences in behavior between platforms and ifdefs in the code.
Can this be solved in some other way?

E.g. in the Xcode glue, etc?

[kubamracek]

In D7203#114142, @kcc wrote:

When possible, we should avoid differences in behavior between platforms and ifdefs in the code.
Can this be solved in some other way?
E.g. in the Xcode glue, etc?

I think this needs to be the standard behavior for command-line usage of clang -fsanitize=address. Otherwise, we could just instruct everyone to add ASAN_OPTIONS=abort_on_error=1 whenever running an ASanified program, but that's really just an annoyance and another reason why abort_on_error=1 should be the default.

Especially on OS X, setting the extra environment variable might be complicated, e.g. when running GUI apps by clicking the icon in Finder. In the same scenario, exit()-ing instead of abort()-ing is really something that is unexpected, because the app will just disappear. Developers are used to diagnostic tools abort()-ing (or crashing the process in some other way).

I understand that platform differences should be minimal, but this is still something that users of ASan on OS X would benefit from. Would implementing this in a different way than an #if be more acceptable (e.g. in CMake configuration)?

ASAN_OPTIONS=$ASAN_OPTIONS:verbosity=1...

This makes the tests non-hermetic (or at least, less hermetic)

How about using a lit substitution instead, for example %env, or adding that to the existing %run substitution (that we currently only use for QEMU testing)?

[samsonov]

lib/asan/asan_flags.h

31 ↗

(On Diff #18828)

Wait a sec, why can't you write

ASAN_FLAG(bool, abort_on_error, (SANITIZER_MAC == 1), "....")

?

[kubamracek]

why can't you write ASAN_FLAG(bool, abort_on_error, (SANITIZER_MAC == 1), "....")

Updated.

However, if you really want to override ASAN_OPTIONS to use "abort_on_error=true", this change might be OK, provided that you unconditionally overwrite ASAN_OPTIONS from config.environment with this value.

Updated.

Thanks for the suggestions!

[ygribov]

Why not enable it on all platforms btw?

[kubamracek]

In D7203#114433, @ygribov wrote:

Why not enable it on all platforms btw?

I'd support having abort_on_error=1 on all platforms, but it was mentioned that current integrations of ASan on Linux probably depend on the current behavior, and they would need to be changed (maybe just setting different ASAN_OPTIONS if you really don't want the process to crash on error?).

[ygribov]

it was mentioned that current integrations of ASan on Linux
probably depend on the current behavior, and they would need to be changed

True but this may be justified by the fact that we make behavior more consistent across platforms.

[kubamracek]

As suggested, this updates the patch to apply abort_on_error=1 by default on all platforms. That makes behavior more consistent across platforms.

[kubamracek]

ping

[glider]

The current integration of ASan at Chrome doesn't depend on the
current behavior, I'm ok with this patch.
Kostya, what do you think?

[zaks.anna]

Ping

Should Kuba proceed with setting abort_on_error=1 by default on all platforms? Or is there anything else we can/should investigate before proceeding?

[kcc]

I am still opposed to having abort_on_error=1 by default, on all platforms or just on one.
We should find a way to enable this by default for all of your use cases, but w/o affecting the upstream default.

[zaks.anna]

Kostya,

Our goal is to have abort_on_error=1 as the default on OS X in all scenarios. Here is the reasoning.

There are several workflows in which one would run an ASanified process:

1. Executable launched from command line. Here, the output from stdout/stderr is expected.
2. A finger launched application or a daemon launched indirectly (via launchd). The current behavior is puzzling to developers - the app just quits and a daemon relaunches. Also, in these cases, stdout/stderr is lost by default. The developers expect to get notified about the program misbehavior through CrashReporter (or syslog). CrashReporter will not work if the executable just exits on error. This is the case that we are trying to address. Unfortunately, the current behavior is not acceptable for this workflow based on the user feedback we’ve received. I don’t think we have a way to handle this in a special way since people just build their apps/daemons, install, and run them in all kind of setups.
3. Various development settings: lldb, IDE… It is not necessary to discuss these as one should be able to alter the default settings here.

Given that we need abort_on_error=1 for #2, I would try to keep as much consistency as possible and have the same default for #1. I don’t know if there is a reason to not crash when running from command line. Apart from breaking existing workflows, is there a reason you prefer exit on error?

We understand that you might not want this behavior and/or it might be breaking existing workflows, so we are fine with keeping this on our branch (either indefinitely or until you change your mind:)). What about allowing the patch that sets the default ASAN_OPTIONS variable in lit (http://reviews.llvm.org/D7204)? This would simplify things for us.

[kcc]

In D7203#135567, @zaks.anna wrote:

Kostya,

Our goal is to have abort_on_error=1 as the default on OS X in all scenarios. Here is the reasoning.

There are several workflows in which one would run an ASanified process:

1. Executable launched from command line. Here, the output from stdout/stderr is expected.
2. A finger launched application or a daemon launched indirectly (via launchd). The current behavior is puzzling to developers - the app just quits and a daemon relaunches. Also, in these cases, stdout/stderr is lost by default. The developers expect to get notified about the program misbehavior through CrashReporter (or syslog). CrashReporter will not work if the executable just exits on error. This is the case that we are trying to address. Unfortunately, the current behavior is not acceptable for this workflow based on the user feedback we’ve received. I don’t think we have a way to handle this in a special way since people just build their apps/daemons, install, and run them in all kind of setups.

We have various ways to alter the default behavior at link-time.
We can have a separate tool like that for abort_on_error.
E.g. a function that will check if we are in #2

3. Various development settings: lldb, IDE… It is not necessary to discuss these as one should be able to alter the default settings here.

Given that we need abort_on_error=1 for #2, I would try to keep as much consistency as possible and have the same default for #1. I don’t know if there is a reason to not crash when running from command line. Apart from breaking existing workflows, is there a reason you prefer exit on error?

Existing workflows is an important concern.
Not just our own workflows (where we can easily override the defaults) but also workflows of some unknown users.

We understand that you might not want this behavior and/or it might be breaking existing workflows, so we are fine with keeping this on our branch (either indefinitely or until you change your mind:)). What about allowing the patch that sets the default ASAN_OPTIONS variable in lit (http://reviews.llvm.org/D7204)? This would simplify things for us.

That thing I dislike even more. Such a change in lit will mean that tests running under lit and manually will behave differently
and that tests that use ASAN_OPTIONS=foo will have to be changed to ASAN_OPTIONS=$ASAN_OPTIONS=foo
What is "generating crashlogs" and can that be disabled separately?

[zaks.anna]

We have various ways to alter the default behavior at link-time.
We can have a separate tool like that for abort_on_error.
E.g. a function that will check if we are in #2

What do you mean by altering the behavior at link time?

It's not how you build the binaries that differs. It's how you run them. You can have the same process that can be launched from command line and through launchd. I don't know how to restrict the set of these at link time. People can build with make, ninja, or xcodebuild, it should not matter. In all cases, we want to trigger the CrashReporter on OS X.

Existing workflows is an important concern.
Not just our own workflows (where we can easily override the defaults) but also workflows of some unknown users.

True. However, crashing on error is the right thing to do on OS X (in my opinion). It might be worth doing the switch and the sooner you do this the better (less people rely on the existing behavior). Also, it should be relatively easy for people to work around this. However, this most likely will break someone and this is a policy decision, so I can only offer an opinion.

That thing I dislike even more. Such a change in lit will mean that tests running under lit and manually will behave differently
and that tests that use ASAN_OPTIONS=foo will have to be changed to ASAN_OPTIONS=$ASAN_OPTIONS=foo

True. Dmitri G. suggested providing a different substitution for lit's "not" on darwin. Specifically, we would use "not crash" instead of "not". Kuba, I don't think we've explored this option before.

What is "generating crashlogs" and can that be disabled separately?

CrashReporter is activated when a program receives a fatal termination signal. It cannot be disabled on per process/application bases and is enabled by default on Mac. Crashlogs are the reports generated by CrashReporter, which is the standard facility for logging information about crashed processes on OS X. It works for command line tools as well as daemons, apps automatically. CrashReporter will not work on processes that do not crash. This is why we believe, an ASan report should trigger CrashReporter and generate a crashlog. This is the first place Mac developers look at when there is a malfunction anywhere in the system.

https://developer.apple.com/library/mac/technotes/tn2004/tn2123.html

[kcc]

In D7203#135783, @zaks.anna wrote:

We have various ways to alter the default behavior at link-time.
We can have a separate tool like that for abort_on_error.
E.g. a function that will check if we are in #2

What do you mean by altering the behavior at link time?

It's not how you build the binaries that differs. It's how you run them. You can have the same process that can be launched from command line and through launchd. I don't know how to restrict the set of these at link time. People can build with make, ninja, or xcodebuild, it should not matter. In all cases, we want to trigger the CrashReporter on OS X.

How about altering the default at run-time?
If you someone recognize that you are running via launchd, enable abort_on_error
That would sounds reasonable to me.

Existing workflows is an important concern.
Not just our own workflows (where we can easily override the defaults) but also workflows of some unknown users.

True. However, crashing on error is the right thing to do on OS X (in my opinion). It might be worth doing the switch and the sooner you do this the better (less people rely on the existing behavior). Also, it should be relatively easy for people to work around this. However, this most likely will break someone and this is a policy decision, so I can only offer an opinion.

That thing I dislike even more. Such a change in lit will mean that tests running under lit and manually will behave differently
and that tests that use ASAN_OPTIONS=foo will have to be changed to ASAN_OPTIONS=$ASAN_OPTIONS=foo

True. Dmitri G. suggested providing a different substitution for lit's "not" on darwin. Specifically, we would use "not crash" instead of "not". Kuba, I don't think we've explored this option before.

There is "not --crash"

What is "generating crashlogs" and can that be disabled separately?

CrashReporter is activated when a program receives a fatal termination signal. It cannot be disabled on per process/application bases and is enabled by default on Mac. Crashlogs are the reports generated by CrashReporter, which is the standard facility for logging information about crashed processes on OS X. It works for command line tools as well as daemons, apps automatically. CrashReporter will not work on processes that do not crash. This is why we believe, an ASan report should trigger CrashReporter and generate a crashlog. This is the first place Mac developers look at when there is a malfunction anywhere in the system.

So, we clearly do not want to have this in lit tests,
but I also don't want the hack that disables changed the default ASAN_OPTIONS in lit.cfg (explained above)

https://developer.apple.com/library/mac/technotes/tn2004/tn2123.html

[zaks.anna]

How about altering the default at run-time?
If you someone recognize that you are running via launchd, enable abort_on_error
That would sounds reasonable to me.

That'd be confusing to the user. Someone launches the app from the dock, the app crashes. They launch the same app from command line, it does not. Having consistency and generating a crashlog is the most user friendly behavior.

You are right, "not --crash" will slow down testing. I did not think about this right away.

The only other idea I have is to use an altered run command (ex: "%run_under_san" or "%set_asan_options %run") that would include ASAN_OPTIONS in its expansion. All tests would have to adopt it, but it would make tests running under lit and manually behave the same.

[kcc]

In D7203#135893, @zaks.anna wrote:

How about altering the default at run-time?
If you someone recognize that you are running via launchd, enable abort_on_error
That would sounds reasonable to me.

That'd be confusing to the user. Someone launches the app from the dock, the app crashes. They launch the same app from command line, it does not. Having consistency and generating a crashlog is the most user friendly behavior.

consistency is good, but here I don't buy this argument completely.
If one runs the app from command line, the app will not just exit(1) but it will also print tons of stuff to the console -- hard to miss.

You are right, "not --crash" will slow down testing. I did not think about this right away.

The only other idea I have is to use an altered run command (ex: "%run_under_san" or "%set_asan_options %run") that would include ASAN_OPTIONS in its expansion. All tests would have to adopt it, but it would make tests running under lit and manually behave the same.

Too complex. (requires lots of test changes and then more complex manual runs).

Again, I propose you a solution that will allow you to have abort_on_error=1 (always, or only for some cases)
w/o a) affecting everyone else upstream b) changing tests and c) requiring you to keep separate branch.

We can define a weak hook asan_default_abort_on_error and you will simply link with a module that defines this hook to return 1;
(Err.. Do we have weak functions on Mac? If not, we'll need to invent other mechanism to override defaults on Mac)
See what we are doing with asan_default_options or __asan_default_suppressions.
It's a good idea to have such hooks on Mac anyway if we don't have them yet.

[zaks.anna]

If one runs the app from command line, the app will not just exit(1) but it will also print tons of stuff to the console -- hard to miss.

Users might not run that app directly; for example, it can be executed from a shell script. There are all kinds of strange scenarios. I just saw a case of a shell script, which executes arch(1), which executes a tool, which presumably dlopen's an ASanified shared library. The stderr and stdin could be redirected; exit(1) can be interpreted in various ways. Crashing is much more reliable.

We do have weak functions, so those hooks are a fair game!

However, if I understand your solution correctly, it still requires private changes to clang/llvm. (We want this functionality in clang, not in a specific application we build with it.) Ex: who will link in the library? We'd need to:

• Make sure the library builds when we build clang.
• Change the driver to link the library into an ASanified binary. (Maybe this could be conditional on the presence of the library and go into mainline?)
• Tests would still be effected. We could only build this library in some special configuration, which is turned off during testing. However, in that case, we are not testing the final product.

I think the advantage here is that it would cause less merge conflicts, especially, if we want to diverge in multiple ways.

[kcc]

What about adding a clang driver flag -fsanitize-abort-on-error that will link an extra object?

[ygribov]

What about adding a clang driver flag -fsanitize-abort-on-error that will link an extra object?

Instead of limiting ourselves to abort-on-error, can this instead be -fsanitize-default-options=? But we'll need something more complicated than just another object file.

[zaks.anna]

What about adding a clang driver flag -fsanitize-abort-on-error that will link an extra object?

The only upside I see with this approach is that it will allow us to keep the tests simpler - instead of prepending ASAN_OPTIONS=abort_on_error=0 to the %run expansion, we would need to change the compiler invocation. Plus, we would not need to change the existing tests that set ASAN_OPTIONS directly.

I am not convinced that adding an extra library to a compiler distribution in order to change the default is the right tradeoff here.

[glider]

I don't like the idea of linking extra objects ultimately for every user.
If the user wants to set his own defaults there'll be two versions of
__asan_default_options.

[kcc]

In D7203#138655, @glider wrote:

I don't like the idea of linking extra objects ultimately for every user.
If the user wants to set his own defaults there'll be two versions of
__asan_default_options.

Agree.
Exactly for this reason I proposed __asan_default_abort_on_error. Which is not perfect too.
Anyway, Anna agreed to wait with this patch for some time, maybe the requirements (either her or ours) will become clearer or will change.

[kubamracek]

Hi everyone, I'd like to (once again) reopen this topic. A recent change introduced "default flags for lit tests", so this patch can be simplified and we don't need to modify any test.

[samsonov]

FWIW I'm fine with letting you decide the default flag value for Mac.

test/asan/TestCases/Darwin/abort_on_error-darwin.cc

15 ↗

(On Diff #30247)

Remove this CHECK line, it doesn't seem to be important for test.

test/asan/TestCases/Linux/abort_on_error-linux.cc

15 ↗

(On Diff #30247)

ditto

[kcc]

I was against introducing ASAN_OPTIONS in lit.cfg and probably missed the change where it got added.
I still dislike this idea.

[zaks.anna]

It's been added by an unrelated commit, see http://reviews.llvm.org/D10294.

[kcc]

In D7203#209296, @zaks.anna wrote:

It's been added by an unrelated commit, see http://reviews.llvm.org/D10294.

Sure. I think this was a mistake and a very bad way forward.

[samsonov]

In D7203#209307, @kcc wrote:

In D7203#209296, @zaks.anna wrote:

It's been added by an unrelated commit, see http://reviews.llvm.org/D10294.

Sure. I think this was a mistake and a very bad way forward.

I think it was me who approved that change by Filipe. I'm sympathetic to your concerns regarding
running tests in a different configuration that is shipped by default to the end users, and this can apply
to different abort_on_error values on different OSes, but not for the existing state.

[kcc]

But the existing state encourage more changes to ASAN_OPTIONS in lit.cfg.
We at the very least should warn in the comments about adding more options there,
but better remove this too.

[filcab]

Hi all,
Sorry for not replying earlier.

Kostya:
It encourages changes if needed, yes.
Like when a platform needs to change something related to other platforms
where ASan runs, and would require lots of changes to tests.

We also clear away ASAN_OPTIONS (and the others) when we run tests, which
allows us to make minimum assumptions when running them. This specific
change seems to be strictly on the "positive" side. Without it we could
simply fail some tests because the user happened to have a specific option
turned on by default and ran the ASan tests.

By having ASAN_OPTIONS be reset by lit, and then having each test that
cares about it adding or replacing it, we can test the options themselves,
their absence, and have "proper" defaults for the tests.
It also allows us to "easily" have platforms specify their own defaults for
flags, like what Kuba wants to do.

About this specific change:
It's surprising (to me) to have ASan, by default, exit() the process, as if
"no error" had happened (by which I mean: The process exited in a "normal"
way, even though it exited with a failure exit code). This doesn't affect
me much, as an exit() on my platform is also "weird". But on Mac OS X it
feels weird to not have it crash.

Regards,

Filipe

[samsonov]

In D7203#209394, @filcab wrote:

Hi all,
Sorry for not replying earlier.

Kostya:
It encourages changes if needed, yes.
Like when a platform needs to change something related to other platforms
where ASan runs, and would require lots of changes to tests.

We also clear away ASAN_OPTIONS (and the others) when we run tests, which
allows us to make minimum assumptions when running them. This specific
change seems to be strictly on the "positive" side. Without it we could
simply fail some tests because the user happened to have a specific option
turned on by default and ran the ASan tests.

By having ASAN_OPTIONS be reset by lit, and then having each test that
cares about it adding or replacing it, we can test the options themselves,
their absence, and have "proper" defaults for the tests.
It also allows us to "easily" have platforms specify their own defaults for
flags, like what Kuba wants to do.

Could you remind me why we set ASAN_OPTIONS=symbolize_vs_style=false in lit config, if we discard
ASAN_OPTIONS from system environment anyway, and symbolize_vs_style=false *is* the default?

That is, clearing ASAN_OPTIONS is fine, what worries us is adding default ASAN_OPTIONS for all
tests that would not match the default configuration we're shipping to users.

About this specific change:
It's surprising (to me) to have ASan, by default, exit() the process, as if
"no error" had happened (by which I mean: The process exited in a "normal"
way, even though it exited with a failure exit code). This doesn't affect
me much, as an exit() on my platform is also "weird". But on Mac OS X it
feels weird to not have it crash.

Regards,

Filipe

[filcab]

That was done because on some platforms (like the PS4. Possibly (some)
Windows people would like it too) symbolize_vs_style=true is the default.
This build configuration wouldn't allow anyone to test the sanitizer
libraries *and* ship the binary they tested with.

Making sure that the platform's defaults are ok is a matter of creating a
test which only runs on that platform and matches whatever defaults are
different from "the usual defaults".

Having the tests depend on the user remembering to set the proper
ASAN_OPTIONS for testing is much more fragile and prone to problems (and a
hidden dependency on the testing environment). And it would still require
all tests that set ASAN_OPTIONS to something also set it to
ASAN_OPTIONS=$ASAN_OPTIONS:... anyway.

Filipe

[zaks.anna]

I'm sympathetic to your concerns regarding
running tests in a different configuration that is shipped by default to the end users, and this can apply
to different abort_on_error values on different OSes, but not for the existing state.

While I am also sympathetic, I prefer having a more complicated tests setup to a bad end user experience, which is what we get on darwin with the current behavior (I believe we've reached a consensus on this above).

I recall people getting confused by non-crashing behavior on other platforms as well. Here are a few links I found with instructions to manually reset the default by doing a quick search:

https://code.google.com/p/address-sanitizer/issues/detail?id=345
https://groups.google.com/forum/#!searchin/address-sanitizer/abort_on_error/address-sanitizer/ROkIGdTicgg/gDc3wLlbrr8J
https://code.google.com/p/address-sanitizer/wiki/AddressSanitizerAndDebugger
https://llvm.org/bugs/show_bug.cgi?id=16276
https://fuzzing-project.org/tutorial2.html
https://trac.webkit.org/wiki/ASanWebKit

[filcab]

It seems abort_on_error-linux.cc could be called abort_on_error-default (or posix, or whatever), but that's just a nit on my part (I'm ok with it being *-linux) :-)

Where are you filtering which tests get executed, though? It looks like both will always be executed, and one will fail.
For now, I guess a "REQUIRES: darwin", and a "UNSUPPORTED: darwin" on the tests should be good enough.

lib/asan/asan_flags.inc

81 ↗

(On Diff #30247)

Why not just SANITIZER_MAC? Possibly even SANITIZER_MAC || SANITIZER_IOSSIM || SANITIZER_IOS, depending on what behavior you want for iOS and the iOS simulator.
Those identifiers are always defined to 1 or 0.

lib/asan/tests/asan_test_main.cc

18 ↗

(On Diff #30247)

Why do you do this here, if lit.cfg already sets ASAN_OPTIONS?

test/asan/TestCases/Darwin/abort_on_error-darwin.cc

2 ↗

(On Diff #30247)

I don't think the "See also ..." part is useful, and will quickly become annoying as other platforms are added.

[kubamracek]

Where are you filtering which tests get executed, though? It looks like both will always be executed, and one will fail.

It's based on the directory the test is in. Tests in TestCases/Darwin/ are only run on Darwin.

... Possibly even SANITIZER_MAC || SANITIZER_IOSSIM || SANITIZER_IOS...

SANITIZER_MAC is 1 on all these three platforms.

Why do you do this here, if lit.cfg already sets ASAN_OPTIONS?

Unit tests don't use settings from test/asan/lit.cfg, AFAIK.

[kcc]

Ok. You seem to be serious about this.
Let us

• Set asan_abort_on_error=(SANITIZER_MAC | ...) as you have it now
• Update config.environment['ASAN_OPTIONS'] but only under "if OSX"
• Same for asan_test_main.cc

There will be a tax: please work with filcab to remove config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false from the main path in lit.cfg
I want it to be done only for those platforms where he needs this to happen -- but not by default and not on Linux

test/asan/TestCases/Darwin/abort_on_error-darwin.cc

7 ↗

(On Diff #30247)

add a line w/o resetting ASAN_OPTIONS. On mac it will exit() because of the change in lit.cfg. We want to still test for it so that we don't run all tests with abort_on_error.
Update the comments.

test/asan/TestCases/Linux/abort_on_error-linux.cc

7 ↗

(On Diff #30247)

add a line w/o ASAN_OPTIONS and update the comment.
We don't want to set ASAN_OPTIONS on Linux in lit.cfg

[kubamracek]

Kostya, thanks for dealing with this!

Changed (SANITIZER_MAC == 1) to just SANITIZER_MAC. Removed the whole __asan_default_options thing for ASan unit tests and instead moved in into test/asan/Unit/lit.site.cfg.in. Addressed other comments.

I don't think the "See also ..." part is useful, and will quickly become annoying as other platforms are added.

I think it's important to note that the two tests are related (if you're editing one, you should be aware of the other). Can we keep this?

There will be a tax: please work with filcab to remove config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false from the main path in lit.cfg
 I want it to be done only for those platforms where he needs this to happen -- but not by default and not on Linux

Filipe, could you tell me on what platforms do you need to override symbolize_vs_style? Also, it looks to me that it's always false by default for all platforms... Are you talking about out-of-tree modifications of the defaults?

Thanks!

[filcab]

Hi Kuba,

[kcc]

Ok, we are converging!

test/asan/Unit/lit.site.cfg.in

34 ↗

(On Diff #30485)

I'd much prefer the previous way of doing this (in asan_test_main.cc)
because it gives us the same results when the test is being executed manually.

test/asan/lit.cfg

32 ↗

(On Diff #30485)

Please add a comment explaining why we set this in the test (abort is slower)

[kubamracek]

Addressed review comments.

Filipe, could you tell me on what platforms do you need to override symbolize_vs_style?

It's false on all platforms in open source.

In that case, let's remove the config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false' line from open-source completely. You can have it in your out-of-tree changes. If we change the open-source default on Windows, we can add this back.

[filcab]

Looks fine on my side. Thanks!
Let's see if Kostya agrees.

[kcc]

LGTM with one small comment.

Thanks for you patience!

lib/asan/tests/asan_test_main.cc

18 ↗

(On Diff #30569)

we use SANITIZER_MAC

compiler-rt/trunk/lib/asan/asan_flags.inc

ASAN_FLAG(
     "terminating the program. Useful for debugging purposes (e.g. when one "
     "needs to attach gdb).")
 ASAN_FLAG(bool, check_malloc_usable_size, true,
           "Allows the users to work around the bug in Nvidia drivers prior to "
           "295.*.")
 ASAN_FLAG(bool, unmap_shadow_on_exit, false,
           "If set, explicitly unmaps the (huge) shadow at exit.")
 ASAN_FLAG(
-    bool, abort_on_error, false,
+    bool, abort_on_error, SANITIZER_MAC,
     "If set, the tool calls abort() instead of _exit() after printing the "
     "error report.")
 ASAN_FLAG(bool, print_stats, false,
           "Print various statistics after printing an error message or if "
           "atexit=1.")
 ASAN_FLAG(bool, print_legend, true, "Print the legend for the shadow bytes.")
 ASAN_FLAG(bool, atexit, false,
           "If set, prints ASan exit stats even after program terminates "

compiler-rt/trunk/lib/asan/tests/asan_test_main.cc

 //===-- asan_test_main.cc -------------------------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of AddressSanitizer, an address sanity checker.
 //
 //===----------------------------------------------------------------------===//
 #include "asan_test_utils.h"
+#include "sanitizer_common/sanitizer_platform.h"
 
 // Default ASAN_OPTIONS for the unit tests. Let's turn symbolication off to
 // speed up testing (unit tests don't use it anyway).
 extern "C" const char* __asan_default_options() {
+#if SANITIZER_MAC
+  // On Darwin, we default to `abort_on_error=1`, which would make tests run
+  // much slower. Let's override this and run lit tests with 'abort_on_error=0'.
+  return "symbolize=false:abort_on_error=0";
+#else
   return "symbolize=false";
+#endif
 }
 
 int main(int argc, char **argv) {
   testing::GTEST_FLAG(death_test_style) = "threadsafe";
   testing::InitGoogleTest(&argc, argv);
   return RUN_ALL_TESTS();
 }

compiler-rt/trunk/test/asan/TestCases/Darwin/abort_on_error.cc

+// Check that with empty ASAN_OPTIONS, ASan reports on OS X actually crash
+// the process (abort_on_error=1). See also Linux/abort_on_error.cc.
+
+// RUN: %clangxx_asan %s -o %t
+
+// Intentionally don't inherit the default ASAN_OPTIONS.
+// RUN: ASAN_OPTIONS="" not --crash %run %t 2>&1 | FileCheck %s
+// When we use lit's default ASAN_OPTIONS, we shouldn't crash.
+// RUN: not %run %t 2>&1 | FileCheck %s
+
+#include <stdlib.h>
+int main() {
+  char *x = (char*)malloc(10 * sizeof(char));
+  free(x);
+  return x[5];
+  // CHECK: {{.*ERROR: AddressSanitizer: heap-use-after-free on address}}
+}

compiler-rt/trunk/test/asan/TestCases/Linux/abort_on_error.cc

+// Check that with empty ASAN_OPTIONS, ASan reports on Linux don't crash
+// the process (abort_on_error=0). See also Darwin/abort_on_error.cc.
+
+// RUN: %clangxx_asan %s -o %t
+
+// Intentionally don't inherit the default ASAN_OPTIONS.
+// RUN: ASAN_OPTIONS="" not run %t 2>&1 | FileCheck %s
+// When we use lit's default ASAN_OPTIONS, we shouldn't crash either. On Linux
+// lit doesn't set ASAN_OPTIONS anyway.
+// RUN: not %run %t 2>&1 | FileCheck %s
+
+#include <stdlib.h>
+int main() {
+  char *x = (char*)malloc(10 * sizeof(char));
+  free(x);
+  return x[5];
+  // CHECK: {{.*ERROR: AddressSanitizer: heap-use-after-free on address}}
+}

compiler-rt/trunk/test/asan/lit.cfg

def push_dynamic_library_lookup_path(config, new_path):
 
   new_ld_library_path = os.path.pathsep.join(
     (new_path, config.environment.get(dynamic_library_lookup_var, '')))
   config.environment[dynamic_library_lookup_var] = new_ld_library_path
 
 # Setup config name.
 config.name = 'AddressSanitizer' + config.name_suffix
 
-# Setup default ASAN_OPTIONS
-config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false'
+# Platform-specific default ASAN_OPTIONS for lit tests.
+if config.host_os == 'Darwin':
+  # On Darwin, we default to `abort_on_error=1`, which would make tests run
+  # much slower. Let's override this and run lit tests with 'abort_on_error=0'.
+  config.environment['ASAN_OPTIONS'] = 'abort_on_error=0'
 
 # testFormat: The test format to use to interpret tests.
 external_bash = (not sys.platform in ['win32'])
 config.test_format = lit.formats.ShTest(external_bash)
 
 # Setup source root.
 config.test_source_root = os.path.dirname(__file__)