[llvm-commits] [PATCH] [msan] MemorySanitizer unit tests.

[Alexey Samsonov]

  Didn't look at unit test itself yet.


================
Comment at: cmake/Modules/CompilerRTCompile.cmake:18
@@ +17,3 @@
+
+# Link a shared library with just-built Clang.
+# clang_link_shared(<output.so>
----------------
Maybe you can move this to CompilerRTLink.cmake

================
Comment at: cmake/Modules/CompilerRTCompile.cmake:21
@@ +20,3 @@
+#                   OBJECTS <list of input objects>
+#                   LINKFLAGS <list of link flags>
+#                   DEPS <list of dependencies>)
----------------
I think for clarity you can use format
clang_link_shared(<output.so> OBJECTS <list of inputs> FLAGS <link flags> DEPS <deps>),
and then 
parse_arguments(LINK "OBJECS;FLAGS;DEPS" "" ${ARGN})
so you'll have reasonable variables
LINK_OBJECTS, LINK_FLAGS and LINK_DEPS.

================
Comment at: lib/msan/CMakeLists.txt:1
@@ -1,2 +1,2 @@
 # Build for the MemorySanitizer runtime support library.
 
----------------
+1 for moving tests into separate directory

================
Comment at: lib/msan/CMakeLists.txt:21
@@ -11,1 +20,3 @@
+  -fPIE
+  -ffreestanding)
 
----------------
Add a comment describing why we need it.

================
Comment at: lib/msan/CMakeLists.txt:30
@@ +29,3 @@
+set(MSAN_LIBCXX_CFLAGS
+  -I${MSAN_LIBCXX_PATH}/include
+  -fsanitize=memory
----------------
You have MSAN_LIBCXX_INCLUDE_CFLAGS

================
Comment at: lib/msan/CMakeLists.txt:24
@@ +23,3 @@
+# Instrumented libcxx sources and build flags.
+set(MSAN_LIBCXX_PATH ${LLVM_MAIN_SRC_DIR}/projects/libcxx)
+file(GLOB MSAN_LIBCXX_SOURCES ${MSAN_LIBCXX_PATH}/src/*.cpp)
----------------
What would happen if libcxx is not checked out? I think we need to just not build the unit tests in this case. We may need a variable defined in somewhere like projects/compiler-rt/CMakeLists.txt (or in modules) that would tell us if we have libcxx. This can be done in later commits, but please add a FIXME in this case.

================
Comment at: lib/msan/CMakeLists.txt:55
@@ +54,3 @@
+set(MSAN_UNITTEST_COMMON_CFLAGS
+  -I${MSAN_LIBCXX_PATH}/include
+  ${COMPILER_RT_GTEST_INCLUDE_CFLAGS}
----------------
MSAN_LIBCXX_INCLUDE_CFLAGS

================
Comment at: lib/msan/CMakeLists.txt:102
@@ +101,3 @@
+                CFLAGS ${ARGN} ${TARGET_CFLAGS}
+                DEPS gtest ${MSAN_RUNTIME_LIBRARIES})
+  list(APPEND ${obj_list} ${output_obj})
----------------
You need to explicitly depend on unittest header files (see my recent changes in ASan)

================
Comment at: lib/msan/CMakeLists.txt:107
@@ +106,3 @@
+macro(msan_link_shared so_list so_name arch)
+  parse_arguments(SOURCE "OBJECTS;LINKFLAGS;DEPS" "" ${ARGN})
+  get_unittest_directory(OUTPUT_DIR)
----------------
Same here, SOURCE seems pretty much irrelevant here.


http://llvm-reviews.chandlerc.com/D236