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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added a comment.

LGTM



================
Comment at: lib/scudo/standalone/internal_defs.h:126
+
+#define COMPILER_CHECK(Pred) static_assert(Pred, "")
+
----------------
cryptoad wrote:
> 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.
I would prefer standard keyword when possible.
static_assert was probably not available at that time.
Not sure why they decided to use INLINE.

I don't have strong opinion on that. Feel free to keep either way.


================
Comment at: lib/scudo/standalone/linux.cc:41
+
+uptr getPageSize() { return static_cast<uptr>(sysconf(_SC_PAGESIZE)); }
+
----------------
cryptoad wrote:
> 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.
It depends.
You are likely going to use this in initialization, and interceptor may try to allocate memory using SCUDO. I guess it's fine to keep it as-is and resolve if we hit this problem.



================
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) {
----------------
cryptoad wrote:
> 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.
 either way is OK


================
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!!
+
----------------
cryptoad wrote:
> 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.
Yep, spaces.
Then it's my mistake, please keep is as it's set by clang-format.


================
Comment at: lib/scudo/standalone/mutex.h:45
+      if (atomic_load_relaxed(&State) == 0 &&
+          atomic_exchange(&State, 1, memory_order_acquire) == 0)
+        return;
----------------
cryptoad wrote:
> 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 
If you are going to submit as-is maybe some TODO or other comment here?


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D58184