[PATCH] D19167: [esan] EfficiencySanitizer instrumentation pass

Mike Aizatsky via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 15 16:59:09 PDT 2016


aizatsky added inline comments.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:21
@@ +20,3 @@
+
+#include "llvm/Transforms/Instrumentation.h"
+#include "llvm/ADT/SmallString.h"
----------------
bruening wrote:
> 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.
Those files do not conform to LLVM coding style right now, but we'd like them to. There's no reason why new code shouldn't.

================
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";
----------------
bruening wrote:
> 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.
kEsanModuleCtorName

There has been couple complaints about this kind of naming. AFAIK the consensus is that clang-tidy should be happy.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:64
@@ +63,3 @@
+
+static void OverrideOptionsFromCL(const EfficiencySanitizerOptions &Options) {
+  if (Options.ToolType == EfficiencySanitizerOptions::ESAN_CacheFrag)
----------------
bruening wrote:
> 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?
I'd rather you convert in the different direction then. From flag to options. Globals are worse than locals :)

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:156
@@ +155,3 @@
+  MemmoveFn = checkSanitizerInterfaceFunction(
+      M.getOrInsertFunction("memmove", IRB.getInt8PtrTy(), IRB.getInt8PtrTy(),
+                            IRB.getInt8PtrTy(), IntptrTy, nullptr));
----------------
bruening wrote:
> 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_.
I'm not sure if there was a reason for that. Maybe history. If nothing prevents it, I suggest you use __esan_ prefix everywhere. Would be much easier to spot/understand the assemby dump.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:167
@@ +166,3 @@
+bool EfficiencySanitizer::doInitialization(Module &M) {
+  Cxt = &(M.getContext());
+  const DataLayout &DL = M.getDataLayout();
----------------
bruening wrote:
> aizatsky wrote:
> > Ctx
> You're saying current LLVM code prefers "Ctx" as an abbrevation for Context over "Cxt"?   OK, sure.
I actually thought this is a typo :) Seems to be 20x usages for Ctx than Cxt :)


================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:182
@@ +181,3 @@
+
+bool EfficiencySanitizer::ignoreMemoryAccess(Instruction *I) {
+  if (ClToolCacheFrag) {
----------------
bruening wrote:
> 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.
Yep. A simple TBD will do.

================
Comment at: lib/Transforms/Instrumentation/EfficiencySanitizer.cpp:196
@@ +195,3 @@
+    return false;
+  initializeCallbacks(*F.getParent());
+  SmallVector<Instruction*, 8> LoadsAndStores;
----------------
bruening wrote:
> 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?
No, I don't propose that. I guess if this becomes a problem, you'd be able to change it to ModulePass. Let's ignore this for a while.


http://reviews.llvm.org/D19167





More information about the llvm-commits mailing list