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

[Chandler Carruth]

================
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(
----------------
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.

================
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 {
----------------
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)

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:71
@@ +70,3 @@
+#include "llvm/Support/raw_ostream.h"
+#include "llvm/DataLayout.h"
+#include "llvm/Transforms/Instrumentation.h"
----------------
This should be sorted with the other top-level headers...

================
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.
----------------
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.


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