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

[Derek Bruening via llvm-commits]

bruening marked 5 inline comments as done.

================
Comment at: lib/esan/esan_interface_internal.h:24
@@ +23,3 @@
+
+#ifdef __cplusplus
+extern "C" {
----------------
filcab wrote:
> This is an internal definition of the interface, it should always be compiled as C++.
This is the interface between the compiler instrumentation and the runtime library.  It is C for simplicity.  This matches what all the existing sanitizers do.

================
Comment at: lib/esan/esan_interface_internal.h:28
@@ +27,3 @@
+
+// This should be kept consistent with LLVM's EfficiencySanitizerOptions.
+typedef enum Type {
----------------
filcab wrote:
> Pop this off to something like `lib/esan/esan_internal_defs.h`.
To me that sounds like definitions internal to the runtime library and not shared with anything else, while these are part of the interface with the compiler instrumentation.  (Having "internal" in the name of this file was not my first choice: requested as part of this review.)  The parallel files tsan_interface.h and asan_interface_internal.h also have types in the same file.

================
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,
----------------
filcab wrote:
> 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)?
You mean, make this a type-less enumerator and typedef u32 as ToolType?  That is less appealing.  I will add a static_assert on the sizeof to the .cc file.

================
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.
 //===----------------------------------------------------------------------===//
----------------
filcab wrote:
> Please split this comment addition to a separate commit.
> 
It is a continuation of this header comment about what macros the including file needs to define, making a comment on one of them.  Probably it looks different only seeing this patch window of a few lines.  Please take a look at the entire top of this file.

================
Comment at: test/esan/lit.cfg:28
@@ +27,3 @@
+# Default test suffixes.
+config.suffixes = ['.c', '.cc', '.cpp']
+
----------------
filcab wrote:
> Since this is new and adhering to LLVM conventions, we can probably remove the `.cc`, right?
Hmm, I suppose the runtime library implementation files should be renamed from esan.cc to esan.cpp, etc. as well.  That will churn the review diff but few comments are in the impl files.

================
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
----------------
filcab wrote:
> 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.
Note that every existing sanitizer lit.cfg tests host_os.


http://reviews.llvm.org/D19168