<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 3:41 PM, Peter Collingbourne <span dir="ltr"><<a href="mailto:peter@pcc.me.uk" target="_blank">peter@pcc.me.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class="">On Tue, Oct 07, 2014 at 03:30:49PM -0700, David Blaikie wrote:<br>
> On Tue, Oct 7, 2014 at 2:51 PM, Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>> wrote:<br>
><br>
> > Looks good, thanks!<br>
> ><br>
> > Can you write the test case, please? You probably have more experience<br>
> > writing debug info tests than I do.<br>
> ><br>
><br>
> Sure - though how would I get the pre-dfsan .ll file to produce this<br>
> behavior? I've tried compiling to a .ll file without dfsan, then feeling<br>
> that .ll through opt -dfsan, and I got different output. Instead of<br>
> wrapping main, it just replaced it. Is there some attribute or other thing<br>
> the frontend is adding to ensure main is wrapped, rather than converted?<br>
<br>
</span>The pass takes an ABI list file, which in this case needs to contain the<br>
following two entries in order to tell the pass that main is uninstrumented:<br>
<br>
fun:main=uninstrumented<br>
fun:main=discard<br>
<br>
Please take a look at the test/Instrumentation/DataFlowSanitizer/abilist.ll<br>
test case for an example of how to specify an ABI list (you only need the<br>
-dfsan-abilist flag).<br></blockquote><div><br></div><div>Lovely. Committed in r219249.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Peter<br>
<div class=""><div class="h5"><br>
><br>
><br>
> ><br>
> > On Tue, Oct 07, 2014 at 02:35:49PM -0700, David Blaikie wrote:<br>
> > > Here's a basic patch which would solve it in sort of the same way as the<br>
> > > other optimizations I was fixing (just special case the debug info & fix<br>
> > it<br>
> > > up). I can work up a test case for this as well, or you can, if you<br>
> > > like/this seems reasonable.<br>
> > ><br>
> > > On Tue, Oct 7, 2014 at 2:30 PM, Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>><br>
> > wrote:<br>
> > ><br>
> > > > On Tue, Oct 07, 2014 at 12:20:55PM -0700, David Blaikie wrote:<br>
> > > > > On Tue, Oct 7, 2014 at 12:18 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
> > > > wrote:<br>
> > > > ><br>
> > > > > ><br>
> > > > > ><br>
> > > > > > On Tue, Oct 7, 2014 at 12:10 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a><br>
> > ><br>
> > > > wrote:<br>
> > > > > ><br>
> > > > > >><br>
> > > > > >><br>
> > > > > >> On Tue, Oct 7, 2014 at 11:48 AM, Peter Collingbourne <<br>
> > <a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a><br>
> > > > ><br>
> > > > > >> wrote:<br>
> > > > > >><br>
> > > > > >>> On Tue, Oct 07, 2014 at 10:04:30AM -0700, David Blaikie wrote:<br>
> > > > > >>> > Hi Peter,<br>
> > > > > >>> ><br>
> > > > > >>> > After discovering several bugs in ArgumentPromotion and<br>
> > > > > >>> > DeadArgumentElimination where llvm::Functions were replaced<br>
> > with<br>
> > > > > >>> similar<br>
> > > > > >>> > functions (with the same name) to transform their type in some<br>
> > > > way, I<br>
> > > > > >>> > started looking at all calls to llvm::Function::takeName to<br>
> > see if<br>
> > > > > >>> there<br>
> > > > > >>> > were any other debug info quality bugs in similar callers.<br>
> > > > > >>> ><br>
> > > > > >>> > One such caller is the DataFlowSanitizer, and I don't see any<br>
> > debug<br>
> > > > > >>> info<br>
> > > > > >>> > tests for this so I'm wondering what /should/ happen here.<br>
> > > > > >>> ><br>
> > > > > >>> > Is DFSan+DebugInfo something that matters? I assume so.<br>
> > > > > >>><br>
> > > > > >>> It may be important in the future, but at the moment the dfsan<br>
> > > > runtime<br>
> > > > > >>> library<br>
> > > > > >>> does not make use of debug info. The debug info could still be<br>
> > > > useful for<br>
> > > > > >>> regular debugging tasks though.<br>
> > > > > >>><br>
> > > > > >><br>
> > > > > >> Yeah - that'd be the baseline. I assume some people probably care<br>
> > > > about<br>
> > > > > >> being able to debug a dfsan binary. Not sure if your users have<br>
> > > > > >> specifically highlighted this situation or come across the bugs<br>
> > I'm<br>
> > > > > >> speculating about.<br>
> > > > > >><br>
> > > > > >><br>
> > > > > >>> > It looks like DFSan is creating wrappers (in/around<br>
> > > > > >>> > DataFlowSanitizer.cpp:680-700) - when it does this, should it<br>
> > > > update<br>
> > > > > >>> the<br>
> > > > > >>> > debug info for these functions? Or are these internal<br>
> > > > instrumentation<br>
> > > > > >>> > functions & nothing to do with the code the user wrote? I can't<br>
> > > > quite<br>
> > > > > >>> tell<br>
> > > > > >>> > from the code.<br>
> > > > > >>><br>
> > > > > >>> The functions created by that part of the code replace the<br>
> > original<br>
> > > > > >>> functions,<br>
> > > > > >>> so they should inherit the debug info for those functions.<br>
> > > > > >>><br>
> > > > > >><br>
> > > > > >> Yep, that won't happen for free - we have to stitch it back into<br>
> > the<br>
> > > > > >> debug info.<br>
> > > > > >><br>
> > > > > >><br>
> > > > > >>> But the code below that can also create wrapper functions which<br>
> > do<br>
> > > > not<br>
> > > > > >>> need<br>
> > > > > >>> debug info (lines 712-746). Wrappers normally show up for<br>
> > > > uninstrumented<br>
> > > > > >>> functions (e.g. main and many libc functions).<br>
> > > > > >>><br>
> > > > > >>> > Could you provide any C/C++ source examples whis part of DFSan<br>
> > > > fires<br>
> > > > > >>> > reliably, so I could experiment with some examples and see how<br>
> > the<br>
> > > > > >>> debug<br>
> > > > > >>> > info looks?<br>
> > > > > >>><br>
> > > > > >>> This is an example of a program that exercises the replacement<br>
> > > > function<br>
> > > > > >>> and<br>
> > > > > >>> wrapper features.<br>
> > > > > >>><br>
> > > > > >>><br>
> > > > > >>><br>
> > > ><br>
> > --------------------------------------------------------------------------------<br>
> > > > > >>> #include <stddef.h><br>
> > > > > >>> #include <string.h><br>
> > > > > >>><br>
> > > > > >>> size_t len(size_t (*strlen_ptr)(const char *), const char *str) {<br>
> > > > > >>> return strlen_ptr(str);<br>
> > > > > >>> }<br>
> > > > > >>><br>
> > > > > >>> int main(void) {<br>
> > > > > >>> return len(strlen, "foo");<br>
> > > > > >>> }<br>
> > > > > >>><br>
> > > > > >>><br>
> > > ><br>
> > --------------------------------------------------------------------------------<br>
> > > > > >>><br>
> > > > > >>> In this example, 'len' is rewritten to 'dfs$len', 'main' keeps<br>
> > its<br>
> > > > > >>> original<br>
> > > > > >>> name (the pass treats it as an uninstrumented function), and<br>
> > > > wrappers are<br>
> > > > > >>> created for 'main' and 'strlen' (the wrapper for 'main' is<br>
> > unused as<br>
> > > > the<br>
> > > > > >>> C runtime calls the regular 'main' function directly).<br>
> > > > > >>><br>
> > > > > >><br>
> > > > > >> OK, so when you say wrappers are created - where does the name go?<br>
> > > > "main"<br>
> > > > > >> keeps the name? (it's important which way this transformation is<br>
> > done<br>
> > > > - if<br>
> > > > > >> the guts of main are moved to a new function and the name moved as<br>
> > > > well,<br>
> > > > > >> that's not the same as keeping the same function as far as debug<br>
> > info<br>
> > > > > >> metadata is concerned)<br>
> > > ><br>
> > > > In this case the function keeps its original name and the wrapper is<br>
> > > > created<br>
> > > > separately.<br>
> > > ><br>
> > > > > >>> I compile this with '-O0 -g'. A 'break main'/'run'/'break<br>
> > > > strlen'/'cont'<br>
> > > > > >>> gives a relevant stack trace:<br>
> > > > > >>><br>
> > > > > >>> #0 __strlen_sse2_pminub () at<br>
> > > > > >>> ../sysdeps/x86_64/multiarch/strlen-sse2-pminub.S:33<br>
> > > > > >>> #1 0x00005555555587ff in __dfsw_strlen (s=0x55555556fe17 "foo",<br>
> > > > > >>> s_label=<optimized out>, ret_label=0x7fffffffddee)<br>
> > > > > >>> at llvm/projects/compiler-rt/lib/dfsan/dfsan_custom.cc:203<br>
> > > > > >>> #2 0x000055555556bbdc in dfsw$strlen ()<br>
> > > > > >>> #3 0x000055555556bb51 in len (strlen_ptr=0x55555556bbc0<br>
> > > > <dfsw$strlen>,<br>
> > > > > >>> str=0x55555556fe17 "foo") at strlen.c:5<br>
> > > > > >>> #4 0x000055555556bb96 in main ()<br>
> > > > > >>><br>
> > > > > >>> In this stack trace, #2 is the compiler-generated wrapper<br>
> > function<br>
> > > > for<br>
> > > > > >>> strlen.<br>
> > > > > >>><br>
> > > > > >>> It looks like the debug info for 'len' is preserved correctly,<br>
> > but I<br>
> > > > > >>> don't<br>
> > > > > >>> know why the debug info for 'main' is missing.<br>
> > > > > >>><br>
> > > > > >><br>
> > > > > >> Yeah - not quite sure either. I'll poke around with it for a<br>
> > little<br>
> > > > bit.<br>
> > > > > >><br>
> > > > > ><br>
> > > > > > DataFlowSanitizer.cpp:719: F.replaceAllUsesWith(WrappedFnCst);<br>
> > > > > ><br>
> > > > > > replaces the debug info metadata's pointer to main with a pointer<br>
> > to<br>
> > > > > > dfsw$main (this issue doesn't come up with DAE or ArgPromo because<br>
> > they<br>
> > > > > > both don<br>
> > > > > ><br>
> > > > ><br>
> > > > > ... they both don't use RAUW because they have to visit each call<br>
> > site to<br>
> > > > > do special stuff anyway. (they're replacing one function with<br>
> > another of<br>
> > > > a<br>
> > > > > different type, RAUW isn't suitable)<br>
> > > > ><br>
> > > > > I /guess/ we never end up codegen'ing dfsw$main? (I haven't looked)<br>
> > and<br>
> > > > > thus the debug info doesn't describe any function at all. It's<br>
> > possible<br>
> > > > > that there's some other reason we don't end up describing any<br>
> > function at<br>
> > > > > all...<br>
> > > > ><br>
> > > > > In any case if we did have "main" in the debug info describe a<br>
> > function,<br>
> > > > at<br>
> > > > > this point, it would be describing the dfsw$main, not main main. So<br>
> > we'd<br>
> > > > > need to handle remapping the debug info back to the original<br>
> > > > uninstrumented<br>
> > > > > function anyway, I assume? (that's the expected behavior? That the<br>
> > > > > uninstrumented function is the one described by debug info, not the<br>
> > > > > instrumented wrapper?)<br>
> > > ><br>
> > > > Yes, the debug info describes 'main', not 'dfsw$main'. So I agree that<br>
> > > > the references in the debug info would need to refer to 'main' instead<br>
> > of<br>
> > > > 'dfsw$main'. Not sure if there is a good way to do that. Perhaps we<br>
> > could<br>
> > > > manually RAUW and skip metadata, but I'm not sure if that would work<br>
> > if the<br>
> > > > debug metadata is not a direct user.<br>
> > > ><br>
> > > > Thanks,<br>
> > > > --<br>
> > > > Peter<br>
> > > ><br>
> ><br>
> > > diff --git lib/Transforms/Instrumentation/DataFlowSanitizer.cpp<br>
> > lib/Transforms/Instrumentation/DataFlowSanitizer.cpp<br>
> > > index 446bcf7..cb84fd6 100644<br>
> > > --- lib/Transforms/Instrumentation/DataFlowSanitizer.cpp<br>
> > > +++ lib/Transforms/Instrumentation/DataFlowSanitizer.cpp<br>
> > > @@ -51,6 +51,7 @@<br>
> > > #include "llvm/ADT/StringExtras.h"<br>
> > > #include "llvm/Analysis/ValueTracking.h"<br>
> > > #include "llvm/IR/Dominators.h"<br>
> > > +#include "llvm/IR/DebugInfo.h"<br>
> > > #include "llvm/IR/IRBuilder.h"<br>
> > > #include "llvm/IR/InlineAsm.h"<br>
> > > #include "llvm/IR/InstVisitor.h"<br>
> > > @@ -243,6 +244,7 @@ class DataFlowSanitizer : public ModulePass {<br>
> > > DFSanABIList ABIList;<br>
> > > DenseMap<Value *, Function *> UnwrappedFnMap;<br>
> > > AttributeSet ReadOnlyNoneAttrs;<br>
> > > + DenseMap<const Function *, DISubprogram> FunctionDIs;<br>
> > ><br>
> > > Value *getShadowAddress(Value *Addr, Instruction *Pos);<br>
> > > bool isInstrumented(const Function *F);<br>
> > > @@ -573,6 +575,8 @@ bool DataFlowSanitizer::runOnModule(Module &M) {<br>
> > > if (ABIList.isIn(M, "skip"))<br>
> > > return false;<br>
> > ><br>
> > > + FunctionDIs = makeSubprogramMap(M);<br>
> > > +<br>
> > > if (!GetArgTLSPtr) {<br>
> > > Type *ArgTLSTy = ArrayType::get(ShadowTy, 64);<br>
> > > ArgTLS = Mod->getOrInsertGlobal("__dfsan_arg_tls", ArgTLSTy);<br>
> > > @@ -725,6 +729,12 @@ bool DataFlowSanitizer::runOnModule(Module &M) {<br>
> > > Value *WrappedFnCst =<br>
> > > ConstantExpr::getBitCast(NewF, PointerType::getUnqual(FT));<br>
> > > F.replaceAllUsesWith(WrappedFnCst);<br>
> > > +<br>
> > > + // Patch the pointer to LLVM function in debug info descriptor.<br>
> > > + auto DI = FunctionDIs.find(&F);<br>
> > > + if (DI != FunctionDIs.end())<br>
> > > + DI->second.replaceFunction(&F);<br>
> > > +<br>
> > > UnwrappedFnMap[WrappedFnCst] = &F;<br>
> > > *i = NewF;<br>
> > ><br>
> ><br>
> ><br>
> > --<br>
> > Peter<br>
> ><br>
<br>
</div></div><span class=""><font color="#888888">--<br>
Peter<br>
</font></span></blockquote></div><br></div></div>