<div dir="ltr">Here's a basic patch which would solve it in sort of the same way as the other optimizations I was fixing (just special case the debug info & fix it up). I can work up a test case for this as well, or you can, if you like/this seems reasonable.</div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 7, 2014 at 2:30 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"><div class="HOEnZb"><div class="h5">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>> 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>> 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>
> >> 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 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 runtime<br>
> >>> library<br>
> >>> does not make use of debug info. The debug info could still be useful for<br>
> >>> regular debugging tasks though.<br>
> >>><br>
> >><br>
> >> Yeah - that'd be the baseline. I assume some people probably care 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 update<br>
> >>> the<br>
> >>> > debug info for these functions? Or are these internal instrumentation<br>
> >>> > functions & nothing to do with the code the user wrote? I can't 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 not<br>
> >>> need<br>
> >>> debug info (lines 712-746). Wrappers normally show up for uninstrumented<br>
> >>> functions (e.g. main and many libc functions).<br>
> >>><br>
> >>> > Could you provide any C/C++ source examples whis part of DFSan 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 function<br>
> >>> and<br>
> >>> wrapper features.<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>
> >>> 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 wrappers are<br>
> >>> created for 'main' and 'strlen' (the wrapper for 'main' is unused as 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? "main"<br>
> >> keeps the name? (it's important which way this transformation is done - if<br>
> >> the guts of main are moved to a new function and the name moved as well,<br>
> >> that's not the same as keeping the same function as far as debug info<br>
> >> metadata is concerned)<br>
<br>
</div></div>In this case the function keeps its original name and the wrapper is created<br>
separately.<br>
<div><div class="h5"><br>
> >>> I compile this with '-O0 -g'. A 'break main'/'run'/'break 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 <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 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 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 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, 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 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>
</div></div>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>
<span class="HOEnZb"><font color="#888888">--<br>
Peter<br>
</font></span></blockquote></div><br></div>