This is an archive of the discontinued LLVM Phabricator instance.

[ASan/Win] Use clang rather than clang-cl by default for lit tests. Make Windows-only tests explicitly use clang-cl.
ClosedPublic

Authored by timurrrr on May 23 2014, 5:53 AM.

Details

Reviewers
samsonov
Summary

This patch reverts minor parts of r208802, r208697, r208682.
See D3771 for the background on the change.

Enabling the non-Windows lit test is still a TODO, I have a local patch for that.

Diff Detail

Event Timeline

timurrrr updated this revision to Diff 9752.May 23 2014, 5:53 AM
timurrrr retitled this revision from to [ASan/Win] Use clang rather than clang-cl by default for lit tests. Make Windows-only tests explicitly use clang-cl..
timurrrr updated this object.
timurrrr edited the test plan for this revision. (Show Details)
timurrrr added a reviewer: samsonov.
timurrrr added a subscriber: Unknown Object (MLST).
samsonov added inline comments.May 23 2014, 3:07 PM
cmake/Modules/AddCompilerRT.cmake
151

Restore a space here.

cmake/Modules/CompilerRTCompile.cmake
24

what if the first element of global_flags starts with / ?

34

and here.

lib/asan/tests/CMakeLists.txt
29

Consider adding this (and, probably, -DGTEST_HAS_SEH=0 on Windows) to COMPILER_RT_GTEST_CFLAGS definition instead.

31

Any particular reason to switch to -gline-tables-only from -g here?

Re: -g

  1. clang crashes on windows with -g [fixable]
  2. we had a chat with Kostya and decided it's the right thing to test ASan

with just glto to be sure it gives us enough DI
24 мая 2014 г. 2:07 пользователь "Alexey Samsonov" <vonosmas@gmail.com>
написал:

Comment at: cmake/Modules/AddCompilerRT.cmake:151
@@ -151,3 +150,3 @@

COMMAND ${COMPILER_RT_TEST_COMPILER} ${TEST_OBJECTS}
  • ${COMPILER_RT_TEST_COMPILER_EXE}"${output_bin}"

+ -o"${output_bin}"

${TEST_LINK_FLAGS}

Restore a space here.

Comment at: cmake/Modules/CompilerRTCompile.cmake:24
@@ +23,3 @@
+ if(MSVC)
+ string(REPLACE ";/" ";-" global_flags "${global_flags}")

+ endif()

what if the first element of global_flags starts with / ?

Comment at: cmake/Modules/CompilerRTCompile.cmake:33
@@ -27,3 +32,3 @@

COMMAND ${COMPILER_RT_TEST_COMPILER} ${compile_flags} -c
  • ${COMPILER_RT_TEST_COMPILER_OBJ}"${object_file}"

+ -o"${object_file}"

${source_rpath}

and here.

Comment at: lib/asan/tests/CMakeLists.txt:29
@@ -28,1 +28,3 @@

-I${COMPILER_RT_SOURCE_DIR}/lib/sanitizer_common/tests

+ -DGTEST_HAS_RTTI=0

+ -fno-rtti

Consider adding this (and, probably, -DGTEST_HAS_SEH=0 on Windows) to
COMPILER_RT_GTEST_CFLAGS definition instead.

Comment at: lib/asan/tests/CMakeLists.txt:31
@@ +30,3 @@
+ -fno-rtti
+ -gline-tables-only

+ -O2

Any particular reason to switch to -gline-tables-only from -g here?

http://reviews.llvm.org/D3893

samsonov edited edge metadata.May 23 2014, 3:11 PM

Okay, let's live with -gline-tables-info here then.

Oh, but please use COMPILER_RT_TEST_COMPILER_ID to enable it for Clang, and use plain -g otherwise.

timurrrr updated this revision to Diff 9810.May 26 2014, 9:00 AM
timurrrr edited edge metadata.

Addressed Alexey's comments.

PTAL

cmake/Modules/CompilerRTCompile.cmake
24

Good catch, fixed.

samsonov accepted this revision.May 27 2014, 12:17 PM
samsonov edited edge metadata.

LGTM

lib/sanitizer_common/tests/CMakeLists.txt
60

GTEST_HAS_SEH=0 now comes from OMPILER_RT_GTEST_CFLAGS, doesn't it?

This revision is now accepted and ready to land.May 27 2014, 12:17 PM

Landed as r209719, thanks for the review!

lib/sanitizer_common/tests/CMakeLists.txt
60

Oops, missed that -- thanks for catching!

I should probably move the stuff we conditionally append to SANITIZER_TEST_CFLAGS_COMMON to COMPILER_RT_GTEST_CFLAGS instead. Will do that in a separate commit.

timurrrr closed this revision.May 28 2014, 1:49 AM