[scudo] seperate cache retrieval logic

[frs513]

Summary

Split cache::retrieve() into separate functions. One that retrieves
the cached block and another that sets the header and MTE environment.
These were split so that the retrieve function could be more easily
changed in the future and so that the retrieve function had the sole
purpose of retrieving a CachedBlock.

[Chia-hungDuan]

Can you add some comment in the commit message (and update the summary here) that saying why we want to do this split?

compiler-rt/lib/scudo/standalone/secondary.h

445–459

Can we merge this two if (useMemoryTagging<Config>(Options))?

[frs513]

Combined if statements in setHeader() and updated commit message to explain
why we separated Cache::retrieve().

[frs513]

fixed grammatical errors in commit message

[cferris]

compiler-rt/lib/scudo/standalone/secondary.h

442

Should be CachedBlock &Entry to match the retrieve call and avoid copying the data structure. Unless that's what you wanted to do.

[frs513]

compiler-rt/lib/scudo/standalone/secondary.h

442

I just passed in a copy because nothing should be writing to the CachedBlock Entry here, setHeader() is only reading values.

[cferris]

compiler-rt/lib/scudo/standalone/secondary.h

442

But it's a good idea to avoid doing the copy. Since the entry is not used after this point, it would make more sense to avoid doing the copy and passing by reference.

[frs513]

passed in CachedBlock by reference to setHeader() to avoid copying the CachedBlock structure

[cferris]

LGTM. After the builds pass, I can submit this for you.

[cferris]

Can you run clang format on this? By looking deep into the logs, I did see an error about clang format failing.

[frs513]

clang format code

[frs513]

clang format and fixed version of file

[cferris]

LGTM again.

[frs513]

rebased and resolved merge conflicts

[Chia-hungDuan]

In D155660#4532864, @frs513 wrote:

rebased and resolved merge conflicts

It looks like you uploaded wrong CL to this change id?

[frs513]

uploading correct commit after rebase

compiler-rt/lib/scudo/standalone/secondary.h

 static inline void unmap(LargeBlock::Header *H) {
   // Note that the `H->MapMap` is stored on the pages managed by itself. Take
   // over the ownership before unmap() so that any operation along with unmap()
   // won't touch inaccessible pages.
   MemMapT MemMap = H->MemMap;
   MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
 }
 
