[scudo][NFC] Add a default unmap() to unmap all pages #102234

ChiaHungDuan · 2024-08-06T21:37:25Z

No description provided.

llvmbot · 2024-08-06T21:37:55Z

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: None (ChiaHungDuan)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/102234.diff

11 Files Affected:

(modified) compiler-rt/lib/scudo/standalone/combined.h (+2-4)
(modified) compiler-rt/lib/scudo/standalone/mem_map_base.h (+4)
(modified) compiler-rt/lib/scudo/standalone/primary64.h (+1-1)
(modified) compiler-rt/lib/scudo/standalone/release.h (+1-1)
(modified) compiler-rt/lib/scudo/standalone/secondary.h (+1-1)
(modified) compiler-rt/lib/scudo/standalone/tests/common_test.cpp (+2-2)
(modified) compiler-rt/lib/scudo/standalone/tests/map_test.cpp (+3-3)
(modified) compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp (+1-1)
(modified) compiler-rt/lib/scudo/standalone/tests/strings_test.cpp (+1-1)
(modified) compiler-rt/lib/scudo/standalone/tests/vector_test.cpp (+1-1)
(modified) compiler-rt/lib/scudo/standalone/vector.h (+1-2)

diff --git a/compiler-rt/lib/scudo/standalone/combined.h b/compiler-rt/lib/scudo/standalone/combined.h
index fcf65652c5fce..9c26282e6f860 100644
--- a/compiler-rt/lib/scudo/standalone/combined.h
+++ b/compiler-rt/lib/scudo/standalone/combined.h
@@ -1706,14 +1706,12 @@ class Allocator {
       return;
     // N.B. because RawStackDepotMap is part of RawRingBufferMap, the order
     // is very important.
-    RB->RawStackDepotMap.unmap(RB->RawStackDepotMap.getBase(),
-                               RB->RawStackDepotMap.getCapacity());
+    RB->RawStackDepotMap.unmap();
     // Note that the `RB->RawRingBufferMap` is stored on the pages managed by
     // itself. Take over the ownership before calling unmap() so that any
     // operation along with unmap() won't touch inaccessible pages.
     MemMapT RawRingBufferMap = RB->RawRingBufferMap;
-    RawRingBufferMap.unmap(RawRingBufferMap.getBase(),
-                           RawRingBufferMap.getCapacity());
+    RawRingBufferMap.unmap();
     atomic_store(&RingBufferAddress, 0, memory_order_release);
   }
 
diff --git a/compiler-rt/lib/scudo/standalone/mem_map_base.h b/compiler-rt/lib/scudo/standalone/mem_map_base.h
index 99ab0cba604fc..cba02a16b02f2 100644
--- a/compiler-rt/lib/scudo/standalone/mem_map_base.h
+++ b/compiler-rt/lib/scudo/standalone/mem_map_base.h
@@ -35,6 +35,10 @@ template <class Derived> class MemMapBase {
     DCHECK((Addr == getBase()) || (Addr + Size == getBase() + getCapacity()));
     invokeImpl(&Derived::unmapImpl, Addr, Size);
   }
+  // A default implementation to unmap all pages.
+  void unmap() {
+    unmap(getBase(), getCapacity());
+  }
 
   // This is used to remap a mapped range (either from map() or dispatched from
   // ReservedMemory). For example, we have reserved several pages and then we
diff --git a/compiler-rt/lib/scudo/standalone/primary64.h b/compiler-rt/lib/scudo/standalone/primary64.h
index 8a583bacb4a93..8436f33c2fdcf 100644
--- a/compiler-rt/lib/scudo/standalone/primary64.h
+++ b/compiler-rt/lib/scudo/standalone/primary64.h
@@ -160,7 +160,7 @@ template <typename Config> class SizeClassAllocator64 {
         ScopedLock ML(Region->MMLock);
         MemMapT MemMap = Region->MemMapInfo.MemMap;
         if (MemMap.isAllocated())
-          MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+          MemMap.unmap();
       }
       *Region = {};
     }
