[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:39
+  return (__gwp_asan_default_options) ? __gwp_asan_default_options() : "";
+}
+
----------------
If the above functions are private, they can go in an unnamed namespace or be made static.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:57
+
+  __sanitizer::InitializeCommonFlags();
+
----------------
What does this do?  Do we need it?


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:60
+  if (!o->Enabled)
+    return;
+
----------------
Should we return early?  Maybe the user will later set `Enabled = true`.  In that case, we skipped the below validation and `Printf` initialization.


================
Comment at: compiler-rt/lib/gwp_asan/optional/options_parser.cpp:89
+  return "";
+}
----------------
I think this is unnecessary since we check `__gwp_asan_default_options != nullptr` before calling it.


================
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.")
+
----------------
I think 2^31 will overflow.  So technically only 2^31 - 1 is supported.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62698