[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