<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 2:51 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div>Sure - though how would I get the pre-dfsan .ll file to produce this behavior? I've tried compiling to a .ll file without dfsan, then feeling that .ll through opt -dfsan, and I got different output. Instead of wrapping main, it just replaced it. Is there some attribute or other thing the frontend is adding to ensure main is wrapped, rather than converted?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><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 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>> 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>
> > wrote:<br>
> > > ><br>
> > > >><br>
> > > >><br>
> > > >> On Tue, Oct 7, 2014 at 11:48 AM, Peter Collingbourne <<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 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 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 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 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 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 the<br>
> > > >> debug info.<br>
> > > >><br>
> > > >><br>
> > > >>> But the code below that can also create wrapper functions which 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 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>
> > > >>> #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>
> > > >>> In this example, 'len' is rewritten to 'dfs$len', 'main' keeps 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 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 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 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 function<br>
> > for<br>
> > > >>> strlen.<br>
> > > >>><br>
> > > >>> It looks like the debug info for 'len' is preserved correctly, 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 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 to<br>
> > > > dfsw$main (this issue doesn't come up with DAE or ArgPromo because they<br>
> > > > both don<br>
> > > ><br>
> > ><br>
> > > ... they both don't use RAUW because they have to visit each call site to<br>
> > > do special stuff anyway. (they're replacing one function with 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) and<br>
> > > thus the debug info doesn't describe any function at all. It's possible<br>
> > > that there's some other reason we don't end up describing any function at<br>
> > > all...<br>
> > ><br>
> > > In any case if we did have "main" in the debug info describe a function,<br>
> > at<br>
> > > this point, it would be describing the dfsw$main, not main main. So 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 of<br>
> > 'dfsw$main'. Not sure if there is a good way to do that. Perhaps we could<br>
> > manually RAUW and skip metadata, but I'm not sure if that would work if the<br>
> > debug metadata is not a direct user.<br>
> ><br>
> > Thanks,<br>
> > --<br>
> > Peter<br>
> ><br>
<br>
</div></div>> diff --git lib/Transforms/Instrumentation/DataFlowSanitizer.cpp 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>
<span class="HOEnZb"><font color="#888888"><br>
<br>
--<br>
Peter<br>
</font></span></blockquote></div><br></div></div>