diff --git a/compiler-rt/lib/scudo/standalone/release.h b/compiler-rt/lib/scudo/standalone/release.h
index b6f76a4d20585..69f926e3f8680 100644
--- a/compiler-rt/lib/scudo/standalone/release.h
+++ b/compiler-rt/lib/scudo/standalone/release.h
@@ -158,7 +158,7 @@ class BufferPool {
       DCHECK_EQ((Mask & (static_cast<uptr>(1) << Buf.BufferIndex)), 0U);
       Mask |= static_cast<uptr>(1) << Buf.BufferIndex;
     } else {
-      Buf.MemMap.unmap(Buf.MemMap.getBase(), Buf.MemMap.getCapacity());
+      Buf.MemMap.unmap();
     }
   }
 
diff --git a/compiler-rt/lib/scudo/standalone/secondary.h b/compiler-rt/lib/scudo/standalone/secondary.h
index df7d1e63b9562..88eb5bd1ee566 100644
--- a/compiler-rt/lib/scudo/standalone/secondary.h
+++ b/compiler-rt/lib/scudo/standalone/secondary.h
@@ -66,7 +66,7 @@ template <typename Config> static Header *getHeader(const void *Ptr) {
 } // namespace LargeBlock
 
 static inline void unmap(MemMapT &MemMap) {
-  MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+  MemMap.unmap();
 }
 
 namespace {
diff --git a/compiler-rt/lib/scudo/standalone/tests/common_test.cpp b/compiler-rt/lib/scudo/standalone/tests/common_test.cpp
index fff7c662a41bc..e6ddbb00b843c 100644
--- a/compiler-rt/lib/scudo/standalone/tests/common_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/common_test.cpp
@@ -50,7 +50,7 @@ TEST(ScudoCommonTest, SKIP_ON_FUCHSIA(ResidentMemorySize)) {
   memset(P, 1, Size);
   EXPECT_GT(getResidentMemorySize(), OnStart + Size - Threshold);
 
-  MemMap.unmap(MemMap.getBase(), Size);
+  MemMap.unmap();
 }
 
 TEST(ScudoCommonTest, Zeros) {
@@ -69,7 +69,7 @@ TEST(ScudoCommonTest, Zeros) {
   MemMap.releasePagesToOS(MemMap.getBase(), Size);
   EXPECT_EQ(std::count(P, P + N, 0), N);
 
-  MemMap.unmap(MemMap.getBase(), Size);
+  MemMap.unmap();
 }
 
 } // namespace scudo
diff --git a/compiler-rt/lib/scudo/standalone/tests/map_test.cpp b/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
index 06a56f848030e..cc7d3ee4dc6c2 100644
--- a/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/map_test.cpp
@@ -46,7 +46,7 @@ TEST(ScudoMapDeathTest, MapUnmap) {
           scudo::uptr P = MemMap.getBase();
           if (P == 0U)
             continue;
-          MemMap.unmap(MemMap.getBase(), Size);
+          MemMap.unmap();
           memset(reinterpret_cast<void *>(P), 0xbb, Size);
         }
       },