+namespace {
+struct CachedBlock {
+  uptr CommitBase = 0;
+  uptr CommitSize = 0;
+  uptr BlockBegin = 0;
+  MemMapT MemMap = {};
+  u64 Time = 0;
+};
+} // namespace
+
 template <typename Config> class MapAllocatorNoCache {
 public:
   void init(UNUSED s32 ReleaseToOsInterval) {}
-  bool retrieve(UNUSED Options Options, UNUSED uptr Size, UNUSED uptr Alignment,
-                UNUSED LargeBlock::Header **H, UNUSED bool *Zeroed) {
-    return false;
-  }
+  bool retrieve(UNUSED uptr Size, UNUSED CachedBlock &Entry) { return false; }
   void store(UNUSED Options Options, LargeBlock::Header *H) { unmap(H); }
   bool canCache(UNUSED uptr Size) { return false; }
   void disable() {}
   void enable() {}
   void releaseToOS() {}
   void disableMemoryTagging() {}
   void unmapTestOnly() {}
   bool setOption(Option O, UNUSED sptr Value) {
void store(Options Options, LargeBlock::Header *H) EXCLUDES(Mutex) {
     if (EmptyCache)
       empty();
     else if (Interval >= 0)
       releaseOlderThan(Time - static_cast<u64>(Interval) * 1000000);
     if (!EntryCached)
       Entry.MemMap.unmap(Entry.MemMap.getBase(), Entry.MemMap.getCapacity());
   }
 
-  bool retrieve(Options Options, uptr Size, uptr Alignment,
-                LargeBlock::Header **H, bool *Zeroed) EXCLUDES(Mutex) {
-    const uptr PageSize = getPageSizeCached();
+  bool retrieve(uptr Size, CachedBlock &Entry) EXCLUDES(Mutex) {
     const u32 MaxCount = atomic_load_relaxed(&MaxEntriesCount);
     bool Found = false;
-    CachedBlock Entry;
-    uptr HeaderPos = 0;
     {
       ScopedLock L(Mutex);
       if (EntriesCount == 0)
         return false;
       for (u32 I = 0; I < MaxCount; I++) {
-        const uptr CommitBase = Entries[I].CommitBase;
-        if (!CommitBase)
-          continue;
-        const uptr CommitSize = Entries[I].CommitSize;
-        const uptr AllocPos =
-            roundDown(CommitBase + CommitSize - Size, Alignment);
-        HeaderPos =
-            AllocPos - Chunk::getHeaderSize() - LargeBlock::getHeaderSize();
-        if (HeaderPos > CommitBase + CommitSize)
+        if (!Entries[I].CommitBase)
           continue;
-        if (HeaderPos < CommitBase ||
-            AllocPos > CommitBase + PageSize * MaxUnusedCachePages) {
+        if (Size > Entries[I].CommitSize)
           continue;
-        }
         Found = true;
         Entry = Entries[I];
         Entries[I].CommitBase = 0;
         EntriesCount--;
         break;
       }
     }
-    if (!Found)
-      return false;
-
-    *H = reinterpret_cast<LargeBlock::Header *>(
-        LargeBlock::addHeaderTag<Config>(HeaderPos));
-    *Zeroed = Entry.Time == 0;
-    if (useMemoryTagging<Config>(Options))
-      Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
-    uptr NewBlockBegin = reinterpret_cast<uptr>(*H + 1);
-    if (useMemoryTagging<Config>(Options)) {
-      if (*Zeroed) {
-        storeTags(LargeBlock::addHeaderTag<Config>(Entry.CommitBase),
-                  NewBlockBegin);
-      } else if (Entry.BlockBegin < NewBlockBegin) {
-        storeTags(Entry.BlockBegin, NewBlockBegin);
-      } else {
-        storeTags(untagPointer(NewBlockBegin), untagPointer(Entry.BlockBegin));
-      }
-    }
-    (*H)->CommitBase = Entry.CommitBase;
-    (*H)->CommitSize = Entry.CommitSize;
-    (*H)->MemMap = Entry.MemMap;
-    return true;
+    return Found;
   }
 
   bool canCache(uptr Size) {
     return atomic_load_relaxed(&MaxEntriesCount) != 0U &&
            Size <= atomic_load_relaxed(&MaxEntrySize);
   }
 
   bool setOption(Option O, sptr Value) {
uptr N = 0;
       IsFullEvents = 0;
     }
     for (uptr I = 0; I < N; I++) {
       MemMapT &MemMap = MapInfo[I];
       MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
     }
   }
 
-  struct CachedBlock {
-    uptr CommitBase = 0;
-    uptr CommitSize = 0;
-    uptr BlockBegin = 0;
-    MemMapT MemMap = {};
-    u64 Time = 0;
-  };
-
   void releaseIfOlderThan(CachedBlock &Entry, u64 Time) REQUIRES(Mutex) {
     if (!Entry.CommitBase || !Entry.Time)
       return;
     if (Entry.Time > Time) {
       if (OldestTime == 0 || Entry.Time < OldestTime)
         OldestTime = Entry.Time;
       return;
     }
template <typename F> void iterateOverBlocks(F Callback) const {
     for (const auto &H : InUseBlocks) {
       uptr Ptr = reinterpret_cast<uptr>(&H) + LargeBlock::getHeaderSize();
       if (allocatorSupportsMemoryTagging<Config>())
         Ptr = untagPointer(Ptr);
       Callback(Ptr);
     }
   }
 
+  inline void setHeader(Options Options, CachedBlock &Entry,
+                        LargeBlock::Header *H, bool &Zeroed) {
+    Zeroed = Entry.Time == 0;
+    if (useMemoryTagging<Config>(Options)) {
+      Entry.MemMap.setMemoryPermission(Entry.CommitBase, Entry.CommitSize, 0);
+      // Block begins after the LargeBlock::Header
+      uptr NewBlockBegin = reinterpret_cast<uptr>(H + 1);
+      if (Zeroed) {
+        storeTags(LargeBlock::addHeaderTag<Config>(Entry.CommitBase),
+                  NewBlockBegin);
+      } else if (Entry.BlockBegin < NewBlockBegin) {
+        storeTags(Entry.BlockBegin, NewBlockBegin);
+      } else {
+        storeTags(untagPointer(NewBlockBegin), untagPointer(Entry.BlockBegin));
+      }
+    }
+    H->CommitBase = Entry.CommitBase;
+    H->CommitSize = Entry.CommitSize;
+    H->MemMap = Entry.MemMap;
+  }
+
   bool canCache(uptr Size) { return Cache.canCache(Size); }
 
   bool setOption(Option O, sptr Value) { return Cache.setOption(O, Value); }
 
   void releaseToOS() { Cache.releaseToOS(); }
 
   void disableMemoryTagging() { Cache.disableMemoryTagging(); }
 
uptr RoundedSize =
                   Chunk::getHeaderSize(),
               PageSize);
   if (Alignment > PageSize)
     RoundedSize += Alignment - PageSize;
 
   if (Alignment < PageSize && Cache.canCache(RoundedSize)) {
     LargeBlock::Header *H;
     bool Zeroed;
-    if (Cache.retrieve(Options, Size, Alignment, &H, &Zeroed)) {
+    CachedBlock Entry;
+    if (Cache.retrieve(RoundedSize, Entry)) {
+      const uptr AllocPos =
+          roundDown(Entry.CommitBase + Entry.CommitSize - Size, Alignment);
+      const uptr HeaderPos =
+          AllocPos - LargeBlock::getHeaderSize() - Chunk::getHeaderSize();
+      H = reinterpret_cast<LargeBlock::Header *>(
+          LargeBlock::addHeaderTag<Config>(HeaderPos));
+      setHeader(Options, Entry, H, Zeroed);
       const uptr BlockEnd = H->CommitBase + H->CommitSize;
       if (BlockEndPtr)
         *BlockEndPtr = BlockEnd;
       uptr HInt = reinterpret_cast<uptr>(H);
       if (allocatorSupportsMemoryTagging<Config>())
         HInt = untagPointer(HInt);
       const uptr PtrInt = HInt + LargeBlock::getHeaderSize();
       void *Ptr = reinterpret_cast<void *>(PtrInt);