[PATCH] DataFlowSanitizer; LLVM changes.

[Peter Collingbourne]

================
Comment at: include/llvm/Transforms/Instrumentation.h:96
@@ +95,3 @@
+#ifdef __GNUC__
+inline ModulePass *createDataFlowSanitizerPassForJIT() {
+  return createDataFlowSanitizerPass(getDFSanArgTLSPtrForJIT,
----------------
Evgeniy Stepanov wrote:
> This does not seem to be used anywhere. And why is it guarded by __GNUC__?
> This does not seem to be used anywhere.

Right.  The idea is that it can be used by JIT clients.

> And why is it guarded by __GNUC__?

Because the functions getDFSanArgTLSPtrForJIT and getDFSanRetValTLSPtrForJIT depend on the availability of __thread and TLS model attibutes.  Without this guard, the build will fail on (e.g.) Windows.

================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:40
@@ +39,3 @@
+// if the input IR contains a load with alignment 8, this flag will cause
+// the shadow load to have alignment 16.  This flag is disabled by default as
+// we have unfortunately encountered too much code (including Clang itself;
----------------
Evgeniy Stepanov wrote:
> A general description of the tool logic and shadow format would be nice to have somewhere above. Perhaps in the file comment.
OK, I'll add something.

================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:60
@@ +59,3 @@
+static cl::opt<bool> ClArgsABI(
+    "dfsan-args-abi",
+    cl::desc("Use the argument ABI rather than the TLS ABI"),
----------------
Evgeniy Stepanov wrote:
> Does it mean you are passing shadow through argument list? Please comment.
I implemented shadow through arguments (the argument ABI) as an alternative to shadow through TLS (the TLS ABI, like msan).  This is generally only useful when we are instrumenting every function in the process from libc up, but since the greylist controls which functions do not get the argument ABI, it is possible in principle to get this to work by listing every libc function in the greylist, except that of course libc functions won't propagate labels.  (Once libc is instrumented we can trim the greylist further to only include functions which need it, such as functions implemented in asm or functions which are called by the compiler backend).  This is a work in progress at the moment, so by default we use TLS.

================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:190
@@ +189,3 @@
+    ArgTypes.push_back(ShadowTy);
+  if (T->isVarArg())
+    ArgTypes.push_back(ShadowPtrTy);
----------------
Evgeniy Stepanov wrote:
> I wonder if you need to do something special with ByVal arguments, too.
Yes, this is on my todo list.

================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:318
@@ +317,3 @@
+          BlockAddress *BA = dyn_cast<BlockAddress>(ui.getUse().getUser());
+          ++ui;
+          if (BA) {
----------------
Evgeniy Stepanov wrote:
> Why not ++ui in the for() clause?
I wanted to be careful here and make sure that the RAUW wouldn't invalidate ui.

================
Comment at: lib/Transforms/Instrumentation/DataFlowSanitizer.cpp:782
@@ +781,3 @@
+    IRBuilder<> IRB(&I);
+    DFSF.AllocaShadowMap[&I] = IRB.CreateAlloca(DFSF.DFS.ShadowTy);
+  }
----------------
Evgeniy Stepanov wrote:
> Thsi is an optimization to skip calculation of shadow address on every load/store of allocas that are always accessed as a whole and don't escape, right?
> Looks like it applies to MSan as well?
> 
> Btw, does it ever happen in optimized IR?
> 
> Thsi is an optimization to skip calculation of shadow address on every load/store of allocas that are always accessed as a whole and don't escape, right?

Exactly.

> Looks like it applies to MSan as well?

I don't see why not.

> Btw, does it ever happen in optimized IR?

I don't think so.  This was specifically implemented for the sake of -O0.


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