[PATCH] D61867: [GWP-ASan] Initial build files, implementation of PRNG [1].

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/random.cpp:28
+                                                 xorShift32(), xorShift32(),
+                                                 xorShift32(), xorShift32()};
+  uint32_t Temp = RandomState[0] ^ (RandomState[0] >> 2);
----------------
morehouse wrote:
> This seems like a lot of state to keep.  Not a big deal, but is there a simpler RNG we can use?  What about just xorshift32?
`xorwow` has a period of `2 ** 192 - 2**32`, versus `xorshift32` having a period of `2 ** 32`. Also, surprisingly, it's about 1.2x faster than `xorshift32` (http://quick-bench.com/mYmSCp5jJ_NLo3yyPXt-2N3TuHg) on my x86_64.

I personally think the 20B extra TLS/thread is worth it (given that it's likely zero overhead anyway unless the 20B pushes someone to alloc a new page for TLS). WDYT?


================
Comment at: compiler-rt/lib/gwp_asan/random.cpp:17
+static thread_local uint64_t RandomStateA =
+    static_cast<uint64_t>(time(nullptr));
+
----------------
morehouse wrote:
> We have double-initialization if `getRandomUnsigned64` is called before these initializers run.
Fixed w/ function-local statics.


================
Comment at: compiler-rt/lib/gwp_asan/random.cpp:29
+
+static thread_local uint64_t RandomStateB = xorShiftStar64();
+
----------------
eugenis wrote:
> I thought we switched to __thread?
> Does the generated code become more compact if these two are combined in a single thread-local aggregate?
> 
As discussed offline, the slow path + guaranteed always-correct PRNG.


================
Comment at: compiler-rt/lib/gwp_asan/random.h:17
+namespace gwp_asan {
+namespace random {
+// TODO(hctim): This may have significant overhead for platforms where
----------------
morehouse wrote:
> A random namespace might be overkill.
Removed.


================
Comment at: compiler-rt/lib/gwp_asan/random.h:20
+// 64-bit arithmetic is emulated. Do we need less than a 2^32 chance of
+// sampling?
+// xorshift128+ (64-bit output). Avoids multiplication.
----------------
morehouse wrote:
> 1 in 2 billion is very rare.  I don't see a use case for less frequent sampling than that.
Changed to 32-bit (using a slightly different algorithm).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61867