[PATCH] Tests and interface for LeakSanitizer.
Alexander Potapenko
glider at google.com
Tue Apr 16 10:50:14 PDT 2013
================
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
More information about the llvm-commits
mailing list