[PATCH] D19167: [esan] EfficiencySanitizer instrumentation pass

[Mike Aizatsky via llvm-commits]

aizatsky added inline comments.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:21
@@ +20,3 @@
+
+#include "llvm/Transforms/Instrumentation.h"
+#include "llvm/ADT/SmallString.h"
----------------
wrong import order. Please run clang-tidy & clang-format on the file.

================
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);
----------------
do you imagine these used for other esan sanitizers as well? Maybe esan-cache-frag-*?

================
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";
----------------
this don't follow llvm naming convention.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:64
@@ +63,3 @@
+
+static void OverrideOptionsFromCL(const EfficiencySanitizerOptions &Options) {
+  if (Options.ToolType == EfficiencySanitizerOptions::ESAN_CacheFrag)
----------------
Why do you need this at all, rather than using Options directly?

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:156
@@ +155,3 @@
+  MemmoveFn = checkSanitizerInterfaceFunction(
+      M.getOrInsertFunction("memmove", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(),
+                            IRB.getInt8PtrTy(), IntptrTy, nullptr));
----------------
why not __esan_memmove?

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

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:182
@@ +181,3 @@
+
+bool EfficiencySanitizer::ignoreMemoryAccess(Instruction *I) {
+  if (ClToolCacheFrag) {
----------------
this function always returns false.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:196
@@ +195,3 @@
+    return false;
+  initializeCallbacks(*F.getParent());
+  SmallVector<Instruction*, 8> LoadsAndStores;
----------------
Is it necessary to call it for each function? There could be quite a lot of them.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:289
@@ +288,3 @@
+    IRB.CreateCall(
+        isa<MemCpyInst>(M) ? MemcpyFn : MemmoveFn,
+        {IRB.CreatePointerCast(M->getArgOperand(0), IRB.getInt8PtrTy()),
----------------
maybe have 3 cases here? Will be more future-proof.

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

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:295
@@ +294,3 @@
+  }
+  return false;
+}
----------------
return true?

================
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)
----------------
please add unaligned read & odd size tests.


http://reviews.llvm.org/D19167