[PATCH] D94212: scudo: Add support for tracking stack traces of secondary allocations.

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1113
+    // partially written state in getSecondaryErrorInfo().
+    atomic_store_relaxed(&Entry->Ptr, 0);
+    atomic_store_relaxed(&Entry->AllocationTrace, AllocationTrace);
----------------
eugenis wrote:
> Does this actually do anything if all stores are relaxed?
The goal is to guard against the thread executing this access being interrupted by a crash, not against concurrent access. By the time the ring buffer entry is accessed the thread will have been stopped so the effects of any stores will have been committed to memory.

That being said, I suspect that the compiler would be allowed to eliminate this store, so we may need something stronger here.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:1135
+
+    storeRingBufferEntry(untagPointer(Ptr), Trace, Tid, Size, 0, 0);
+  }
----------------
eugenis wrote:
> Why create this ring buffer entry while the allocation is still alive?
> This would add some complexity to error reporting, but we could iterate over the live secondary allocations to find the right one. That should reduce ring buffer traffic by 2x (not counting primary of course).
> 
> 
It would add quite a bit of complexity. malloc_iterate is implemented via pointer chasing, whereas `__scudo_get_error_info` expects a few fixed blocks of data to be copied. Implementing the pointer chasing on the `__scudo_get_error_info` side would probably require callbacks to read memory, or something like that. It doesn't seem worth it to reduce ring buffer traffic for secondary allocations, which are quite uncommon anyway.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94212/new/

https://reviews.llvm.org/D94212