[PATCH] D19921: [esan] EfficiencySanitizer shadow memory

[Mike Aizatsky via llvm-commits]

aizatsky added inline comments.

================
Comment at: lib/esan/esan.cpp:66
@@ +65,3 @@
+  if (WhichTool == ESAN_CacheFrag)
+    ShadowMapping.initMapping(2); // 4B:1B, so 4 to 1 == >>2.
+
----------------
Looks like mapping won't be initialized for other tools.

================
Comment at: lib/esan/esan_interceptors.cpp:425
@@ +424,3 @@
+  if (*addr) {
+    if (!isAppMem((uptr)*addr) || !isAppMem((uptr)*addr + sz - 1)) {
+      VPrintf(1, "mmap conflict: %p-%p is not in an app region\n",
----------------
filcab wrote:
> What if `addr+sz` gets to the next region?
theoretically addr & addr + sz - 1 could be in app memory, but in different regions. Do we worry about such scenario? 

================
Comment at: lib/esan/esan_shadow.h:39
@@ +38,3 @@
+//
+// We model our shadow memory after Umbra, a library used by the Dr. Memory
+// tool.  We use Umbra's scheme as it was designed to support different
----------------
Any reference? Paper/source code/github project.

================
Comment at: lib/esan/esan_shadow.h:47
@@ +46,3 @@
+//
+//   shadow(app) = ((app & 0x00000fff'ffffffff) + offs) >> scale
+//
----------------
filcab wrote:
> Don't abbreviate "offset" (same for next line).
Mask choice surprises me. Why 3 f's?

================
Comment at: lib/esan/esan_shadow.h:51
@@ +50,3 @@
+// from a pure "<< scale" strategy:
+//   scale == 3: 0x0000900'000000000
+//   scale == 2: 0x0000440'000000000
----------------
Why can't we use the same shadow offset for all scales? You'll need 2 loads now to calculate shadow address. Why not:

(app & mask) >> scale + offs? 


http://reviews.llvm.org/D19921