[PATCH] D57412: [scudo] Initial standalone skeleton check-in

[Matt Morehouse via Phabricator via llvm-commits]

morehouse accepted this revision.
morehouse added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lib/scudo/standalone/atomic_helpers.h:63
+template <typename T>
+INLINE typename T::Type atomic_load(const volatile T *A, memory_order MO) {
+  DCHECK(!(reinterpret_cast<uptr>(A) % sizeof(*A)));
----------------
cryptoad wrote:
> morehouse wrote:
> > Have we  lost the `memory_order` checking that sanitizer_atomic_clang_x86.h does, or is this done in `__atomic_load`?
> I looked around (non-exhaustively) and found that it was checked by `isValidOrderingForOp` in Clang, so I figured it could go away.
> Just checked gcc and it has checks in https://github.com/gcc-mirror/gcc/blob/master/gcc/builtins.c#L6434.
> Let me know if you feel it's reasonable to not have them.
In sanitizer_common, these are DCHECKs so not critical.  And since we're just wrapping the builtins, I think we can delegate the checking to them.


================
Comment at: lib/scudo/standalone/atomic_helpers.h:98
+  __atomic_exchange(&A->ValDoNotUse, &V, &R, MO);
+  return R;
+}
----------------
cryptoad wrote:
> morehouse wrote:
> > Can we simplify to `return __atomic_exchange(&A->ValDoNotUse, &V, MO)` instead?
> As far as I can tell `__atomic_exchange` has a `void` return type.
> It's worth noting that `__atomic_exchange_n` returns `type` but it doesn't seem to be widely used (see libcxx's `__c11_atomic_exchange` for example).
Hm, right I see that now in the [gcc documentation](https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html).

I was looking at [this list](https://libcxx.llvm.org/atomic_design_a.html), which might not be the right reference.


================
Comment at: lib/scudo/standalone/tests/CMakeLists.txt:36
+        SOURCES ${TEST_SOURCES} ${COMPILER_RT_GTEST_SOURCE} 
+        # RUNTIME ${SCUDO_TEST_RUNTIME}
+        COMPILE_DEPS ${TEST_HEADERS}
----------------
cryptoad wrote:
> morehouse wrote:
> > Will this be uncommented eventually?
> It might, or not!
> I'm not awesome at this CMake/lit thing and I feel it could come back and bit me if I remove it and forgot it was useful at some point.
> But I'm Ok removing it.
My guess is that you'll need it once you add some `.cpp` files.  I'm fine either leaving the commented line or not.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D57412