[PATCH] D19167: [esan] EfficiencySanitizer instrumentation pass

[Filipe Cabecinhas via llvm-commits]

filcab added a subscriber: filcab.
filcab added a comment.

Thanks a lot for working on this. I'm interested in it. :-)


================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:51
@@ +50,3 @@
+    "esan-instrument-memory-accesses", cl::init(true),
+    cl::desc("Instrument memory accesses"), cl::Hidden);
+static cl::opt<bool> ClInstrumentMemIntrinsics(
----------------
Would "loads and stores" be better than a general "memory accesses"?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:94
@@ +93,3 @@
+  bool instrumentMemIntrinsic(Instruction *I);
+  bool ignoreMemoryAccess(Instruction *I);
+  int getMemoryAccessFuncIndex(Value *Addr, const DataLayout &DL);
----------------
`shouldIgnoreMemoryAccess`?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:138
@@ +137,3 @@
+    // read + write instrumentation: these callouts are for the slowpath.
+    SmallString<32> AlignedReadName("__esan_aligned_read" + ByteSizeStr);
+    EsanAlignedRead[Idx] =
----------------
Why not `load`/`store`, like the llvm instructions, instead of `read` and `write`?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:146
@@ +145,3 @@
+            AlignedWriteName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
+    SmallString<64> UnalignedReadName("__esan_unaligned_read" + ByteSizeStr);
+    EsanUnalignedRead[Idx] =
----------------
Why the different `SmallString` size?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:167
@@ +166,3 @@
+bool EfficiencySanitizer::doInitialization(Module &M) {
+  Ctx = &(M.getContext());
+  const DataLayout &DL = M.getDataLayout();
----------------
Any reason for the extra parens?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:306
@@ +305,3 @@
+    I->eraseFromParent();
+    Res = true;
+  } else
----------------
The three cases above can be written more succintly (and made easier to follow/spot (lack of) differences).
Check out `AddressSanitizer::instrumentMemIntrinsic`.


================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:322
@@ +321,3 @@
+    NumAccessesWithBadSize++;
+    // Ignore all unusual sizes.
+    return -1;
----------------
Do we want to warn once (per run of whatever tool), though?


http://reviews.llvm.org/D19167