[PATCH] [lsan] CMakeLists and lit test configs for LeakSanitizer.
Alexey Samsonov
samsonov at google.com
Mon May 20 02:18:20 PDT 2013
Overall, this looks reasonable.
================
Comment at: lib/lsan/CMakeLists.txt:6
@@ +5,3 @@
+ -I${COMPILER_RT_SOURCE_DIR}/lib
+ ${COMPILER_RT_GTEST_INCLUDE_CFLAGS}
+ )
----------------
This and the previous compile flag should not be needed for building LSan.
================
Comment at: lib/lsan/CMakeLists.txt:21
@@ +20,3 @@
+set(LSAN_RUNTIME_LIBRARIES)
+ set(arch "x86_64")
+ add_compiler_rt_static_runtime(clang_rt.lsan-${arch} ${arch}
----------------
weird indentation
================
Comment at: lib/lsan/CMakeLists.txt:16
@@ +15,3 @@
+ lsan_common_linux.cc
+ )
+
----------------
nit: I prefer
set(VAR
a
b)
to
set(VAR
a
b
)
I know it's not consistent in compiler-rt rules, but let's stick to the former in the new files.
================
Comment at: lib/lsan/CMakeLists.txt:25
@@ +24,3 @@
+ $<TARGET_OBJECTS:RTInterception.${arch}>
+ $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>
+ CFLAGS ${LSAN_CFLAGS}
----------------
check if you need new weird RTSanitizerCommonLibc stuff.
I haven't looked at this part yet.
================
Comment at: lib/lsan/tests/CMakeLists.txt:25
@@ +24,3 @@
+ clang_compile(${output_obj} ${source}
+ CFLAGS ${ARGN} ${TARGET_CFLAGS}
+ DEPS gtest)
----------------
weird indentation here and below
================
Comment at: lib/lsan/tests/CMakeLists.txt:32
@@ +31,3 @@
+ set(testname_arch ${testname}-${arch}-Test)
+ get_target_flags_for_arch(${arch} TARGET_LINKFLAGS)
+ add_unittest(LsanTests ${testname_arch} ${ARGN})
----------------
TARGET_LINKFLAGS is unused.
================
Comment at: lib/lsan/tests/CMakeLists.txt:36
@@ +35,3 @@
+ set_target_compile_flags(${testname_arch} ${LSAN_CFLAGS})
+ set_property(TARGET ${testname_arch} APPEND_STRING
+ PROPERTY LINK_FLAGS " -pie")
----------------
you may want to use set_target_link_flags
================
Comment at: lib/lsan/tests/CMakeLists.txt:69
@@ +68,3 @@
+ ${LSAN_RUNTIME_LIBRARIES}
+ )
+ set(LSAN_TEST_PARAMS
----------------
see nit about closing brace above
================
Comment at: lib/lsan/tests/CMakeLists.txt:55
@@ +54,3 @@
+ AND CAN_TARGET_x86_64
+ AND CMAKE_SIZEOF_VOID_P EQUAL 8
+ AND NOT LLVM_BUILD_32_BITS)
----------------
remove this and the following line
================
Comment at: lib/lsan/tests/CMakeLists.txt:52
@@ +51,3 @@
+
+# Build on 64-bit Linux.
+if(UNIX AND NOT APPLE
----------------
Change comment to "Build tests for 64-bit Linux only"
================
Comment at: lib/lsan/tests/CMakeLists.txt:49
@@ +48,3 @@
+ add_dependencies(Lsan-${arch}-Test lsan_tls_loadable-${arch})
+ message(${CMAKE_CURRENT_BINARY_DIR})
+endmacro()
----------------
debug print?
http://llvm-reviews.chandlerc.com/D804
More information about the llvm-commits
mailing list