[PATCH] D19167: [esan] EfficiencySanitizer instrumentation pass

[Derek Bruening via llvm-commits]

bruening added inline comments.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:21
@@ +20,3 @@
+
+#include "llvm/Transforms/Instrumentation.h"
+#include "llvm/ADT/SmallString.h"
----------------
aizatsky wrote:
> wrong import order. Please run clang-tidy & clang-format on the file.
Will do.  Note that AddressSanitizer.cpp, MemorySanitizer, ThreadSanitizer.cpp, etc. on which this file is based all have this same (incorrect) order.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:48
@@ +47,3 @@
+static cl::opt<bool>  ClInstrumentMemoryAccesses(
+    "esan-instrument-memory-accesses", cl::init(true),
+    cl::desc("Instrument memory accesses"), cl::Hidden);
----------------
aizatsky wrote:
> do you imagine these used for other esan sanitizers as well? Maybe esan-cache-frag-*?
Yes, most of the esan tools instrument all memory references and these options will apply to all of them.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:59
@@ +58,3 @@
+
+static const char *const kEsanModuleCtorName = "esan.module_ctor";
+static const char *const kEsanInitName = "__esan_init";
----------------
aizatsky wrote:
> this don't follow llvm naming convention.
Please clarify: the kEsanModuleCtorName, or the esan.module.ctor?  Both are matching what the other sanitizers use.  Neither matches the official LLVM guide, though lower-case k seems to be used enough it feels like an informal convention, and the runtime interface routines seem to all be C-style to match the extern C interface.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:64
@@ +63,3 @@
+
+static void OverrideOptionsFromCL(const EfficiencySanitizerOptions &Options) {
+  if (Options.ToolType == EfficiencySanitizerOptions::ESAN_CacheFrag)
----------------
aizatsky wrote:
> Why do you need this at all, rather than using Options directly?
There are two methods of invocation: from the clang front-end where -fsanitize=efficiency-foo will become EfficiencySanitizerOptions::ESAN_Foo, or when opt is directly invoked and -esan-foo wlil become ClToolFoo.  One needs to be converted to the other.  If you have a solution that is simpler, please elaborate on what you mean?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:156
@@ +155,3 @@
+  MemmoveFn = checkSanitizerInterfaceFunction(
+      M.getOrInsertFunction("memmove", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(),
+                            IRB.getInt8PtrTy(), IntptrTy, nullptr));
----------------
aizatsky wrote:
> why not __esan_memmove?
That would be more consistent.  This current code is modeled after what ThreadSanitizer.cpp and AddressSanitizer.cpp do: they have __xsan_* for the others but just "mem*" for the intrinsics.  I am happy to add __esan_.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:167
@@ +166,3 @@
+bool EfficiencySanitizer::doInitialization(Module &M) {
+  Cxt = &(M.getContext());
+  const DataLayout &DL = M.getDataLayout();
----------------
aizatsky wrote:
> Ctx
You're saying current LLVM code prefers "Ctx" as an abbrevation for Context over "Cxt"?   OK, sure.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:182
@@ +181,3 @@
+
+bool EfficiencySanitizer::ignoreMemoryAccess(Instruction *I) {
+  if (ClToolCacheFrag) {
----------------
aizatsky wrote:
> this function always returns false.
Yes, but not in a future CL.  We have built some prototypes and some return true for some cases here.  We are trying to submit in small pieces for easier reviews.  I will add a comment explaining that this will not always be a nop.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:196
@@ +195,3 @@
+    return false;
+  initializeCallbacks(*F.getParent());
+  SmallVector<Instruction*, 8> LoadsAndStores;
----------------
aizatsky wrote:
> Is it necessary to call it for each function? There could be quite a lot of them.
Hmm, this is modeled after what AddressSanitizer.cpp and ThreadSanitizer.cpp do.  They are all function passes, so I believe they do need to re-acquire these functions every time and can't store them in a simple manner.  Are you proposing changing them all (esan, asan, tsan, maybe others) to be module passes?  How about tackling that in a separate CL?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:289
@@ +288,3 @@
+    IRB.CreateCall(
+        isa<MemCpyInst>(M) ? MemcpyFn : MemmoveFn,
+        {IRB.CreatePointerCast(M->getArgOperand(0), IRB.getInt8PtrTy()),
----------------
aizatsky wrote:
> maybe have 3 cases here? Will be more future-proof.
Sure.  Again, this was shamelessly modeled after the tsan and asan code.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:294
@@ +293,3 @@
+    I->eraseFromParent();
+  }
+  return false;
----------------
aizatsky wrote:
> llvm_unreachable?
OK.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:295
@@ +294,3 @@
+  }
+  return false;
+}
----------------
aizatsky wrote:
> return true?
Agreed, this is modifying the function.  It seems to be a bug in ThreadSanitizer.cpp as well.

================
Comment at: test/Instrumentation/EfficiencySanitizer/cache_frag_basic.ll:52
@@ +51,2 @@
+; CHECK: define internal void @esan.module_ctor()
+; CHECK: call void @__esan_init(i32 0)
----------------
aizatsky wrote:
> please add unaligned read & odd size tests.
WIll do.


http://reviews.llvm.org/D19167