[PATCH] Tests and interface for LeakSanitizer.

[Sergey Matveev]

================
Comment at: lib/lsan/lsan_interface.h:41
@@ +40,3 @@
+
+void __lsan_set_pointer_alignment(uptr alignment);
+
----------------
Alexey Samsonov wrote:
> This function may return the old value of alignment. (Alternatively, add get_pointer_alignment function).
I don't see a use case for this tbh.

================
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
----------------
Alexey Samsonov wrote:
> The comment doesn't correspond to the test case below - there is no small allocation there.
Reworded.

================
Comment at: lib/lsan/tests/lsan_test.cc:95
@@ +94,3 @@
+
+volatile void *data_var = (void*) 1;
+
----------------
Alexander Potapenko wrote:
> Alexey Samsonov wrote:
> > I think you can avoid wasting global namespace by anonymous namespaces (here and below)
> A single anonymous namespace should be actually enough.
Fixed.

================
Comment at: lib/lsan/tests/lsan_test.cc:120
@@ +119,3 @@
+  bss_var = malloc(kSmallAllocSize);
+  EXPECT_TRUE(NoLeaks());
+  __lsan_disable_source(LSAN_SOURCE_GLOBALS);
----------------
Alexander Potapenko wrote:
> FYI: this whole "var = malloc; check for leaks; free(var)" thing can be moved into a separate function.
Carryover from when I couldn't disable stack and thus avoided function calls. Fixed.

================
Comment at: lib/lsan/tests/lsan_test.cc:115
@@ +114,3 @@
+TEST(LeakSanitizer, UninitializedGlobals) {
+  __lsan_enable_all_sources();
+  __lsan_disable_source(LSAN_SOURCE_STACKS);
----------------
Alexey Samsonov wrote:
> 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.
Done.

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

================
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");
----------------
Alexey Samsonov wrote:
> 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).
Done

================
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());
----------------
Alexey Samsonov wrote:
> typedef for void**(*store)(void*) ?
Done

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

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

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

================
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);
----------------
Alexey Samsonov wrote:
> I think static_cast will work (here and below)
Keeping reinterpret_cast as per http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Casting#Casting

================
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);
----------------
Alexander Potapenko wrote:
> Alexey Samsonov wrote:
> > Curious: why not plain "memcpy"?
> Yeah, I think we'd better avoid including too much sanitizer_common headers. Ideally your test should only depend on lsan_interface.h
fixed

================
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
+
----------------
Alexander Potapenko wrote:
> The variable name is too vague.
I hope it's better now.

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

================
Comment at: lib/asan/asan_rtl.cc:67
@@ -66,3 +66,3 @@
 // -------------------------- Flags ------------------------- {{{1
-static const int kDeafultMallocContextSize = 30;
+static const int kDefaultMallocContextSize = 30;
 
----------------
Glitch - please disregard this.


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