[scudo] releaseToOSMaybe can fail if it can't allocate PageMap

[Chia-hungDuan]

Summary

PageMap is allocated with MAP_ALLOWNOMEM if there's no static buffer
left. So it can be failed and return nullptr without any assertion
triggered. Instead of crashing in the releaseToOSMaybe in the middle,
just return and let the program handles the page failure.

[Chia-hungDuan]

Remove UNLIKELY attribute which was added by accident

[cferris]

Just a few questions.

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

877

Is there a reason you don't make the ensurePageMapAllocated call before mark functions the same as in primary64.h?

It's not immediately obvious why that's not possible.

If you can pull it in front of these two calls, I would recommend removing the calls to ensurePageMapAllocated in both of the markXXX functions, replaced with a DCHECK(PageMap.isAllocated) call and leave the functions returning void.

[Chia-hungDuan]

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

877

In Primary64, we have determined which memory groups will release so that we can prepare the page map in advance.

In Primary32, we don't exam all the memory groups first to determine the groups to release, so we still have the chance of not releasing any group (i.e., don't need the page map).

The reason we haven't implemented this in primary32 is the logic is a little bit more complicated than primary64 because of multiple regions. Therefore, we didn't implement this in the same CL when we brought partial release in primary64

And yes, I agree that if both allocators apply the same strategy, we can remove theuse of ensurePageMapAllocated().

[cferris]

LGTM

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

for (BatchGroup &BG : Sci->FreeList) {
 
         // Note that we don't always visit blocks in each BatchGroup so that we
         // may miss the chance of releasing certain pages that cross
         // BatchGroups.
         Context.markFreeBlocksInRegion(BG.Batches, DecompactPtr, GroupBase,
                                        RegionIndex, AllocatedGroupSize,
                                        /*MayContainLastBlockInRegion=*/true);
       }
