[PATCH] Tests and interface for LeakSanitizer.

Sergey Matveev earthdok at google.com
Fri Apr 19 10:25:11 PDT 2013



================
Comment at: lib/lsan/lsan_interface.h:40
@@ +39,3 @@
+void SetSources(uptr sources);
+void SetPointerAlignment(uptr alignment);
+
----------------
Kostya Serebryany wrote:
> Why do you need these functions at all? 
> Why not just two more parameters to ReportLeaks? 
> 
It makes sense, but the readability of the tests would suffer. The use of masks has already complicated simple operations like "flip one source flag and keep the rest unchanged". Now we would also have to specify pointer alignment everywhere -- even though there's only one test that has anything to do with it. Besides, internally I'm storing those flags in global state anyway (in non-test usage they're not supposed to change after initialization, so there's no sense in passing them into every function that uses them). So the functions wouldn't go away -- I'd just move the call to Set{Sources,PointerAlignment} into ReportLeaks.

================
Comment at: lib/lsan/tests/lsan_tls_loadable.cc:19
@@ +18,3 @@
+// space (see STATIC_TLS_SURPLUS in glibc).
+__thread void *huge_thread_local_array[(1 << 20) / sizeof(void *)]; // NOLINT
+
----------------
Kostya Serebryany wrote:
> This may not build on anything that doesn't have __thread. 
> Do you plan to build this tests only on proper systems? Otherwise you need to guard the whole test with something (#if LEAK_SANITIZER?)
> 
> Also, why NOLINT? (Why not use sizeof(uptr)?)
> Do you plan to build this tests only on proper systems?

Yes. Building something called lsan_tls_loadable.so on systems with no tls would only cause confusion.

NOLINT - fixed

================
Comment at: lib/lsan/tests/lsan_test.cc:109
@@ +108,3 @@
+
+// FIXME: disable TLS tests for Android
+volatile THREADLOCAL void *tl_var;
----------------
Kostya Serebryany wrote:
> Why xifme? Just disable the whole thing for anything other than x86_64 linux (for now)
ok

================
Comment at: lib/lsan/lsan_interface.h:1
@@ +1,2 @@
+//=-- lsan_interface.h ----------------------------------------------------===//
+//
----------------
Kostya Serebryany wrote:
> so, why not rename now? 
ok


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

BRANCH
  lsan_interface_and_tests

ARCANIST PROJECT
  compiler-rt



More information about the llvm-commits mailing list