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

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad marked 5 inline comments as done.
cryptoad added inline comments.


================
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)));
----------------
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.


================
Comment at: lib/scudo/standalone/atomic_helpers.h:98
+  __atomic_exchange(&A->ValDoNotUse, &V, &R, MO);
+  return R;
+}
----------------
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).


================
Comment at: lib/scudo/standalone/list.h:148
+
+  // private, do not use directly.
+  uptr Size;
----------------
morehouse wrote:
> Any reason not to put a `private:` here?
Good question, I think this was the case in the sanitizer_common code and I didn't question it.
I will check if that can actually be private.


================
Comment at: lib/scudo/standalone/tests/CMakeLists.txt:36
+        SOURCES ${TEST_SOURCES} ${COMPILER_RT_GTEST_SOURCE} 
+        # RUNTIME ${SCUDO_TEST_RUNTIME}
+        COMPILE_DEPS ${TEST_HEADERS}
----------------
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.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D57412