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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked an inline comment as done.
cryptoad added subscribers: dvyukov, kcc.
cryptoad added a comment.

Currently OOO so addressing mostly the easy to deal with comments.



================
Comment at: lib/scudo/standalone/internal_defs.h:126
+
+#define COMPILER_CHECK(Pred) static_assert(Pred, "")
+
----------------
vitalybuka wrote:
> Why not to use static_assert directly
> Same for INLINE
Personal preference here: I like the consistency.
For inline, it's so that the function attributes all are cased identically.
For the static assert, it's more because of habit with the current sanitizer code (since it used a macro). The macro isn't great with Werror due to unused typedefs, but static_assert is supported widely enough.
I am not opposed to changing to changing static_assert everywhere, but I would like to keep `INLINE` for consistency.
Let me know.


================
Comment at: lib/scudo/standalone/linux.cc:41
+
+uptr getPageSize() { return static_cast<uptr>(sysconf(_SC_PAGESIZE)); }
+
----------------
vitalybuka wrote:
> sysconf can be intercepted, e.g. in Google internal code
Will that code not work if the code is intercepted? Current sanitizer code returns `EXEC_PAGESIZE` for Linux, should I just use that? If so it feel like it could go in the headers instead and that would optimize a bunch of code.


================
Comment at: lib/scudo/standalone/linux.cc:106
+
+// Blocking is possibly unused if the getrandom block is not compiled in.
+bool getRandom(void *Buffer, uptr Length, UNUSED bool Blocking) {
----------------
vitalybuka wrote:
> I assume this comment is for "#if defined"
> could you move it there?
Well, my comment was more about the `UNUSED` attribute for `Blocking` in the function prototype, hence the position here.
Technically it only matters for the `#if defined` block indeed. Let me know what you think makes more sense.


================
Comment at: lib/scudo/standalone/linux.cc:118
+      syscall(SYS_getrandom, Buffer, Length, Blocking ? 0 : GRND_NONBLOCK);
+  if (ReadBytes == static_cast<ssize_t>(Length))
+    return true;
----------------
vitalybuka wrote:
> do you need static_cast here?
Yes, the `ssize_t` is signed and the compiler complains about signed vs unsigned comparison otherwise.


================
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!!
+
----------------
vitalybuka wrote:
> please run clang-format
It is currently clang-format'd but with `-style=llvm` which doesn't enforce the 2 space prior to comment if that's the thing you are referring to.


================
Comment at: lib/scudo/standalone/mutex.h:45
+      if (atomic_load_relaxed(&State) == 0 &&
+          atomic_exchange(&State, 1, memory_order_acquire) == 0)
+        return;
----------------
vitalybuka wrote:
> this if does not look atomic
> atomic_compare_exchange? 
Good point. So this is current how it's done in sanitizer_common and I didn't put additional thought into it.
Maybe the atomicity was too costly given that the state would eventually settle to something consistent?
cc: @kcc  @dvyukov 


================
Comment at: lib/scudo/standalone/tests/map_test.cc:64
+
+// Death tests
+
----------------
morehouse wrote:
> Maybe we should combine these with the above tests since they will only cost ~1 line each then. 
I'll do that. I initially went for full separation of Death Tests but it's not worth it.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184