[compiler-rt] [NFC] Make RingBuffer an atomic pointer (PR #82547)

via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 23 10:12:07 PST 2024


================
@@ -1300,32 +1311,36 @@ class Allocator {
 
   void storeSecondaryAllocationStackMaybe(const Options &Options, void *Ptr,
                                           uptr Size) {
-    auto *Depot = getDepotIfEnabled(Options);
-    if (!Depot)
+    if (!UNLIKELY(Options.get(OptionBit::TrackAllocationStacks)))
+      return;
+    AllocationRingBuffer *RB = getRingBuffer();
+    if (!RB)
       return;
-    u32 Trace = collectStackTrace(Depot);
+    u32 Trace = collectStackTrace(RB->Depot);
     u32 Tid = getThreadID();
 
     auto *Ptr32 = reinterpret_cast<u32 *>(Ptr);
     Ptr32[MemTagAllocationTraceIndex] = Trace;
     Ptr32[MemTagAllocationTidIndex] = Tid;
 
-    storeRingBufferEntry(untagPointer(Ptr), Trace, Tid, Size, 0, 0);
+    storeRingBufferEntry(RB, untagPointer(Ptr), Trace, Tid, Size, 0, 0);
   }
 
   void storeDeallocationStackMaybe(const Options &Options, void *Ptr,
                                    u8 PrevTag, uptr Size) {
-    auto *Depot = getDepotIfEnabled(Options);
-    if (!Depot)
+    if (!UNLIKELY(Options.get(OptionBit::TrackAllocationStacks)))
+      return;
+    AllocationRingBuffer *RB = getRingBuffer();
+    if (!RB)
       return;
----------------
ChiaHungDuan wrote:

Just want to add some thought about inlining this.

I was asking for putting them to a function (the getDepotIfEnabled) to simplify the code. The reason I suggest that we inline it again is the names (getRingBuffer/getRingBufferIfEnabled) can be kind of confusing (like if it's disabled, why do we still allocate the ring buffer? until we know it can be disabled temporarily)

I'm still thinking that we can reduce the check of `OptionBit::TrackAllocationStacks` by something like,

```
enum AllocationStackType {
  Primary,
  Secondary,
  Deallocation,
};
storeAllocationStack(AllocationStackType Type, ...) {
  if (!UNLIKELY(Options.get(OptionBit::TrackAllocationStacks)))
      return;
  /* call the functions respectively */
}
```
And/or we can simplify some logic as well. For example, `storePrimaryAllocationStackMaybe`/`storeSecondaryAllocationStackMaybe` seem to have some same logic.

We don't need to do the change in this one but I would like to have further refactoring in the future (I can do it if you don't have bandwidth for this) 

https://github.com/llvm/llvm-project/pull/82547


More information about the llvm-commits mailing list