[PATCH] D19168: [esan] EfficiencySanitizer base runtime library

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

Almost LGTM with a few minor comments, now.
Thank you!


================
Comment at: lib/esan/CMakeLists.txt:7
@@ +6,3 @@
+append_rtti_flag(OFF ESAN_CFLAGS)
+set(ESAN_RTL_CFLAGS ${ESAN_CFLAGS})
+
----------------
No need to have this variable if it's the same as `ESAN_CFLAGS`

================
Comment at: lib/esan/esan.cc:31
@@ +30,3 @@
+
+static const char *const EsanOpsEnv = "ESAN_OPTIONS";
+
----------------
`EsanOptsEnv`, to keep consistency with other abbreviations of Options
Since you're changing this line, why not the shorter `static const char EsanOptsEnv[] = "..."`?

================
Comment at: lib/esan/esan_interface_internal.h:24
@@ +23,3 @@
+
+#ifdef __cplusplus
+extern "C" {
----------------
This is an internal definition of the interface, it should always be compiled as C++.

================
Comment at: lib/esan/esan_interface_internal.h:28
@@ +27,3 @@
+
+// This should be kept consistent with LLVM's EfficiencySanitizerOptions.
+typedef enum Type {
----------------
Pop this off to something like `lib/esan/esan_internal_defs.h`.

================
Comment at: lib/esan/esan_interface_internal.h:29
@@ +28,3 @@
+// This should be kept consistent with LLVM's EfficiencySanitizerOptions.
+typedef enum Type {
+  ESAN_None = 0,
----------------
In the instrumentation pass, you get the function with an i32 argument.
Should we fix the size of this enum to be 32-bits (I don't know of any practical problems in a common arch with this as it is off the top of my head, though)?

================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:35
@@ -33,1 +34,3 @@
+// COMMON_INTERCEPTOR_ON_EXIT is only called if the app explicitly calls exit(),
+// not on a normal exit.
 //===----------------------------------------------------------------------===//
----------------
Please split this comment addition to a separate commit.


================
Comment at: test/esan/lit.cfg:28
@@ +27,3 @@
+# Default test suffixes.
+config.suffixes = ['.c', '.cc', '.cpp']
+
----------------
Since this is new and adhering to LLVM conventions, we can probably remove the `.cc`, right?

================
Comment at: test/esan/lit.cfg:31
@@ +30,3 @@
+# EfficiencySanitizer tests are currently supported on Linux x86-64 only.
+if config.host_os not in ['Linux']:
+  config.unsupported = True
----------------
I really don't like testing host_os. ESan availability will depend on the target, not the host.
Right now there's no better option in lit, so let's do it this way.
But I'll ask you to try and keep test command lines to constructs that lit's shell knows how to parse, if possible/easy.

================
Comment at: test/esan/lit.cfg:34
@@ +33,3 @@
+
+if config.target_arch != 'x86_64':
+  config.unsupported = True
----------------
You can merge this with the above:
    if config.host_os not in ['Linux'] or config.target_arch != 'x86_64':
      config.unsupported = True


http://reviews.llvm.org/D19168