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

[Filipe Cabecinhas via llvm-commits]

filcab accepted this revision.
filcab added a reviewer: filcab.
filcab added a comment.

Not much to do now except for some final problems. I'll LGTM since I might not be able to reply today and I only need minor fixes.


================
Comment at: lib/esan/esan_interface_internal.h:25
@@ +24,3 @@
+#ifdef __cplusplus
+extern "C" {
+#endif
----------------
Sorry, I have the annotation in the wrong line. I'm not complaining about the `extern "C"`.
The `#ifdef __cplusplus` is always true, since this is the internal (for the library only) definition of the API, which we know will always be compiled as C++. (Check ASan's `asan_interface_internal.h`. TSan has the ifdefs because of one unit test. Otherwise, it wouldn't need the ifdef either)

================
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,
----------------
My problem was with having the type defined twice, but it seems I messed up when looking for ESAN_CacheFrag.
The constants are only defined once inside compiler-rt, so it's good. Sorry about that.

================
Comment at: lib/esan/esan_interface_internal.h:30
@@ +29,3 @@
+typedef enum Type {
+  ESAN_None = 0,
+  ESAN_CacheFrag,
----------------
No. It was more about fixing the underlying type:
http://en.cppreference.com/w/cpp/language/enum
Version (2) of "Unscoped enumeration".

================
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.
 //===----------------------------------------------------------------------===//
----------------
The problem is that later, when blaming that file, people will see this commit, and possibly spend time looking at it because of an unrelated change.
I'm totally ok with adding the comment, I think it's useful. But I don't think it belongs to the "Introduce ESan" commit.

================
Comment at: test/esan/lit.cfg:29
@@ +28,3 @@
+config.suffixes = ['.c', '.cpp']
+
+# EfficiencySanitizer tests are currently supported on Linux x86-64 only.
----------------
Since these are all new files, probably it would be best to use `.cpp` to adhere more to llvm current practice.

================
Comment at: test/esan/lit.cfg:32
@@ +31,2 @@
+if config.host_os not in ['Linux'] or config.target_arch != 'x86_64':
+  config.unsupported = True
----------------
Yeah, unfortunately this is what we have now.
No problems here, as there's no good solution you can start using. I ended up ranting a bit. Sorry about the noise.



http://reviews.llvm.org/D19168