<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 12, 2017 at 4:43 PM Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 12, 2017 at 4:26 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="m_-3705704942787822768gmail-HOEnZb"><div class="m_-3705704942787822768gmail-h5">On Wed, Jul 12, 2017 at 4:17 PM, Davide Italiano <<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>> wrote:<br>
> On Tue, Jul 11, 2017 at 4:10 PM, Davide Italiano via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> Author: davide<br>
>> Date: Tue Jul 11 16:10:17 2017<br>
>> New Revision: 307729<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=307729&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=307729&view=rev</a><br>
>> Log:<br>
>> [IPO] Temporarily rollback r307215.<br>
>><br>
>> [GlobalOpt] Remove unreachable blocks before optimizing a function.<br>
>> While the change is presumably correct, it exposes a latent bug<br>
>> in DI which breaks on of the CFI checks. I'll analyze it further<br>
>> and try to understand what's going on.<br>
>><br>
><br>
> After Matt's report and the revert I looked at this bug at length today.<br>
> The CFI tests are failing because `sanstats` is not actually able to<br>
> map back to the correct line number (see, e.g. the test called<br>
> stats.cpp in compiler-rt/test/cfi/cross-dso/)<br>
><br>
> i.e. in the working case we have:<br>
><br>
> /home/davide/work/llvm/projects/compiler-rt/test/cfi/cross-dso/stats.cpp:23<br>
> vcall.cfi cfi-vcall 37<br>
><br>
> and in the broken case we have:<br>
><br>
> /home/davide/work/llvm/projects/compiler-rt/test/cfi/cross-dso/stats.cpp:0<br>
> vcall.cfi cfi-vcall 37<br>
><br>
> The difference in IR with and without this patch for @vcall.cfi is the<br>
> following:<br>
><br>
> -  br label %cfi.slowpath, !dbg !20<br>
> + br i1 false, label %cfi.cont, label %cfi.slowpath, !dbg !20, !prof<br>
> !21, !nosanitize !2<br>
><br>
> In the "broken" case (i.e. with the globalOpt patch) we transform a<br>
> branch on a known condition to an unconditional branch:<br>
><br>
> The two IR dumps look like:<br>
><br>
> WORKING:<br>
><br>
>   vtable = load void (%struct.a*)**, void (%struct.a*)*** %1, align 8, !dbg !20<br>
>   call void @__sanitizer_stat_report(...), !dbg !20<br>
>   %2 = bitcast void (%struct.a*)** %vtable to i8*, !dbg !20, !nosanitize !2<br>
>   br i1 false, label %cfi.cont, label %cfi.slowpath, !dbg !20, !prof<br>
> !21, !nosanitize !2<br>
><br>
> !20 = !DILocation(line: 23, column: 6, scope: !9)<br>
><br>
> and<br>
><br>
> BROKEN:<br>
><br>
>   %vtable = load void (%struct.A*)**, void (%struct.A*)*** %1, align 8, !dbg !20<br>
>   call void @__sanitizer_stat_report(...), !dbg !20<br>
>   %2 = bitcast void (%struct.A*)** %vtable to i8*, !dbg !20, !nosanitize !2<br>
>   br label %cfi.slowpath, !dbg !20<br>
><br>
> !20 = !DILocation(line: 23, column: 6, scope: !9)<br>
><br>
> (this information lives up to the end of the optimizer so I don't<br>
> think it's a bug in the middle-end). Note, the `!prof` and<br>
> `!nosanitize` metadata attached to the `br` instruction are stripped<br>
> as well but I'm not sure it matters here.<br>
><br>
><br>
> The code generated looks like:<br>
> WORKING:<br>
><br>
>     206e:       4c 8b 33                mov    (%rbx),%r14<br>
>     2071:       48 89 c7                mov    %rax,%rdi<br>
>     2074:       e8 d7 f0 ff ff          callq  1150 <__sanitizer_stat_report><br>
>     2079:       31 c0                   xor    %eax,%eax<br>
>     207b:       a8 01                   test   $0x1,%al<br>
>     207d:       75 12                   jne    2091 <vcall.cfi+0x41><br>
>     207f:       48 bf 12 14 8e 46 2e    movabs $0x6133c22e468e1412,%rdi<br>
><br>
> and line table:<br>
><br>
> Address            Line   Column File   ISA Discriminator Flags<br>
> ------------------ ------ ------ ------ --- ------------- -------------<br>
> 0x000000000000206e     23      6      1   0             0<br>
> 0x000000000000207f      0      6      1   0             0<br>
> 0x0000000000002089     23      6      1   0             0<br>
><br>
> BROKEN:<br>
><br>
>     2071:       48 89 c7                mov    %rax,%rdi<br>
>     2074:       e8 d7 f0 ff ff          callq  1150 <__sanitizer_stat_report><br>
>     2079:       48 bf 12 14 8e 46 2e    movabs $0x6133c22e468e1412,%rdi<br>
><br>
> Address            Line   Column File   ISA Discriminator Flags<br>
> ------------------ ------ ------ ------ --- ------------- -------------<br>
> 0x000000000000206e     23      6      1   0             0<br>
> 0x0000000000002079      0      6      1   0             0<br>
> 0x0000000000002083     23      6      1   0             0<br>
><br>
><br>
> `sanstats` asks for 0x2079.<br>
> So it just seems to me without this optimization we were able to get<br>
> the correct line number because there were some instructions mapped to<br>
> the same line before the `movabs`, which then are removed when we fold<br>
> the branch so we map back to "0". I'm not entirely sure who's at fault<br>
> here. Thoughts?<br>
><br>
> --<br>
> Davide<br>
<br>
</div></div>Note, I don't know how this whole machinery works (at least not in<br>
detail ;) so I could be completely off, but I expected `sanstats` to<br>
ask the symbolication of the call, i.e.<br>
<span class="m_-3705704942787822768gmail-"><br>
    2074:       e8 d7 f0 ff ff          callq  1150 <__sanitizer_stat_report><br>
<br>
</span>`0x2074` instead of `0x2079`.<br></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I also believe this to be a bug in sanstats. The underlying instrumentation tracks only the return address, not the address of the call:</div><div><a href="http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/stats/stats_client.cc#72" target="_blank">http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/stats/stats_client.cc#72</a></div><div>so we need to compensate for that in sanstats.</div><div><br></div><div>I think the best thing to do would be to change sanstats to subtract one from the return address before symbolizing.</div></div></div></div></blockquote><div><br>Yep, imho this is likely the right fix. I've tracked at least one other symbolication issue to the same sort of thing - the user thought they'd find the line a call was made in the backtrace, but instead it was the instruction after that (that had no location, because of optimizations), etc...<br><br>I wonder if there's something general we could do in symbolizers, etc, to address this sort of problem - but I doubt it. The symbolizer is just told to symbolize an address, which it should do faithfully.<br><br>It's the backtrace tool/library, I think, that might do better for users if it symbolized ReturnAddress - 1.<br><br>But advanced users may find that confusing... <br><br>(CC'd Chandler & Eric in case they had ideas around this)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div> I'll take care of that. Sorry for the trouble!</div><div><br></div><div>Thanks,</div></div></div></div><div dir="ltr"><div class="gmail_extra">-- <br><div class="m_-3705704942787822768gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div></blockquote></div></div>