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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:60
+  if (!o->Enabled)
+    return;
+
----------------
hctim wrote:
> 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?
In that case, why support modifying options at runtime at all?

`getOptions` should return a const-ref, not a pointer and all initialization should be done through `initOptions`.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:81
+
+  if (o->SampleRate >= INT32_MAX) {
+    __sanitizer::Printf("GWP-ASan ERROR: SampleRate must be <= 2^31 - 1 when "
----------------
Hm..  So now 2^31 - 1 is no longer supported?


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:38
+    "deallocation when reporting a memory error. GWP-ASan's signal handler "
+    "will call the signal handler that was installed before its own, and user "
+    "programs that install furthersignal handlers should make sure they do the "
----------------
"will forward the signal to any previously-installed handler, and user"


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:39
+    "will call the signal handler that was installed before its own, and user "
+    "programs that install furthersignal handlers should make sure they do the "
+    "same.Note, if the previously installed SIGSEGV handler is SIG_IGN, we "
----------------
further signal


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:40
+    "programs that install furthersignal handlers should make sure they do the "
+    "same.Note, if the previously installed SIGSEGV handler is SIG_IGN, we "
+    "terminate the process after dumping the error report.")
----------------
sam.  Note


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62698