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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: lib/scudo/standalone/linux.h:20
+typedef struct PlatformData {
+  u8 Unused;
+} PlatformData;
----------------
cryptoad wrote:
> 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.
This is C rule.
it's allowed in C++ 


================
Comment at: lib/scudo/standalone/secondary.h:186
+
+  void disable() { Mutex.lock(); }
+
----------------
cryptoad wrote:
> 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.
Confusing thing is that this is going not just disable, but block all users.



================
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)
----------------
cryptoad wrote:
> 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.
Do Android blocks users as well?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D60787