This is an archive of the discontinued LLVM Phabricator instance.

[ASan] Test churn for setting ASAN_OPTIONS=symbolize_vs_style=false
ClosedPublic

Authored by filcab on Jun 5 2015, 7:38 PM.

Details

Summary

This commit adds symbolize_vs_style=false to every instance of
ASAN_OPTIONS in the asan tests and sets
ASAN_OPTIONS=symbolize_vs_style=false in lit, for tests which don't set
it.

This way we don't need to make the tests be able to deal with both
symbolize styles.

This is the first patch in the series. I will eventually submit for the
other sanitizers too.

We need this change (or another way to deal with the different outputs) in
order to be able to default to symbolize_vs_style=true on some platforms.

Diff Detail

Repository
rL LLVM

Event Timeline

filcab updated this revision to Diff 27254.Jun 5 2015, 7:38 PM
filcab retitled this revision from to [ASan] Test churn for setting ASAN_OPTIONS=symbolize_vs_style=false.
filcab updated this object.
filcab edited the test plan for this revision. (Show Details)
filcab added reviewers: samsonov, kcc, rnk.
filcab added a subscriber: Unknown Object (MLST).
rnk edited edge metadata.Jun 8 2015, 1:42 PM

This is verbose, it makes me think we want some llvm-env command that supports something like ASAN_OPTIONS+=foo. Let's see what Alexey says.

samsonov edited edge metadata.Jun 9 2015, 5:22 PM

Wait a second, why can't you set

config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false'

in lit configs, and then use ASAN_OPTIONS+= or ASAN_OPTIONS=$ASAN_OPTIONS:<....> in the tests which set this env variable?
In this case you won't have to update all the test cases every time you want to set the default value of certain runtime flag for the test suite.

Wait a second, why can't you set

config.environment['ASAN_OPTIONS'] = 'symbolize_vs_style=false'

OK, I see that you've already done it, my bad, but...

ASAN_OPTIONS=$ASAN_OPTIONS:<....> in the tests which set this env variable?

^^
Why can't we use this form instead?

filcab added a comment.Jun 9 2015, 5:38 PM

(I said on IRC, but I'll repeat to have it included in the thread)

env ASAN_OPTIONS+=blah=true
doesn't work on Windows, unfortunately.

I can change it to either (I think any of these would be preferable to the
current patch):

  • use Reid's idea of making a (yet another) single purpose utility

(llvm-env)

  • use expansion (ASAN_OPTIONS=$ASAN_OPTIONS:...) instead of spelling out

symbolize_vs_style=true again (I didn't think about this one)

  • change lit's shell, if we're sure that's the one that's being used.

Thanks,

Filipe
filcab added a comment.Jun 9 2015, 5:41 PM

Will do. I should have thought of that one, but something wasn't working
very well that day :-)

Will update.

Thanks,

Filipe

I can change it to either (I think any of these would be preferable to the
current patch):

  • use Reid's idea of making a (yet another) single purpose utility

(llvm-env)

  • use expansion (ASAN_OPTIONS=$ASAN_OPTIONS:...) instead of spelling out

symbolize_vs_style=true again (I didn't think about this one)

^^
Right, let's use this one. We already use similar construct in another places

$ grep -rnH "$.SAN_OPTIONS" projects/compiler-rt/test/*
  • change lit's shell, if we're sure that's the one that's being used.

Thanks,

Filipe
filcab updated this revision to Diff 27536.Jun 11 2015, 12:31 PM
filcab edited edge metadata.

Added "env " before any run lines which set variables and call %run.

samsonov added inline comments.Jun 12 2015, 11:14 AM
test/asan/TestCases/Darwin/suppressions-darwin.cc
7 ↗(On Diff #27536)

Looks like quotes should enclose the whole string here

ASAN_OPTIONS="$ASAN_OPTIONS:suppressions='%t.supp'"
test/asan/TestCases/Linux/leak.cc
6 ↗(On Diff #27536)

Huh? Why not

env ASAN_OPTIONS=$ASAN_OPTIONS

?

test/asan/TestCases/Linux/malloc_delete_mismatch.cc
33 ↗(On Diff #27536)

No, I don't think we need to modify this line ;)

test/asan/TestCases/Posix/log_path_fork_test.cc.disabled
4 ↗(On Diff #27536)

Same note about quotes.

test/asan/TestCases/Windows/report_globals_reload_dll.cc
4 ↗(On Diff #27536)

You sure you need both?

test/asan/TestCases/atol_strict.c
4 ↗(On Diff #27536)

This test doesn't have $ASAN_OPTIONS, is it intentional?

test/asan/TestCases/atoll_strict.c
4 ↗(On Diff #27536)

This test doesn't have $ASAN_OPTIONS, is it intentional?

test/asan/TestCases/strcasestr-1.c
6 ↗(On Diff #27536)

This test doesn't have $ASAN_OPTIONS, is it intentional?

test/asan/TestCases/strcasestr_strict.c
3 ↗(On Diff #27536)

This test doesn't have $ASAN_OPTIONS, is it intentional?(more cases below)

test/asan/TestCases/suppressions-exec-relative-location.cc
12 ↗(On Diff #27536)

quotes (here and below)

filcab updated this revision to Diff 27622.Jun 12 2015, 5:51 PM

Fixed problems pointed out by Alexey.
Included a bunch of tests I had missed.

Thanks Alexey. I missed a bunch of places that were messed up. I also tried
to make sure I got all files this time (I had missed some of them).

Filipe

samsonov accepted this revision.Jun 13 2015, 10:22 AM
samsonov edited edge metadata.

LGTM, thanks! Single comment below.

test/asan/TestCases/stack-use-after-return.cc
15 ↗(On Diff #27622)

Extra $ASAN_OPTIONS here.

16 ↗(On Diff #27622)

And here

This revision is now accepted and ready to land.Jun 13 2015, 10:22 AM
This revision was automatically updated to reflect the committed changes.

This got committed, but Windows doesn't have the 'env' command, so I reverted that directory.
I'll try to figure out something for those.