[PATCH] D19167: [esan] EfficiencySanitizer instrumentation pass

[Derek Bruening via llvm-commits]

bruening marked 6 inline comments as done.

================
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(
----------------
filcab wrote:
> Would "loads and stores" be better than a general "memory accesses"?
OK.  The flag was modeled after tsan's flag which has this name.

================
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] =
----------------
filcab wrote:
> Why not `load`/`store`, like the llvm instructions, instead of `read` and `write`?
This was modeled after tsan, which calls them read and write.  It looks like asan uses load and store.  I am fine changing it.

================
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] =
----------------
filcab wrote:
> Why the different `SmallString` size?
That's what tsan has, presumably being paranoid about the extra length of "unaligned".

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:167
@@ +166,3 @@
+bool EfficiencySanitizer::doInitialization(Module &M) {
+  Ctx = &(M.getContext());
+  const DataLayout &DL = M.getDataLayout();
----------------
filcab wrote:
> Any reason for the extra parens?
This is what AddressSanitizer.cpp and MemorySanitizer.cpp have.  Will remove from here.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:306
@@ +305,3 @@
+    I->eraseFromParent();
+    Res = true;
+  } else
----------------
aizatsky wrote:
> filcab wrote:
> > bruening wrote:
> > > filcab wrote:
> > > > The three cases above can be written more succintly (and made easier to follow/spot (lack of) differences).
> > > > Check out `AddressSanitizer::instrumentMemIntrinsic`.
> > > > 
> > > Reviewer ping-pong :)  Please see the original version of this code in the first patchset, which matched what asan had, and the reviewer comment above asking to change it to the current form.
> > I have a very strong preference over the simpler (to read and (even more important: ) to spot differences) code.
> > Mike: Do you have a strong preference over this idiom?
> > `MemTransferIntrin` has been there, with only these two subclasses, since 2009.
> > I think it's better to have the simpler version, and eventually change it if we need.
> > 
> > btw, Derek: You can reduce the noise by using `isa<Mem...>`, and getting the arguments from a `MemIntrinsic`, like ASan does.
> No I do not have any strong preference.
> btw, Derek: You can reduce the noise by using isa<Mem...>, and getting the arguments from a MemIntrinsic, like ASan does.

This code was originally modeled after tsan.  Yes, asan is more succinct than the tsan code, once it does the cast.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:322
@@ +321,3 @@
+    NumAccessesWithBadSize++;
+    // Ignore all unusual sizes.
+    return -1;
----------------
filcab wrote:
> Do we want to warn once (per run of whatever tool), though?
OK.  I don't see any DO_ONCE macro or std::call_once in the code base so I suppose a direct static bool is the method.

How do I test for this warning in the .ll file?  Also, it seems to pollute the lit output a bit.


http://reviews.llvm.org/D19167