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

[Derek Bruening via llvm-commits]

bruening added inline comments.

================
Comment at: cmake/config-ix.cmake:633
@@ +632,3 @@
+
+if (COMPILER_RT_HAS_SANITIZER_COMMON AND CFI_SUPPORTED_ARCH AND
+    OS_NAME MATCHES "Linux")
----------------
aizatsky wrote:
> is CFI_SUPPORTED_ARCH expected here?
No, that is an error: it should be ESAN_.

================
Comment at: lib/esan/esan.cc:54
@@ +53,3 @@
+ALWAYS_INLINE USED
+void processMemAccess(uptr PC, uptr Addr, int SizeLog, bool IsWrite) {
+  VPrintf(4, "in esan::%s %p: %c %p %d\n", __FUNCTION__, PC,
----------------
vitalybuka wrote:
> This looks inconsistent with functions below.
> why it's SizeLog, not just Size and round up to the power of 2 here?
The log is passed from the compiler partly to simplify the indexing in its own code.  We're not doing anything with the log in the runtime, so it would probably be cleaner to change this to Size.  I'll do that.

================
Comment at: lib/esan/esan.cc:84
@@ +83,3 @@
+  parser.ParseString(GetEnv(kEsanOpsEnv));
+  SetVerbosity(common_flags()->verbosity);
+  if (Verbosity())
----------------
aizatsky wrote:
> replace by a call to InitializeCommonFlags (sanitizer_flags.h)
Agreed.

================
Comment at: lib/esan/esan.h:41
@@ +40,3 @@
+const int kSizeLog1 = 0;
+const int kSizeLog2 = 1;
+const int kSizeLog4 = 2;
----------------
vitalybuka wrote:
> I am new to this code, so is there a convention for this?
> Why not enum or even: constexpr sizeLog(size_t size) {...}
This is based on what tsan does.  However, we are leaning toward just throwing these out here as the log is not used for anything.

================
Comment at: lib/esan/esan_interface.cc:47
@@ +46,3 @@
+  processMemAccess(CALLERPC, (uptr)Addr, kSizeLog8, false);
+  processMemAccess(CALLERPC, (uptr)Addr + 8, kSizeLog8, false);
+}
----------------
aizatsky wrote:
> any special reason why not kSizeLog16?
No, this is an artifact coming from tsan.  For us we are not limited to 8 so this could become a single call (and as mentioned above we can move away from the log here).  I will change it.

================
Comment at: lib/esan/esan_interface.h:37
@@ +36,3 @@
+
+SANITIZER_INTERFACE_ATTRIBUTE void __esan_aligned_read1(void *Addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __esan_aligned_read2(void *Addr);
----------------
aizatsky wrote:
> Is there a reason why you have lots of _N functions instead of
> 
> __esan_aligned_write(void* Addr, size_t sizeLog)?
> 
> Each one of these makes this kind of call anyway.
The initial reason was precedence: that's what asan and tsan do.  A (maybe minor) reason not to change is that if profiling shows this slowpath time causing bottlenecks we can more easily write dedicated handlers for each size.


http://reviews.llvm.org/D19168