[PATCH] Interceptors etc for standalone LSan.
Dmitry Vyukov
dvyukov at google.com
Tue Apr 23 04:12:47 PDT 2013
================
Comment at: lib/lsan/lsan_allocator.h:1
@@ +1,2 @@
+//=-- lsan_allocator.h ----------------------------------------------------===//
+//
----------------
I think we need to put it into lsan/src/ or something.
================
Comment at: lib/lsan/lsan_allocator.h:33
@@ +32,3 @@
+
+void *Allocate(StackTrace const &stack, uptr size, uptr alignment,
+ bool cleared = false);
----------------
In the codebase we use "const T &" instead of "T const&"
================
Comment at: lib/lsan/lsan_allocator.h:34
@@ +33,3 @@
+void *Allocate(StackTrace const &stack, uptr size, uptr alignment,
+ bool cleared = false);
+void Deallocate(void *p);
----------------
Alexander Potapenko wrote:
> I think it's better to avoid the default parameter here.
Default values are discouraged by the code style. I think you will call this in very few places anyway, so passing it explicitly is fine.
================
Comment at: lib/lsan/lsan_allocator.h:24
@@ +23,3 @@
+
+struct ChunkMetadata {
+ uptr requested_size;
----------------
In sanitizer_common, asan and tsan we use "block" term for this. Please use "block" instead of "chunk" here and elsewhere.
================
Comment at: lib/lsan/lsan_allocator.cc:17
@@ +16,3 @@
+
+#include <pthread.h>
+
----------------
Do you need this?
================
Comment at: lib/lsan/lsan_allocator.cc:46
@@ +45,3 @@
+ Init();
+ inited = true;
+ allocator.Init();
----------------
move this one line up
================
Comment at: lib/lsan/lsan_allocator.cc:58
@@ +57,3 @@
+
+void RegisterAllocation(StackTrace const &stack, void *p, uptr size) {
+ if (!p) return;
----------------
s/void/static void/
================
Comment at: lib/lsan/lsan_allocator.cc:81
@@ +80,3 @@
+ // ensures that leak checking is not taking place now.
+ return NULL;
+}
----------------
s/NULL/0/
================
Comment at: lib/lsan/lsan_allocator.cc:84
@@ +83,3 @@
+
+class MaybeScopedLock {
+ BlockingMutex *m_;
----------------
move this into sanitizer_common/sanitizer_mutex.h
================
Comment at: lib/lsan/lsan_allocator.cc:149
@@ +148,3 @@
+
+LsanMetadata::LsanMetadata(void *chunk) {
+ metadata_ = Metadata(chunk);
----------------
Where is the definition of this class?
Do you actually need it? Looks somewhat useless.
Please do not prefix things with Lsan in Lsan. This is a deadend.
================
Comment at: lib/lsan/lsan_allocator.cc:176
@@ +175,3 @@
+void ForEachChunk(Callable const &callback) {
+ allocator.ForEachChunk(callback);
+}
----------------
This also needs to be renamed to ForRachBlock(), because other functions in allocator uses "block", e.g. GetBlockBegin().
================
Comment at: lib/lsan/lsan_interceptors.cc:16
@@ +15,3 @@
+#include <stddef.h>
+#include <stdlib.h> // setenv
+#include <string.h> // memset
----------------
Is lint happy with this? it should say "2 spaces between code and comment".
Such comments are usually useless and outdated.
================
Comment at: lib/lsan/lsan_interceptors.cc:60
@@ +59,3 @@
+ size *= nmemb;
+ void *p = Allocate(stack, size, 8, false);
+ memset(p, 0, size);
----------------
s/false/true/
================
Comment at: lib/lsan/lsan_interceptors.cc:47
@@ +46,3 @@
+ GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind);
+ void *p = Allocate(stack, size, 8);
+ return p;
----------------
const int kAlignment = 8;
================
Comment at: lib/lsan/lsan_interceptors.cc:59
@@ +58,3 @@
+ GET_STACK_TRACE(flags()->stack_trace_size, flags()->fast_unwind);
+ size *= nmemb;
+ void *p = Allocate(stack, size, 8, false);
----------------
check for integer overflow
================
Comment at: lib/lsan/lsan_interceptors.cc:61
@@ +60,3 @@
+ void *p = Allocate(stack, size, 8, false);
+ memset(p, 0, size);
+ return p;
----------------
Don't we want to zeroize memory in *all* allocation functions to not treat old garbage as potential pointers?
================
Comment at: lib/lsan/lsan_interceptors.cc:83
@@ +82,3 @@
+ *memptr = Allocate(stack, size, alignment);
+ return 0;
+}
----------------
ENOMEM if the pointer is 0
================
Comment at: lib/lsan/lsan_interceptors.cc:96
@@ +95,3 @@
+INTERCEPTOR(void, malloc_usable_size, void) {
+}
+
----------------
Is it not implemented yet or intentionally empty?
If the former, then add a comment saying "not implemented".
================
Comment at: lib/lsan/lsan_interceptors.cc:99
@@ +98,3 @@
+INTERCEPTOR(void, mallinfo, void) {
+}
+
----------------
ditto
================
Comment at: lib/lsan/lsan_interceptors.cc:102
@@ +101,3 @@
+INTERCEPTOR(void, mallopt, void) {
+}
+
----------------
ditto
================
Comment at: lib/lsan/lsan_stack.cc:45
@@ +44,3 @@
+
+void PrintStackTrace(const uptr *trace, uptr size, bool symbolize) {
+ static char current_path[256];
----------------
Why does not it declared in lsan_stack.h?
================
Comment at: lib/lsan/lsan_thread.cc:31
@@ +30,3 @@
+
+ThreadRegistry *thread_registry;
+BlockingMutex thread_create_mutex(LINKER_INITIALIZED);
----------------
static
================
Comment at: lib/lsan/lsan_thread.cc:32
@@ +31,3 @@
+ThreadRegistry *thread_registry;
+BlockingMutex thread_create_mutex(LINKER_INITIALIZED);
+THREADLOCAL u32 current_thread_tid = kInvalidTid;
----------------
static
================
Comment at: lib/lsan/lsan_thread.cc:33
@@ +32,3 @@
+BlockingMutex thread_create_mutex(LINKER_INITIALIZED);
+THREADLOCAL u32 current_thread_tid = kInvalidTid;
+
----------------
static
================
Comment at: lib/lsan/lsan_thread.cc:60
@@ +59,3 @@
+ : ThreadContextBase(tid),
+ stack_begin_(0), stack_end_(0),
+ cache_begin_(0), cache_end_(0),
----------------
one statement on line plz
================
Comment at: lib/lsan/lsan_thread.cc:86
@@ +85,3 @@
+ return thread_registry->CreateThread(user_id, detached,
+ parent_tid, NULL);
+}
----------------
Alexander Potapenko wrote:
> Please fix the indentation.
s/NULL/0/
================
Comment at: lib/lsan/lsan_thread.cc:92
@@ +91,3 @@
+#ifdef __x86_64__
+#define TCB_SIZE 2304
+#endif
----------------
use const int
================
Comment at: lib/lsan/lsan_thread.cc:125
@@ +124,3 @@
+ uptr tls_size = 0;
+ GetThreadStackAndTls(tid == 0, &stack_addr, &stack_size,
+ &tls_addr, &tls_size);
----------------
use &args.stack_begin here and remove the standalone vars
================
Comment at: lib/lsan/lsan_thread.cc:140
@@ +139,3 @@
+ThreadContext *CurrentThreadContext() {
+ if (!thread_registry) return NULL;
+ if (GetCurrentThread() == (u32)-1)
----------------
s/NULL/0/
================
Comment at: lib/lsan/lsan_thread.cc:141
@@ +140,3 @@
+ if (!thread_registry) return NULL;
+ if (GetCurrentThread() == (u32)-1)
+ return NULL;
----------------
s/(u32)-1/kInvalidTid/
================
Comment at: lib/lsan/lsan_thread.cc:149
@@ +148,3 @@
+bool FindThreadByOsIdCallback(ThreadContextBase *tctx, void *arg) {
+ return (tctx->os_id == (uptr)arg && tctx->status != ThreadStatusInvalid);
+}
----------------
os_id is not valid for ThreadStatusDead threads as well
I think you need to fix ThreadRegistry to zeroize os_id before when changing state to ThreadStatusDead.
================
Comment at: lib/lsan/lsan_thread.cc:211
@@ +210,3 @@
+ thread_registry->RunCallbackForEachThreadLocked(LockAllocatorsCallback,
+ NULL);
+}
----------------
Alexander Potapenko wrote:
> Please indent NULL properly and add a comment describing it. ("/*arg*/" ?)
s/NULL/0/
================
Comment at: lib/lsan/lsan_thread.cc:216
@@ +215,3 @@
+ thread_registry->RunCallbackForEachThreadLocked(UnlockAllocatorsCallback,
+ NULL);
+ thread_registry->Unlock();
----------------
s/NULL/0/
================
Comment at: lib/lsan/lsan_allocator.cc:78
@@ +77,3 @@
+ return current_context->allocator_mutex();
+ // If the thread has not been registered yet, it's safe to access the cache
+ // without acquiring a mutex, because our global lock on pthread_create
----------------
Who does call malloc() before the thread is registered? pthread_setspecific?
Don't we have the same problem during thread destruction when we do not hold any global mutexes?
================
Comment at: lib/lsan/lsan_thread.cc:210
@@ +209,3 @@
+ thread_registry->Lock();
+ thread_registry->RunCallbackForEachThreadLocked(LockAllocatorsCallback,
+ NULL);
----------------
Why do you need to lock all these mutexes?
http://llvm-reviews.chandlerc.com/D702
More information about the llvm-commits
mailing list