[PATCH] D60787: [scudo][standalone] Introduce the Secondary allocator

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/fuchsia.h:25
+  uint64_t VmoSize;
+} PlatformData;
+
----------------
Can we simplify to `struct PlatformData { ... }`?


================
Comment at: lib/scudo/standalone/linux.h:21
+  u8 Unused;
+} PlatformData;
+
----------------
Can we simplify to `struct PlatformData { ... }`?


================
Comment at: lib/scudo/standalone/secondary.h:64
+  // alignment, so that the frontend can align the user allocation. The hint
+  // parameter allows us to be able unmap spurious memory when dealing with
+  // larger (greater than a page) alignments.
----------------
"allows us to unmap" seems more readable.


================
Comment at: lib/scudo/standalone/secondary.h:76
+                                   MAP_NOACCESS | MAP_ALLOWNOMEM, &Data));
+    if (UNLIKELY(!MapBase))
+      return nullptr;
----------------
We're already on the slow path, and we just did a system call, so this path optimization is negligible.  Same for below.


================
Comment at: lib/scudo/standalone/secondary.h:98
+                             roundUpTo((Size - AlignmentHint), PageSize) +
+                             PageSize;
+      DCHECK_LE(NewMapEnd, MapEnd);
----------------
When is `NewMapEnd < MapEnd`?


================
Comment at: lib/scudo/standalone/secondary.h:104
+        MapEnd = NewMapEnd;
+      }
+    }
----------------
So we potentially shrink the committed region for large alignments but did not allocate extra beforehand.  Does this mean the actual region size may be less than the requested size?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787