<div dir="ltr"><br><div class="gmail_extra"><br><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="gmail-HOEnZb"><div class="gmail-h5">On Wed, Jul 12, 2017 at 4:17 PM, Davide Italiano <<a href="mailto:davide@freebsd.org">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">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-<wbr>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-<wbr>dso/)<br>
><br>
> i.e. in the working case we have:<br>
><br>
> /home/davide/work/llvm/<wbr>projects/compiler-rt/test/cfi/<wbr>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/<wbr>projects/compiler-rt/test/cfi/<wbr>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="gmail-"><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>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">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. I'll take care of that. Sorry for the trouble!</div><div><br></div><div>Thanks,</div></div>-- <br><div class="gmail_signature"><div dir="ltr">-- <div>Peter</div></div></div>
</div></div>