[PATCH] D63231: [scudo][standalone] Introduce the combined allocator

[Kostya Kortchinsky via Phabricator via llvm-commits]

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


================
Comment at: lib/scudo/standalone/allocator.h:1
+//===-- allocator.h ---------------------------------------------*- C++ -*-===//
+//
----------------
morehouse wrote:
> Why call this file allocator.h?  allocator_config.h seems more accurate.
Renamed to allocator_config.h and changed header and `#if` guards accordingly. Updated the CMake file.


================
Comment at: lib/scudo/standalone/combined.h:114
+
+    if (UNLIKELY(!getRandom(reinterpret_cast<void *>(&Cookie), sizeof(Cookie))))
+      Cookie = static_cast<u32>(getMonotonicTime() ^
----------------
morehouse wrote:
> Is this reinterpret_cast required?
So indeed, I tried on a few platforms and it appears not to be.
At this point I am a bit wary about extra warning inducing compiler flags, and I am actually not certain if one would trip on that.
I am open to removing it if that feels safe "enough". WDYT?


================
Comment at: lib/scudo/standalone/tests/combined_test.cc:73
+  bool Found = false;
+  for (scudo::uptr I = 0; I < 1024U && !Found; I++) {
+    void *P = Allocator->allocate(NeedleSize, Origin);
----------------
morehouse wrote:
> Is this test flaky at all?
Currently no, the number of iterations & size chosen are large enough with a small quarantine to ensure that recycling will happen and that we will end up reusing the chunk. This is mostly experimentally determined. This could change in the future if more randomization is introduced or concepts of "never released blocks" or things like that are introduced.


================
Comment at: lib/scudo/standalone/tests/combined_test.cc:133
+  UseQuarantine = false;
+  testAllocator<scudo::AndroidSvelteConfig>();
+}
----------------
morehouse wrote:
> What about FuchsiaConfig?
Added FuchsiaConfig for 64-bit tests. Replaced Config with DefaultConfig to cover them all.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D63231