[PATCH] D58184: [scudo][standalone] Introduce platform specific code & mutexes

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: lib/scudo/standalone/common.h:89
+
+template <typename T> INLINE void shuffle(T *A, u32 N, u32 *RandState) {
+  if (N <= 1)
----------------
What is this used for?


================
Comment at: lib/scudo/standalone/common.h:122
+    return PageSizeCached;
+  return getPageSizeSlow();
+}
----------------
If we will always get the page size through this function call (and not the global variable), this could be simplified to:

```
static PageSizeCached = getPageSizeSlow();
return PageSizeCached;
```


================
Comment at: lib/scudo/standalone/common.h:171
+
+extern "C" typedef void (*IterateCb)(uintptr_t Base, size_t Size, void *Arg);
+
----------------
What's this here for?


================
Comment at: lib/scudo/standalone/linux.cc:49
+  if (Flags & MAP_NOACCESS)
+    MmapFlags |= MAP_NORESERVE;
+  if (Addr) {
----------------
Since we will be mapping `PROT_NONE` in the scenario, I believe `MAP_NORESERVE` is unnecessary.  I.e., the OS won't reserve swap space anyway since the memory cannot be accessed.


================
Comment at: lib/scudo/standalone/linux.cc:53
+    DCHECK_EQ(Flags & MAP_NOACCESS, 0);
+    MmapFlags |= MAP_FIXED;
+  }
----------------
Does Scudo actually do `MAP_FIXED`?  If so, why?


================
Comment at: lib/scudo/standalone/linux.cc:58
+  void *P = mmap(Addr, Size, MmapProt, MmapFlags, -1, 0);
+  if (UNLIKELY(P == MAP_FAILED)) {
+    if (!(Flags & MAP_ALLOWNOMEM) || errno != ENOMEM)
----------------
Considering we just did a system call, this path optimization doesn't provide any substantial benefit.


================
Comment at: lib/scudo/standalone/linux.cc:71
+void unmap(void *Addr, uptr Size, UNUSED uptr Flags, UNUSED u64 *Extra) {
+  if (UNLIKELY(munmap(Addr, Size) != 0))
+    dieOnMapUnmapError();
----------------
Another unnecessary path optimization.


================
Comment at: lib/scudo/standalone/linux.cc:77
+                      UNUSED u64 *Extra) {
+  madvise(reinterpret_cast<void *>(BaseAddress + Offset), Size, MADV_DONTNEED);
+}
----------------
Should we check errno for `EAGAIN` here?


================
Comment at: lib/scudo/standalone/linux.cc:84
+void BlockingMutex::wait() {
+  syscall(SYS_futex, (uptr)OpaqueStorage, FUTEX_WAIT_PRIVATE, MtxSleeping,
+          nullptr, nullptr, 0);
----------------
`reinterpret_cast`?  Here and below


================
Comment at: lib/scudo/standalone/linux.cc:103
+  CHECK_EQ(sched_getaffinity(0, sizeof(cpu_set_t), &CPUs), 0);
+  return static_cast<u32>(CPU_COUNT(&CPUs));
+}
----------------
Should this be cached, as it is in sanitizer_common?


================
Comment at: lib/scudo/standalone/linux.cc:118
+      syscall(SYS_getrandom, Buffer, Length, Blocking ? 0 : GRND_NONBLOCK);
+  if (ReadBytes == (ssize_t)Length)
+    return true;
----------------
`static_cast`?  Same for below.


================
Comment at: lib/scudo/standalone/linux.h:55
+// store sanitizer thread specific data.
+static const int TLS_SLOT_SANITIZER = 8; // TODO(kostyak): 6 for Q!!
+
----------------
Why is this 8 when sanitizer_common uses 6?


================
Comment at: lib/scudo/standalone/mutex.h:39
+  void NOINLINE lockSlow() {
+    for (u8 I = 0;; I++) {
+      if (I < 10)
----------------
With a `u8`, we might wraparound and do 10 more "quick yields" on a long lock.  Should we bump this up to `u32` to reduce chance of this happening?


================
Comment at: lib/scudo/standalone/mutex.h:57
+  SpinMutex(const SpinMutex &);
+  void operator=(const SpinMutex &);
+};
----------------
Are these constructors ever used?  If not, maybe just use `SpinMutex(const SpinMutex &) = delete`.


================
Comment at: lib/scudo/standalone/mutex.h:100
+  GenericScopedLock(const GenericScopedLock &);
+  void operator=(const GenericScopedLock &);
+};
----------------
Maybe make these deleted too.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184