[PATCH] D35694: [scudo] Quarantine overhaul

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_allocator.cpp:164
 struct AllocatorOptions {
-  u32 QuarantineSizeMb;
+  u32 QuarantineSizeKb;
   u32 ThreadLocalQuarantineSizeKb;
----------------
alekseyshl wrote:
> Please remind me, what was the reason of switching to Kb? Do you need to go lower than 1Mb in real life or you need more granularity?
Correct, going under 1MB is the objective. The added granularity is a bonus.


================
Comment at: lib/scudo/scudo_allocator.cpp:473
+      // GetActuallyAllocatedSize.
+      uptr LiableSize = Size + (Header->Offset << MinAlignmentLog);
       UnpackedHeader NewHeader = *Header;
----------------
alekseyshl wrote:
> EstimatedSize?
Ok, I will change that.


================
Comment at: lib/scudo/scudo_flags.cpp:113
+  if (f->QuarantineChunksUpToSize > (4 * 1024 * 1024)) {
+    dieWithMessage("ERROR: the chunk quarantine threshold is too large\n");
   }
----------------
alekseyshl wrote:
> Why do we care to check this?
I think it's good practice to avoid too large of settings, keep people in check.


================
Comment at: test/scudo/quarantine.cpp:33
+}
+
 int main(int argc, char **argv)
----------------
alekseyshl wrote:
> Those functions make the code more concise, but more fragile and less readable. Do you really need them? Inline asserts were just fine.
Ok, I will change that.


================
Comment at: test/scudo/quarantine.cpp:78
     }
     assert(found == true);
   }
----------------
alekseyshl wrote:
>  assert(found);
Ok.


https://reviews.llvm.org/D35694