[PATCH] D62698: [GWP-ASan] Configuration options [3].

[Mitch Phillips via Phabricator via llvm-commits]

hctim marked 9 inline comments as done.
hctim added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:57
+
+  __sanitizer::InitializeCommonFlags();
+
----------------
morehouse wrote:
> What does this do?  Do we need it?
In this patchset, nothing. New patchest it initialises verbosity, which is used to report unrecognised flags.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:60
+  if (!o->Enabled)
+    return;
+
----------------
morehouse wrote:
> Should we return early?  Maybe the user will later set `Enabled = true`.  In that case, we skipped the below validation and `Printf` initialization.
I don't think we should be targeting live-enable/disable of GWP-ASan at this point in time, only at start time, unless I misunderstand the premise of the question.

@vlad.tsyrklevich WDYT?


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:31
+                "selected for GWP-ASan sampling. Default is 5000. Sample rates "
+                "up to 2^31 are supported.")
+
----------------
morehouse wrote:
> I think 2^31 will overflow.  So technically only 2^31 - 1 is supported.
Also added a sanity check in the optional parser.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:42
+    "Note, if the previously installed SIGSEGV handler is SIG_IGN, we "
+    "terminate the process after dumping the error report.")
----------------
morehouse wrote:
> This wording is a bit confusing.  There are a few uses of the phrase "previously installed handler", but they refer to different handlers in each case.
Changed it. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62698