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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 6 inline comments as done.
cryptoad added a comment.

Thanks Matt! Textual answers only for now, code changes will follow.



================
Comment at: lib/scudo/standalone/common.h:89
+
+template <typename T> INLINE void shuffle(T *A, u32 N, u32 *RandState) {
+  if (N <= 1)
----------------
morehouse wrote:
> What is this used for?
shuffle will be used for randomizing arrays in the Primary & Quarantine.
Both haven't been checked in yet but will be in a few check-ins (once all the "common" files make it in).
A similar function can be found in sanitizer_common, use will be very similar if not identical.


================
Comment at: lib/scudo/standalone/common.h:122
+    return PageSizeCached;
+  return getPageSizeSlow();
+}
----------------
morehouse wrote:
> 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;
> ```
AFAIR those constructs end up in an initialization function being created and run with the global ctors which I am generally trying to avoid.
I will give it a try.


================
Comment at: lib/scudo/standalone/common.h:171
+
+extern "C" typedef void (*IterateCb)(uintptr_t Base, size_t Size, void *Arg);
+
----------------
morehouse wrote:
> What's this here for?
It's a common function used by the Primary & Secondary (not checked in yet).
It probably could find a home somewhere else, I am going to remove it for now.


================
Comment at: lib/scudo/standalone/linux.cc:49
+  if (Flags & MAP_NOACCESS)
+    MmapFlags |= MAP_NORESERVE;
+  if (Addr) {
----------------
morehouse wrote:
> 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.
I will revisit that.


================
Comment at: lib/scudo/standalone/linux.cc:53
+    DCHECK_EQ(Flags & MAP_NOACCESS, 0);
+    MmapFlags |= MAP_FIXED;
+  }
----------------
morehouse wrote:
> Does Scudo actually do `MAP_FIXED`?  If so, why?
We MAP_FIXED inside memory that was reserved with PROT_NONE.
The two usage scenario are:
- mapping on demand in the Primary, basically extending regions;
- mapping secondary allocations with guard pages on either side;


================
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)
----------------
morehouse wrote:
> Considering we just did a system call, this path optimization doesn't provide any substantial benefit.
I will change that.


================
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();
----------------
morehouse wrote:
> Another unnecessary path optimization.
I will change that.


================
Comment at: lib/scudo/standalone/linux.cc:77
+                      UNUSED u64 *Extra) {
+  madvise(reinterpret_cast<void *>(BaseAddress + Offset), Size, MADV_DONTNEED);
+}
----------------
morehouse wrote:
> Should we check errno for `EAGAIN` here?
Good point, I didn't think of it. It is indeed probably useful to retry in that scenario rather than wait for another opportunity.


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


================
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));
+}
----------------
morehouse wrote:
> Should this be cached, as it is in sanitizer_common?
In the current state of things it's only called once for the initialization of the Shared TSD registry.
I figured caching was not needed (I don't think sanitizer_common use it more than once either).


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


================
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!!
+
----------------
morehouse wrote:
> Why is this 8 when sanitizer_common uses 6?
It changed somewhat recently (https://android-review.googlesource.com/c/platform/bionic/+/847554).
Right now I am testing on platforms that still have 8, but it will have to be change to 6 for Q.


================
Comment at: lib/scudo/standalone/mutex.h:39
+  void NOINLINE lockSlow() {
+    for (u8 I = 0;; I++) {
+      if (I < 10)
----------------
morehouse wrote:
> 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?
I will change that.


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


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


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184