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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/linux.h:20
+typedef struct PlatformData {
+  u8 Unused;
+} PlatformData;
----------------
vitalybuka wrote:
> Why this can't be empty?
Mostly because:
(C11, 6.7.2.1 Structure and union specifiers p8) "If the struct-declaration-list does not contain any named members, either directly or via an anonymous structure or anonymous union, the behavior is undefined."
So I figured I'd put a single byte field.


================
Comment at: lib/scudo/standalone/secondary.h:66
+  // larger (greater than a page) alignments.
+  NOINLINE void *allocate(uptr Size, uptr AlignmentHint = 0,
+                          uptr *BlockEnd = nullptr) {
----------------
vitalybuka wrote:
> It's NOINLINE so can go into cpp file?
Good point, I kept a template model in my head but there is no point, I will split it into a CC (I will rename them all to CPP once we land it all).


================
Comment at: lib/scudo/standalone/secondary.h:186
+
+  void disable() { Mutex.lock(); }
+
----------------
vitalybuka wrote:
> Why not just lock, unlock?
This is mostly due to the name of the Android APIs: malloc_enable & malloc_disable.
I carried the name over to the allocator's functions in Combined, Primary & Secondary (the top level disable effectively disables Primary & Secondary).
I can rename them.


================
Comment at: lib/scudo/standalone/secondary.h:190
+
+  template <typename F> void iterateOverBlocks(F Callback) const {
+    for (LargeBlock::Header *H = Tail; H != nullptr; H = H->Prev)
----------------
vitalybuka wrote:
> Wouldn't be better if we lock inside of iterateOverBlocks
See the comment above. Due to how the APIs are used in Android (disable/iterate/enable), I mirrored the behavior here.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787