[PATCH] D60593: [GwpAsan] Introduce GWP-ASan.

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:23
+namespace gwp_asan {
+GuardedPoolAllocator::~GuardedPoolAllocator() {
+  if (GuardedPagePool) {
----------------
This is going to cause racy use-after-dtors on thread exit.

We need a trivial dtor if we want a global `GuardedPoolAllocator`, or we need to avoid destruction.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:178
+
+void GuardedPoolAllocator::applyAllocationRightAlignment(
+    std::size_t *Size, uintptr_t *AllocationPtr) {
----------------
Why do we need so many options?  In general I think we need to enforce some minimal alignment, probably `alignof(std::max_align_t)`.  For smaller allocs than that, we might be able to do smaller alignments.  So maybe keep `POWER_OF_TWO` case and remove all other options.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:244
+    return;
+  case AllocationStrategyEnum::RANDOM:
+    if (Random::getRandomUnsigned64() % 2 == 0) {
----------------
What do we gain from so many "strategies"?  Why not have even-left and odd-right and be done?


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:267
+GuardedPoolAllocator::reportErrorAndDieInternal(uintptr_t AccessPtr,
+                                                GwpAsanError ErrorType) {
+  if (!pointerIsMine(reinterpret_cast<void *>(AccessPtr)))
----------------
This function is way too long.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.h:43
+  // functions before init() has been called.
+  constexpr GuardedPoolAllocator() {}
+  GuardedPoolAllocator(const GuardedPoolAllocator &) = delete;
----------------
We should have a static_assert to ensure a constexpr GuardedPoolAllocator can actually be instantiated.


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.h:19
+// Parse the options from the GWP_ASAN_FLAGS environment variable.
+void initOptions();
+// Returns a pointer to the initialised options. Call initFlags() prior to
----------------
Can we outsource all flag parsing and just have GuardedPoolAllocator configured during init?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593