[llvm-commits] [PATCH] MemorySanitizer instrumentation pass.

[Evgeniy Stepanov]

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1233
@@ +1232,3 @@
+
+struct MemorySanitizerVarArgHelper_X86_64: public MemorySanitizerVarArgHelper {
+  // An unfortunate workaround for asymmetric lowering of va_arg stuff.
----------------
Chandler Carruth wrote:
> I think a helper *class* is overkill. Especially a dynamic class with virtual dispatch.
> 
> Why not just a few helper functions? I was mostly looking for you to add a 'visitVarArgCallSite' or some such above.
> 
> In particular, I think pulling all of this out here makes the above visitor a bit harder to read as it has an odd forwarding layer.
> 
> That said, I do like the factoring you have here. I would just fold all of this into the above visitor class.
That will get messy again if we add support for VarArg on other platforms, like x86_32. With the helper class we would just add a new implementation. With helper method we'd need a bunch of if(s) all around.

If virtual dispatch overhead is what you are worried about, we can avoid that with some template magic, though I doubt it matters.



================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:198-199
@@ +197,4 @@
+
+  // Insert a call to __msan_init/__msan_track_origins into the module's CTORs.
+  IRBuilder<> IRB(*C);
+  appendToGlobalCtors(M, cast<Function>(M.getOrInsertFunction(
----------------
Chandler Carruth wrote:
> Evgeniy Stepanov wrote:
> > Chandler Carruth wrote:
> > > If you bild this 3 statemetns earlier you can use its integer type building facilities.
> > I'm not sure what you mean.
> I'm suggesting using IRB.getIntNTy(PtrSize) and IRB.getInt32Ty() above.
Ah, yes. Done. I've also added getIntPtrTy to IRB, please take a look at that and I'll land it ahead of this patch.

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:208-214
@@ +207,9 @@
+  // Create the callback.
+  // FIXME: this function should have "Cold" calling conv,
+  // which is not yet implemented. Alternatively, we may use llvm.trap.
+  if (ClUseTrap) {
+    // WarningFn = Intrinsic::getDeclaration(&M, Intrinsic::trap);
+    // We use inline asm because Intrinsic::trap is treated as never return.
+    WarningFn = InlineAsm::get(FunctionType::get(Type::getVoidTy(*C), false),
+                                  StringRef("ud2"), StringRef(""), true);
+  } else {
----------------
Chandler Carruth wrote:
> Evgeniy Stepanov wrote:
> > Chandler Carruth wrote:
> > > I don't understand any of this. Why aren't we using trap? Why aren't we following the pattern of ASan? What is the goal of UseTrap?
> > Unlike ASan, we want MSan reports to be non-fatal. Normal call spills scratch registers in the caller, etc - it's way too slow (AFAIR, it's ~20% slower than ud2 for very cold calls).
> > 
> I would like to understand why cold function calls aren't sunk (including their spills) out of the flow of the hot path. Could this be factored so that there is a switch controlling it, and a specific function for building a "call" to the warning function? I'm not thrilled about names like "WarningFn" referring to things other than a function. I don't expect this inline asm production to be expensive, why not just do this as-needed in a helper method? That might make it easier to gate different behaviors on a flag.
> 
> Eventually, I agree that implementing a dedicated calling convention that, for example, doesn't spill would be really useful to make the IR more clear. Catching and handling ud2 inline asm is really magical.
> 
> Also, is there any discussion of the rationale behind fatal errors in ASan and non-fatal here? (similarly for tsan)
I did not investigate the source of these performance issues. I don't see a reason why these spills should affect hot code path at all, provided that the branch to the error reporting call is annotated with appropriate weights. I guess our codegen is just not smart enough for this?

Switching to noreturn call for now.

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:398
@@ +397,3 @@
+
+  Type *getShadowTy(Type *OrigTy) {
+    if (!OrigTy->isSized()) {
----------------
Chandler Carruth wrote:
> Doxygen comments throughout please.
I've added some doxygen comments. Most of the methods are trivial or at least similar to one another, I don't think we need to comment all of them. Let me know if there is a non-obvious place I've missed.


http://llvm-reviews.chandlerc.com/D56