[PATCH] Tests and interface for LeakSanitizer.

[Alexey Samsonov]

================
Comment at: lib/lsan/lsan_interface.h:10
@@ +9,3 @@
+//
+// This file is a part of LeakSanitizer.
+//
----------------
Please clarify the use of this header. Who is supposed to call functions listed here? Is it instrumented user code, or other sanitizer tools?
Usually we have "extern "C" __xsan_something()" functions that are exposed to the user and internal functions put in namespace __xsan
(or in __sanitizer).

================
Comment at: lib/lsan/lsan_interface.h:19
@@ +18,3 @@
+// Sources of pointers.
+enum __lsan_sources {
+  // Global variables (.data and .bss).
----------------
I think you can name it "__lsan_source" and drop "enum" in function declaration below

================
Comment at: lib/lsan/lsan_interface.h:41
@@ +40,3 @@
+
+void __lsan_set_pointer_alignment(uptr alignment);
+
----------------
This function may return the old value of alignment. (Alternatively, add get_pointer_alignment function).

================
Comment at: lib/lsan/tests/lsan_test.cc:61
@@ +60,3 @@
+// allocations get in the way.
+::testing::AssertionResult IsNotLeaked(void *hidden_p) {
+  void *leaks_array[kMaxLeaks];
----------------
I think you can merge this function with IsLeaked()

================
Comment at: lib/lsan/tests/lsan_test.cc:95
@@ +94,3 @@
+
+volatile void *data_var = (void*) 1;
+
----------------
I think you can avoid wasting global namespace by anonymous namespaces (here and below)

================
Comment at: lib/lsan/tests/lsan_test.cc:74
@@ +73,3 @@
+
+// Running every test for both large and small allocations is unnecessary, but
+// we should check for false negatives, in case some data structures used by
----------------
The comment doesn't correspond to the test case below - there is no small allocation there.

================
Comment at: lib/lsan/tests/lsan_test.cc:142
@@ +141,3 @@
+
+volatile THREADLOCAL void *tl_var;
+
----------------
You need to somehow check if we have THREADLOCAL. At least add a FIXME

================
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;
----------------
Alexander Potapenko wrote:
> Hint: you can use C++ strings here.
This is a string constant, you may use "const std::string kLoadableSO = "....";

================
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");
----------------
Alexander Potapenko wrote:
> The preferred way to assert a pointer is not NULL is:
> ASSERT_TRUE(handle != NULL) 
You may also print the result of dlerror() here in assert failure message (here and in dlsym below).

================
Comment at: lib/lsan/tests/lsan_test.cc:173
@@ +172,3 @@
+  ASSERT_NE((uptr)0, (uptr)handle);
+  void **(*store)(void *p) = (void **(*)(void *p))dlsym(handle, "store");
+  ASSERT_FALSE(dlerror());
----------------
typedef for void**(*store)(void*) ?

================
Comment at: lib/lsan/tests/lsan_test.cc:278
@@ +277,3 @@
+void *StackThreadFunc(void *param) {
+  ThreadArgument *arg = reinterpret_cast<ThreadArgument *>(param);
+  arg->sync_store(1);
----------------
I think static_cast will work (here and below)

================
Comment at: lib/lsan/tests/lsan_test.cc:115
@@ +114,3 @@
+TEST(LeakSanitizer, UninitializedGlobals) {
+  __lsan_enable_all_sources();
+  __lsan_disable_source(LSAN_SOURCE_STACKS);
----------------
Alexander Potapenko wrote:
> I suggest to put __lsan_enable_all_sources into the test fixture setup.
I'm mildly opposed to this: seems it's not always called at the very beginning of the test.

================
Comment at: lib/lsan/tests/lsan_test.cc:27
@@ +26,3 @@
+namespace __lsan {
+char **global_argv;
+uptr kMagic = 0xBABABABABABABABA;
----------------
Global variables in this file can be static.

================
Comment at: lib/lsan/tests/lsan_test.cc:389
@@ +388,3 @@
+  void *p = malloc(kSmallAllocSize);
+  internal_memcpy(char_arr + 1, &p, sizeof(uptr));
+  EXPECT_LEAKED(p);
----------------
Curious: why not plain "memcpy"?

================
Comment at: lib/lsan/tests/lsan_test.cc:399
@@ +398,3 @@
+}  // namespace __lsan
+
+int main(int argc, char **argv) {
----------------
I would add a few stress tests: tests with lots of malloc/frees, possibly threaded. This may be done in following commits.


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