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

[Matt Morehouse via Phabricator via llvm-commits]

morehouse added inline comments.


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:26
+  va_end(args);
+  return 0;
+}
----------------
May as well make this return `void` if we're always returning 0.


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:29
+
+static Options GWPAsanFlags; // Use via getOptions().
+
----------------
vlad.tsyrklevich wrote:
> Nit: GwpAsan or GWPASan are internally coherent capitalization-wise.
Can we make `GWPAsanFlags` a static inside `getOptions`?


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:47
+static const char *getGWPAsanDefaultOptions() {
+  return (&__gwp_asan_default_options) ? __gwp_asan_default_options() : "";
+}
----------------
I'm pretty sure `&` is unnecessary.


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:48
+  return (&__gwp_asan_default_options) ? __gwp_asan_default_options() : "";
+}
+
----------------
Unnamed namespace would allow us to remove all these `statics` on variables and functions.


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:58
+    OverrideCommonFlags(cf);
+  }
+  Options *o = getOptions();
----------------
Do we care about any of the `sanitizer_common` flags?


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.cpp:99
+
+#if !SANITIZER_SUPPORTS_WEAK_HOOKS
+SANITIZER_INTERFACE_WEAK_DEF(const char *, __gwp_asan_default_options, void) {
----------------
Why the `#if`?


================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.h:1
+//===-- runtime_env_flag_parser.h -------------------------------*- C++ -*-===//
+//
----------------
Why is this file called `runtime_env_flag_parser.h`?



  # We're also parsing compile-time flags.
  # Looks like we're calling these "options" rather than flags.






================
Comment at: compiler-rt/lib/gwp_asan/optional/runtime_env_flag_parser.h:20
+void initOptions();
+// Returns a pointer to the initialised options. Call initFlags() prior to
+// calling this function.
----------------
s/initFlags/initOptions


================
Comment at: compiler-rt/lib/gwp_asan/options.h:27
+#include "gwp_asan/options.inc"
+#undef GWP_ASAN_OPTION
+
----------------
Seems we have potential double-initialization of the global options:

# We call `initOptions` before the `GwpAsanFlags` constructor runs.
# `GwpAsanFlags` constructor runs and sets back to defaults.


================
Comment at: compiler-rt/lib/gwp_asan/options.inc:36
+                "better error reports by providing stack traces for "
+                "allocation/deallocation when reporting a use-after-free.")
----------------
This option should describe interactions with the user's signal handlers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60593