+
+      // We may not be able to do the page release In a rare case that we may
+      // fail on PageMap allocation.
+      if (UNLIKELY(!Context.hasBlockMarked()))
+        return 0;
     }
 
     if (!Context.hasBlockMarked())
       return 0;
 
     auto SkipRegion = [this, First, ClassId](uptr RegionIndex) {
       ScopedLock L(ByteMapMutex);
       return (PossibleRegions[First + RegionIndex] - 1U) != ClassId;

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

const uptr ReleaseEnd =
         Region->RegionBeg;
     const uptr ReleaseRangeSize = ReleaseEnd - ReleaseBase;
     const uptr ReleaseOffset = ReleaseBase - Region->RegionBeg;
 
     RegionReleaseRecorder<MemMapT> Recorder(&Region->MemMap, Region->RegionBeg,
                                             ReleaseOffset);
     PageReleaseContext Context(BlockSize, /*NumberOfRegions=*/1U,
                                ReleaseRangeSize, ReleaseOffset);
+    // We may not be able to do the page release in a rare case that we may
+    // fail on PageMap allocation.
+    if (UNLIKELY(!Context.ensurePageMapAllocated()))
+      return 0;
 
     for (BatchGroup &BG : GroupToRelease) {
       const uptr BatchGroupBase =
           decompactGroupBase(CompactPtrBase, BG.CompactPtrGroupBase);
       const uptr BatchGroupEnd = BatchGroupBase + GroupSize;
       const uptr AllocatedGroupSize = AllocatedUserEnd >= BatchGroupEnd
                                           ? GroupSize
                                           : AllocatedUserEnd - BatchGroupBase;

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

void reset(uptr NumberOfRegion, uptr CountersPerRegion, uptr MaxValue) {
     PackingRatioLog = getLog2(PackingRatio);
     BitOffsetMask = PackingRatio - 1;
 
     SizePerRegion =
         roundUp(NumCounters, static_cast<uptr>(1U) << PackingRatioLog) >>
         PackingRatioLog;
     BufferSize = SizePerRegion * sizeof(*Buffer) * Regions;
     Buffer = Buffers.getBuffer(BufferSize);
-    DCHECK_NE(Buffer, nullptr);
   }
 
   bool isAllocated() const { return !!Buffer; }
 
   uptr getCount() const { return NumCounters; }
 
   uptr get(uptr Region, uptr I) const {
     DCHECK_LT(Region, Regions);
PageReleaseContext(uptr BlockSize, uptr NumberOfRegions, uptr ReleaseSize,
     ReleasePageOffset = ReleaseOffset >> PageSizeLog;
   }
 
   // PageMap is lazily allocated when markFreeBlocks() is invoked.
   bool hasBlockMarked() const {
     return PageMap.isAllocated();
   }
 
-  void ensurePageMapAllocated() {
+  bool ensurePageMapAllocated() {
     if (PageMap.isAllocated())
-      return;
+      return true;
     PageMap.reset(NumberOfRegions, PagesCount, FullPagesBlockCountMax);
-    DCHECK(PageMap.isAllocated());
+    // TODO: Log some message when we fail on PageMap allocation.
+    return PageMap.isAllocated();
   }
 
   // Mark all the blocks in the given range [From, to). Instead of visiting all
   // the blocks, we will just mark the page as all counted. Note the `From` and
   // `To` has to be page aligned but with one exception, if `To` is equal to the
   // RegionSize, it's not necessary to be aligned with page size.
-  void markRangeAsAllCounted(uptr From, uptr To, uptr Base,
+  bool markRangeAsAllCounted(uptr From, uptr To, uptr Base,
                              const uptr RegionIndex, const uptr RegionSize) {
     DCHECK_LT(From, To);
     DCHECK_LE(To, Base + RegionSize);
     DCHECK_EQ(From % PageSize, 0U);
     DCHECK_LE(To - From, RegionSize);
 
-    ensurePageMapAllocated();
+    if (!ensurePageMapAllocated())
+      return false;
 
     uptr FromInRegion = From - Base;
     uptr ToInRegion = To - Base;
     uptr FirstBlockInRange = roundUpSlow(FromInRegion, BlockSize);
 
     // The straddling block sits across entire range.
     if (FirstBlockInRange >= ToInRegion)
-      return;
+      return true;
 
     // First block may not sit at the first pape in the range, move
     // `FromInRegion` to the first block page.
     FromInRegion = roundDown(FirstBlockInRange, PageSize);
 
     // When The first block is not aligned to the range boundary, which means
     // there is a block sitting acorss `From`, that looks like,
     //
bool markRangeAsAllCounted(uptr From, uptr To, uptr Base,
     }
 
     // After handling the first page and the last block, it's safe to mark any
     // page in between the range [From, To).
     if (FromInRegion < ToInRegion) {
       PageMap.setAsAllCountedRange(RegionIndex, getPageIndex(FromInRegion),
                                    getPageIndex(ToInRegion - 1));
     }
+
+    return true;
   }
 
   template <class TransferBatchT, typename DecompactPtrT>
-  void markFreeBlocksInRegion(const IntrusiveList<TransferBatchT> &FreeList,
+  bool markFreeBlocksInRegion(const IntrusiveList<TransferBatchT> &FreeList,
                               DecompactPtrT DecompactPtr, const uptr Base,
                               const uptr RegionIndex, const uptr RegionSize,
                               bool MayContainLastBlockInRegion) {
-    ensurePageMapAllocated();
+    if (!ensurePageMapAllocated())
+      return false;
 
     if (MayContainLastBlockInRegion) {
       const uptr LastBlockInRegion =
           ((RegionSize / BlockSize) - 1U) * BlockSize;
       // The last block in a region may not use the entire page, we mark the
       // following "pretend" memory block(s) as free in advance.
       //
       //     Region Boundary
if (BlockSize <= PageSize && PageSize % BlockSize == 0) {
       for (const auto &It : FreeList) {
         for (u16 I = 0; I < It.getCount(); I++) {
           const uptr PInRegion = DecompactPtr(It.get(I)) - Base;
           PageMap.incRange(RegionIndex, getPageIndex(PInRegion),
                            getPageIndex(PInRegion + BlockSize - 1));
         }
       }
     }
+
+    return true;
   }
 
   uptr getPageIndex(uptr P) { return (P >> PageSizeLog) - ReleasePageOffset; }
 
   uptr BlockSize;
   uptr NumberOfRegions;
   // For partial region marking, some pages in front are not needed to be
   // counted.

compiler-rt/lib/scudo/standalone/tests/release_test.cpp

 #include <algorithm>
 #include <random>
 #include <set>
 
 TEST(ScudoReleaseTest, RegionPageMap) {
   for (scudo::uptr I = 0; I < SCUDO_WORDSIZE; I++) {
     // Various valid counter's max values packed into one word.
     scudo::RegionPageMap PageMap2N(1U, 1U, 1UL << I);
+    ASSERT_TRUE(PageMap2N.isAllocated());
     EXPECT_EQ(sizeof(scudo::uptr), PageMap2N.getBufferSize());
     // Check the "all bit set" values too.
     scudo::RegionPageMap PageMap2N1_1(1U, 1U, ~0UL >> I);
+    ASSERT_TRUE(PageMap2N1_1.isAllocated());
     EXPECT_EQ(sizeof(scudo::uptr), PageMap2N1_1.getBufferSize());
     // Verify the packing ratio, the counter is Expected to be packed into the
     // closest power of 2 bits.
     scudo::RegionPageMap PageMap(1U, SCUDO_WORDSIZE, 1UL << I);
+    ASSERT_TRUE(PageMap.isAllocated());
     EXPECT_EQ(sizeof(scudo::uptr) * scudo::roundUpPowerOfTwo(I + 1),
               PageMap.getBufferSize());
   }
 
   // Go through 1, 2, 4, 8, .. {32,64} bits per counter.
   for (scudo::uptr I = 0; (SCUDO_WORDSIZE >> I) != 0; I++) {
     // Make sure counters request one memory page for the buffer.
     const scudo::uptr NumCounters =
         (scudo::getPageSizeCached() / 8) * (SCUDO_WORDSIZE >> I);
     scudo::RegionPageMap PageMap(1U, NumCounters,
                                        1UL << ((1UL << I) - 1));
+    ASSERT_TRUE(PageMap.isAllocated());
     PageMap.inc(0U, 0U);
     for (scudo::uptr C = 1; C < NumCounters - 1; C++) {
       EXPECT_EQ(0UL, PageMap.get(0U, C));
       PageMap.inc(0U, C);
       EXPECT_EQ(1UL, PageMap.get(0U, C - 1));
     }
     EXPECT_EQ(0UL, PageMap.get(0U, NumCounters - 1));
     PageMap.inc(0U, NumCounters - 1);