Page MenuHomePhabricator

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

Authored by kubamracek on Jan 27 2015, 10:56 AM.

Details

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()).

Diff Detail

Repository
rL LLVM

Event Timeline

kubamracek retitled this revision from to [compiler-rt] Turn abort_on_error=1 by default on OS X (part 1/2).
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), glider, samsonov and 2 others.
kcc added a comment.Jan 27 2015, 12:04 PM

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?

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 added inline comments.Jan 27 2015, 4:26 PM
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 updated this revision to Diff 18869.Jan 27 2015, 5:32 PM

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!

Why not enable it on all platforms btw?

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?).

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 updated this revision to Diff 19560.Feb 8 2015, 4:15 PM

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

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?

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 edited edge metadata.Mar 5 2015, 2:37 PM

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.

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 added a comment.Mar 6 2015, 9:19 AM

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

  1. 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?

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 added a comment.Mar 6 2015, 2:48 PM

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

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 added a comment.Mar 6 2015, 5:40 PM

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.

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 added a comment.Mar 9 2015, 3:23 PM

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

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.

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.

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 added a comment.Mar 11 2015, 11:52 AM

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 updated this revision to Diff 30247.Jul 21 2015, 5:32 AM
kubamracek retitled this revision from [compiler-rt] Turn abort_on_error=1 by default on OS X (part 1/2) to [asan] Set abort_on_error=1 by default on OS X.
kubamracek updated this object.
kubamracek added reviewers: samsonov, glider.

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 accepted this revision.Jul 21 2015, 12:18 PM
samsonov edited edge metadata.

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

This revision is now accepted and ready to land.Jul 21 2015, 12:18 PM
kcc added a comment.Jul 21 2015, 2:56 PM

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

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

kcc added a comment.Jul 21 2015, 3:20 PM

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.

In D7203#209307, @kcc 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 added a comment.Jul 21 2015, 3:30 PM

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.

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

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

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

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

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.

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 added a comment.Jul 22 2015, 1:24 PM

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 updated this revision to Diff 30485.Jul 23 2015, 7:52 AM
kubamracek edited edge metadata.

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!

kcc added a comment.Jul 23 2015, 6:36 PM

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 updated this revision to Diff 30569.Jul 24 2015, 5:13 AM

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 accepted this revision.Jul 24 2015, 8:57 AM
filcab added a reviewer: filcab.

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

kcc accepted this revision.Jul 27 2015, 12:33 PM
kcc edited edge metadata.

LGTM with one small comment.

Thanks for you patience!

lib/asan/tests/asan_test_main.cc
18 ↗(On Diff #30569)

we use SANITIZER_MAC

This revision was automatically updated to reflect the committed changes.