[PATCH] Tests and interface for LeakSanitizer.

[Alexander Potapenko]

================
Comment at: lib/lsan/lsan_interface.h:43
@@ +42,3 @@
+
+void __lsan_start_local_leak_checking();
+// Find unreachable chunks among those allocated since the most recent call to
----------------
What does this function do?

================
Comment at: lib/lsan/tests/lsan_test.cc:10
@@ +9,3 @@
+//
+// This file is a part of LeakSanitizer.
+//
----------------
It's a good idea to describe what's this file for in the comment.

================
Comment at: lib/lsan/tests/lsan_tls_loadable.cc:11
@@ +10,3 @@
+// This file is a part of LeakSanitizer.
+//
+//===----------------------------------------------------------------------===//
----------------
What's this file for? Don't you need build rules for this?

================
Comment at: lib/lsan/tests/lsan_tls_loadable.cc:16
@@ +15,3 @@
+// space (see STATIC_TLS_SURPLUS in libc).
+__thread void *tls[(1 << 20) / sizeof(void *)]; // NOLINT
+
----------------
The variable name is too vague.

================
Comment at: lib/lsan/tests/lsan_test.cc:161
@@ +160,3 @@
+TEST(LeakSanitizer, DynamicTLS) {
+  // Compute the path to our loadable DSO.  We assume it's in the same
+  // directory.
----------------
Can we pass the path via argv or env instead?

================
Comment at: lib/lsan/tests/lsan_test.cc:164
@@ +163,3 @@
+  char **argv = global_argv;
+  const char *basename = "liblsan_tls_loadable-x86_64.so";
+  size_t path_max = strlen(argv[0]) + 1 + strlen(basename) + 1;
----------------
Hint: you can use C++ strings here.

================
Comment at: lib/lsan/tests/lsan_test.cc:172
@@ +171,3 @@
+  void *handle = dlopen(path, RTLD_LAZY);
+  ASSERT_NE((uptr)0, (uptr)handle);
+  void **(*store)(void *p) = (void **(*)(void *p))dlsym(handle, "store");
----------------
The preferred way to assert a pointer is not NULL is:
ASSERT_TRUE(handle != NULL) 

================
Comment at: lib/lsan/tests/lsan_test.cc:174
@@ +173,3 @@
+  void **(*store)(void *p) = (void **(*)(void *p))dlsym(handle, "store");
+  ASSERT_FALSE(dlerror());
+
----------------
I'm not sure that this assertion will print the contents of dlerror() if it fails. How about:
ASSERT_EQ(dlerror(), NULL)

================
Comment at: lib/lsan/tests/lsan_test.cc:190
@@ +189,3 @@
+
+const uptr PTHREAD_KEY_2NDLEVEL_SIZE = 32;
+
----------------
Where does this constant come from?

================
Comment at: lib/lsan/tests/lsan_test.cc:120
@@ +119,3 @@
+  bss_var = malloc(kSmallAllocSize);
+  EXPECT_TRUE(NoLeaks());
+  __lsan_disable_source(LSAN_SOURCE_GLOBALS);
----------------
FYI: this whole "var = malloc; check for leaks; free(var)" thing can be moved into a separate function.

================
Comment at: lib/lsan/lsan_interface.h:38
@@ +37,3 @@
+void __lsan_enable_all_sources();
+void __lsan_enable_source(enum __lsan_sources source);
+void __lsan_disable_source(enum __lsan_sources source);
----------------
I'm curious if __lsan_enable_xxx takes any effect after __lsan_start_local_leak_checking() is called.

================
Comment at: lib/lsan/tests/lsan_test.cc:115
@@ +114,3 @@
+TEST(LeakSanitizer, UninitializedGlobals) {
+  __lsan_enable_all_sources();
+  __lsan_disable_source(LSAN_SOURCE_STACKS);
----------------
I suggest to put __lsan_enable_all_sources into the test fixture setup.

================
Comment at: lib/lsan/tests/lsan_test.cc:265
@@ +264,3 @@
+struct ThreadArgument {
+  void *hidden_p;
+  atomic_uintptr_t sync;
----------------
Methods before data members, please.


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