@@ -68,7 +68,7 @@ TEST(ScudoMapDeathTest, MapWithGuardUnmap) {
   ASSERT_TRUE(MemMap.remap(Q, Size, MappingName));
   memset(reinterpret_cast<void *>(Q), 0xaa, Size);
   EXPECT_DEATH(memset(reinterpret_cast<void *>(Q), 0xaa, Size + 1), "");
-  MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+  MemMap.unmap();
 }
 
 TEST(ScudoMapTest, MapGrowUnmap) {
@@ -87,5 +87,5 @@ TEST(ScudoMapTest, MapGrowUnmap) {
   Q += PageSize;
   ASSERT_TRUE(MemMap.remap(Q, PageSize, MappingName));
   memset(reinterpret_cast<void *>(Q), 0xbb, PageSize);
-  MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+  MemMap.unmap();
 }
diff --git a/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp b/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
index 0613847f1e203..1fae651865359 100644
--- a/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/memtag_test.cpp
@@ -63,7 +63,7 @@ class MemtagTest : public Test {
   void TearDown() override {
     if (Buffer) {
       ASSERT_TRUE(MemMap.isAllocated());
-      MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+      MemMap.unmap();
     }
   }
 
diff --git a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
index 2c0916d789202..f81e5036e83b0 100644
--- a/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/strings_test.cpp
@@ -145,7 +145,7 @@ TEST(ScudoStringsTest, CapacityIncreaseFails) {
   scudo::MemMapT MemMap;
   if (MemMap.map(/*Addr=*/0U, scudo::getPageSizeCached(), "scudo:test",
                  MAP_ALLOWNOMEM)) {
-    MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+    MemMap.unmap();
     setrlimit(RLIMIT_AS, &Limit);
     TEST_SKIP("Limiting address space does not prevent mmap.");
   }
diff --git a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
index a972d24a62688..cec8f46a8af21 100644
--- a/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
+++ b/compiler-rt/lib/scudo/standalone/tests/vector_test.cpp
@@ -62,7 +62,7 @@ TEST(ScudoVectorTest, ReallocateFails) {
   scudo::MemMapT MemMap;
   if (MemMap.map(/*Addr=*/0U, scudo::getPageSizeCached(), "scudo:test",
                  MAP_ALLOWNOMEM)) {
-    MemMap.unmap(MemMap.getBase(), MemMap.getCapacity());
+    MemMap.unmap();
     setrlimit(RLIMIT_AS, &Limit);
     TEST_SKIP("Limiting address space does not prevent mmap.");
   }
diff --git a/compiler-rt/lib/scudo/standalone/vector.h b/compiler-rt/lib/scudo/standalone/vector.h
index 98b3db4ad6980..0d059bab461c4 100644
--- a/compiler-rt/lib/scudo/standalone/vector.h
+++ b/compiler-rt/lib/scudo/standalone/vector.h
@@ -86,8 +86,7 @@ template <typename T, size_t StaticNumEntries> class VectorNoCtor {
   }
   void destroy() {
     if (Data != &LocalData[0])
-      ExternalBuffer.unmap(ExternalBuffer.getBase(),
-                           ExternalBuffer.getCapacity());
+      ExternalBuffer.unmap();
   }
 
 private:

github-actions · 2024-08-06T21:41:05Z

✅ With the latest revision this PR passed the C/C++ code formatter.

cferris1000

Assuming my question about the two test cases that use Size instead of the capacity value are actually an error.

cferris1000 · 2024-08-06T22:00:40Z

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

@@ -50,7 +50,7 @@ TEST(ScudoCommonTest, SKIP_ON_FUCHSIA(ResidentMemorySize)) {
  memset(P, 1, Size);
  EXPECT_GT(getResidentMemorySize(), OnStart + Size - Threshold);

-  MemMap.unmap(MemMap.getBase(), Size);


I presume that the usage of Size here was incorrect and this was supposed to be a clean-up, correct?

Oh yes, the Size is always equal to getCapacity() here.

ChiaHungDuan requested a review from cferris1000 August 6, 2024 21:37

llvmbot added compiler-rt compiler-rt:scudo Scudo Hardened Allocator compiler-rt:sanitizer labels Aug 6, 2024

[scudo][NFC] Add a default unmap() to unmap all pages

02238ea

ChiaHungDuan force-pushed the main branch from f96933a to 02238ea Compare August 6, 2024 21:45

cferris1000 approved these changes Aug 6, 2024

View reviewed changes

ChiaHungDuan mentioned this pull request Aug 6, 2024

[scudo] Added test fixture for cache tests. #102230

Merged

ChiaHungDuan merged commit dd741fc into llvm:main Aug 7, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[scudo][NFC] Add a default unmap() to unmap all pages #102234

[scudo][NFC] Add a default unmap() to unmap all pages #102234

Uh oh!

ChiaHungDuan commented Aug 6, 2024

Uh oh!

llvmbot commented Aug 6, 2024

Uh oh!

github-actions bot commented Aug 6, 2024 •

edited

Loading

Uh oh!

cferris1000 left a comment

Uh oh!

cferris1000 Aug 6, 2024

Uh oh!

ChiaHungDuan Aug 6, 2024

Uh oh!

Uh oh!

Uh oh!

[scudo][NFC] Add a default unmap() to unmap all pages #102234

[scudo][NFC] Add a default unmap() to unmap all pages #102234

Uh oh!

Conversation

ChiaHungDuan commented Aug 6, 2024

Uh oh!

llvmbot commented Aug 6, 2024

Uh oh!

github-actions bot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cferris1000 left a comment

Choose a reason for hiding this comment

Uh oh!

cferris1000 Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

ChiaHungDuan Aug 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Aug 6, 2024 •

edited

Loading