Page MenuHomePhabricator

[ASan tests] Add Windows-specific flags to lib/asan/tests/CMakeLists.txt
ClosedPublic

Authored by timurrrr on May 12 2014, 4:21 AM.

Details

Reviewers
samsonov
Summary

[CC Hans in case we can improve some things in the clang-cl driver]

Diff Detail

Event Timeline

timurrrr updated this revision to Diff 9306.May 12 2014, 4:21 AM
timurrrr retitled this revision from to [ASan tests] Add Windows-specific flags to lib/asan/tests/CMakeLists.txt.
timurrrr updated this object.
timurrrr edited the test plan for this revision. (Show Details)
timurrrr added a reviewer: samsonov.
timurrrr added subscribers: Unknown Object (MLST), rnk, hans.
hans added inline comments.May 12 2014, 8:47 AM
lib/asan/tests/CMakeLists.txt
75

I think adding -fsanitize-blacklist (and possible others) and -mllvm to clang-cl's supported flags would be reasonable.

I think adding -fsanitize-blacklist (and possible others) and -mllvm to
clang-cl's supported flags would be reasonable.

Sounds good!
I assume it shouldn't block my patch.

kcc added a subscriber: kcc.May 12 2014, 9:22 AM
kcc added inline comments.
lib/asan/tests/CMakeLists.txt
74

Can you just remove these two lines (asan-stack and asan-globals)?

timurrrr updated this revision to Diff 9319.May 12 2014, 9:28 AM

Removed two redundant lines per Kostya's comment.
These flags are now =1 by default.

lib/asan/tests/CMakeLists.txt
74

Done

samsonov edited edge metadata.May 12 2014, 10:50 AM

-Xclang is horrible. Please add a FIXME assigned to you to remove this as soon as the necessary flags are supported by clang-cl.

lib/asan/tests/CMakeLists.txt
77

Why do you need this? If there is no libstdc++ on Windows, you should instead detect this in the build system and introduce
COMPILER_RT_HAS_LIBSTDCXX like we do for libpthread and libdl.

106

ditto

OK, sounds reasonable.

Do I need to do another round of review when I address these three comments?

samsonov accepted this revision.May 12 2014, 10:57 AM
samsonov edited edge metadata.

Feel free to submit after addressing comments.

lib/asan/tests/CMakeLists.txt
34

You can use COMPILER_RT_HAS_G_FLAG / COMPILER_RT_HAS_Zi_FLAG here.

This revision is now accepted and ready to land.May 12 2014, 10:57 AM
timurrrr closed this revision.May 13 2014, 3:44 AM

Landed everything except for one questionable if() in r208682.

lib/asan/tests/CMakeLists.txt
77

We had an offline discussion here and wonder why doesn't the driver itself pass this flag?

We also have to do if(NOT MSVC) for the recently-introduced --driver=g++ flag here anyways, so not clear if the extra variable is worth it.

(we can probably discuss that on the commit email)

OK, I've added COMPILER_RT_HAS_LIBSTDCXX in r208695.