[PATCH] [lsan] Convert all LSan tests to LIT tests. Remove the old testing interface.

Sergey Matveev earthdok at google.com
Fri May 24 05:04:31 PDT 2013



================
Comment at: lib/lsan/lit_tests/stale_stack_leak.cc:15
@@ +14,3 @@
+// Hopefully the argument p will be passed on a register, saving us from false
+// positives.
+__attribute__((noinline))
----------------
Kostya Serebryany wrote:
> I don't understand this comment? 
> did you mean False negatives? 
I don't know how I keep confusing them.

================
Comment at: lib/lsan/lit_tests/stale_stack_leak.cc:30
@@ +29,3 @@
+
+// Hopefully this will run after LSan.
+__attribute__((destructor))
----------------
Kostya Serebryany wrote:
> Hm?
> 
I'll reword the comment.

================
Comment at: lib/lsan/lit_tests/use_stacks_threaded.cc:30
@@ +29,3 @@
+    pthread_yield();
+  exit(0);
+  return 0;
----------------
Kostya Serebryany wrote:
> exit(0) followed by return 0 ? 
I expected a compiler warning about no return statement. Removed.

================
Comment at: lib/lsan/lit_tests/use_tls_dynamic.cc:2
@@ +1,3 @@
+// RUN: LSAN_BASE="report_blocks=1:use_stacks=0:use_registers=0"
+// RUN: %clangxx_lsan -fno-sanitize=leak -fno-sanitize=address %p/SharedLibs/huge_tls_lib_so.cc -fPIC -shared -o %t-so.so
+// RUN: %clangxx_lsan %s -o %t
----------------
Kostya Serebryany wrote:
> Should this be something similar, like clangxx ? 
I'd agree if it was used more than once, but is it really worth creating an extra substitution just for this line? It's going to take several lines in the lit config.

================
Comment at: lib/lsan/lit_tests/large_allocation_leak.cc:3
@@ +2,3 @@
+// RUN: %clangxx_lsan %s -o %t
+// RUN: LSAN_OPTIONS=$LSAN_BASE:"use_globals=0" %t 2>&1 | FileCheck --check-prefix=CHECK-leak %s
+// RUN: LSAN_OPTIONS=$LSAN_BASE:"use_globals=1" %t 2>&1 | FileCheck --check-prefix=CHECK-noleak %s
----------------
Kostya Serebryany wrote:
> I think you should also run with default options and check for leaks, 
> i.e. add 3-rd line to ever test (where applicable) with empty LSAN_OPTIONS
Ok.

================
Comment at: lib/lsan/lit_tests/test_defs.h:2
@@ +1,3 @@
+#ifndef TEST_DEFS_H
+#define TEST_DEFS_H
+
----------------
Kostya Serebryany wrote:
> I'd really prefer not to have this common file. 
> Standalone .cc test files have their own coolness. 
ok

================
Comment at: lib/lsan/lit_tests/use_globals_initialized.cc:20
@@ +19,2 @@
+// CHECK-leak: Directly leaked 1337 byte block at [[ADDR]]
+// CHECK-noleak-NOT: block at [[ADDR]]
----------------
Kostya Serebryany wrote:
> here and in all other tests: 
> the test should not have *any* leak, not just a leak on ADDR, so 
> replace this with something like 
>   CHECK-noleak-NOT: block at
> or  
>   CHECK-noleak-NOT: LeakSanitizer
ok
I also added an extra check for fatal failures.


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



More information about the llvm-commits mailing list