[PATCH] D27656: [tsan] Implement __tsan_get_alloc_stack to query the allocation stack of a heap pointer

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubabrecka added inline comments.


================
Comment at: lib/tsan/rtl/tsan_debugging.cc:168
+int __tsan_get_alloc_stack(uptr addr, uptr *trace, uptr size, int *thread_id,
+                           uptr *os_id) {
+  MBlock *b = 0;
----------------
dvyukov wrote:
> Since the interface is different from asan, it may makes sense to return block begin and size as well. What do you think?
Makes sense.  What about going further and adding a TSan version of `__asan_locate_address` (see asan_debugging.cc), which will "describe" a pointer and return its bounds if known, i.e. having two APIs:

    __tsan_locate_address(addr) ... returns region name, start address, block size
    __tsan_get_alloc_stack(addr) ... returns trace, thread ID

Does that sound better or worse than having just one

    __tsan_get_alloc_stack(addr) ... returns trace, thread ID, start address and block size

?


================
Comment at: lib/tsan/rtl/tsan_debugging.cc:181
+  ThreadContextBase *tctx = ctx->thread_registry->GetThreadLocked(b->tid);
+  *os_id = tctx->os_id;
+
----------------
dvyukov wrote:
> os_id can be stale if status is not ThreadStatusRunning
> I think it's better to return something more predictable in such case, e.g. 0.
Hm.  At least on Darwin, GetTid() returns a unique ID for the current boot of the OS.  So it's correct even after the thread finished, and even after the process has ended (as long as you don't reboot).  Is gettid() on Linux different?

I'm already using os_id to "match" threads in TSan reports:  If we see the same os_id, we know it's the same thread even when that thread doesn't exist anymore.  I'd like not to lose this feature.

Does Linux reuse the thread ID soon enough for this to be a problem?  If yes, how about we reset the os_id in the ThreadRegistry when a thread ends (for Linux, and keep the value for Darwin)?


Repository:
  rL LLVM

https://reviews.llvm.org/D27656