[PATCH] D61385: [scudo][standalone] Introduce the Quarantine

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 4 inline comments as done.
cryptoad added inline comments.


================
Comment at: lib/scudo/standalone/quarantine.h:138
+    uptr TotalQuarantineChunks = 0;
+    for (auto It = List.begin(); It != List.end(); ++It) {
+      BatchCount++;
----------------
morehouse wrote:
> Does range-based loop work here?
If you could please clarify, I am unsure as to which range you are referring to.


================
Comment at: lib/scudo/standalone/quarantine.h:181
+    // is zero (it allows us to perform just one atomic read per put() call).
+    CHECK((Size == 0 && CacheSize == 0) || CacheSize != 0);
+
----------------
morehouse wrote:
> What range of `MaxCacheSize` will Scudo allow?  I notice you removed handling for `MaxCacheSize == 0` in `put` below.
This is due to the fact that the Combined will skip a `put` and directly `deallocate` a chunk if:
- the global quarantine cache size is 0 
- or the size of the chunk is greater than `quarantine_max_chunk_size`
- or the size of a  chunk is 0 (as requested by user)

This is the current behavior of Scudo with `quarantineOrDeallocateChunk`, except that since we were using the sanitizer_common Quarantine, the `put` were doing an extraneous comparison.

As for range of values, they will likely be identical to the ones currently implemented in the non-standalone version.


================
Comment at: lib/scudo/standalone/quarantine.h:188
+    CacheMutex.init();
+    RecyleMutex.init();
+  }
----------------
morehouse wrote:
> morehouse wrote:
> > I think these can be `initLinkerInitialized`.
> Right now it's a no-op, but I think we should call `Cache.initLinkerInitialized` here too in case a future change makes it not a no-op.
Thanks, it turned out `StaticMutex` didn't have an `initLinkerInitialized`, so I added one.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D61385