[PATCH] D66189: [GWP-ASan] Implement stack frame compression.

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/guarded_pool_allocator.cpp:78
+  if (Backtrace) {
+    uintptr_t *UncompressedBuffer =
+        static_cast<uintptr_t *>(alloca(MaxTraceLength * sizeof(uintptr_t)));
----------------
vlad.tsyrklevich wrote:
> Use a VLA instead of allocas, here and below.
Unfortunately VLA with non-constexpr size is C++14 only, and IMHO adding explicit C++14 requirements in the CMake file is more ugly than just using `alloca` here. LMK if you're really keen on a different strategy.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:28
 
+GWP_ASAN_OPTION(
+    int, MaxTraceLength, 128,
----------------
vlad.tsyrklevich wrote:
> Why make this an option? I don't see why users would need to tune it, I'd set it to something reasonable (64?) and just leave it as a constant.
Agreed that runtime users shouldn't generally need to tune this, but this is more for the supporting allocator to tune. I'd expect that bionic on Android has vastly different requirements for scudo on fuchsia for example, and this allows them to tune it in the supporting allocator. Scudo currently exposes this to the user (as they use `optional/options_parser.cpp` from sanitizer-common to enumaret the options), but allocators don't have to expose this option to the user.


================
Comment at: compiler-rt/lib/gwp_asan/stack_trace_compressor.cpp:10
+#include "gwp_asan/stack_trace_compressor.h"
+
+namespace gwp_asan {
----------------
vlad.tsyrklevich wrote:
> Most of these implementations vary from the ones in Chrome, I'd consider just using the same ones (modulo style differences) for consistency.
I've done this and updated the tests. Original code didn't zigzag encode the first pointer, only the subsequent diffs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66189