[llvm] r307729 - [IPO] Temporarily rollback r307215.

Wed Jul 12 23:43:18 PDT 2017

On Wed, Jul 12, 2017 at 4:43 PM Peter Collingbourne <peter at pcc.me.uk> wrote:

> On Wed, Jul 12, 2017 at 4:26 PM, Davide Italiano <davide at freebsd.org>
> wrote:
>
>> On Wed, Jul 12, 2017 at 4:17 PM, Davide Italiano <davide at freebsd.org>
>> wrote:
>> > On Tue, Jul 11, 2017 at 4:10 PM, Davide Italiano via llvm-commits
>> > <llvm-commits at lists.llvm.org> wrote:
>> >> Author: davide
>> >> Date: Tue Jul 11 16:10:17 2017
>> >> New Revision: 307729
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=307729&view=rev
>> >> Log:
>> >> [IPO] Temporarily rollback r307215.
>> >>
>> >> [GlobalOpt] Remove unreachable blocks before optimizing a function.
>> >> While the change is presumably correct, it exposes a latent bug
>> >> in DI which breaks on of the CFI checks. I'll analyze it further
>> >> and try to understand what's going on.
>> >>
>> >
>> > After Matt's report and the revert I looked at this bug at length today.
>> > The CFI tests are failing because `sanstats` is not actually able to
>> > map back to the correct line number (see, e.g. the test called
>> > stats.cpp in compiler-rt/test/cfi/cross-dso/)
>> >
>> > i.e. in the working case we have:
>> >
>> >
>> /home/davide/work/llvm/projects/compiler-rt/test/cfi/cross-dso/stats.cpp:23
>> > vcall.cfi cfi-vcall 37
>> >
>> > and in the broken case we have:
>> >
>> >
>> /home/davide/work/llvm/projects/compiler-rt/test/cfi/cross-dso/stats.cpp:0
>> > vcall.cfi cfi-vcall 37
>> >
>> > The difference in IR with and without this patch for @vcall.cfi is the
>> > following:
>> >
>> > -  br label %cfi.slowpath, !dbg !20
>> > + br i1 false, label %cfi.cont, label %cfi.slowpath, !dbg !20, !prof
>> > !21, !nosanitize !2
>> >
>> > In the "broken" case (i.e. with the globalOpt patch) we transform a
>> > branch on a known condition to an unconditional branch:
>> >
>> > The two IR dumps look like:
>> >
>> > WORKING:
>> >
>> >   vtable = load void (%struct.a*)**, void (%struct.a*)*** %1, align 8,
>> !dbg !20
>> >   call void @__sanitizer_stat_report(...), !dbg !20
>> >   %2 = bitcast void (%struct.a*)** %vtable to i8*, !dbg !20,
>> !nosanitize !2
>> >   br i1 false, label %cfi.cont, label %cfi.slowpath, !dbg !20, !prof
>> > !21, !nosanitize !2
>> >
>> > !20 = !DILocation(line: 23, column: 6, scope: !9)
>> >
>> > and
>> >
>> > BROKEN:
>> >
>> >   %vtable = load void (%struct.A*)**, void (%struct.A*)*** %1, align 8,
>> !dbg !20
>> >   call void @__sanitizer_stat_report(...), !dbg !20
>> >   %2 = bitcast void (%struct.A*)** %vtable to i8*, !dbg !20,
>> !nosanitize !2
>> >   br label %cfi.slowpath, !dbg !20
>> >
>> > !20 = !DILocation(line: 23, column: 6, scope: !9)
>> >
>> > (this information lives up to the end of the optimizer so I don't
>> > think it's a bug in the middle-end). Note, the `!prof` and
>> > `!nosanitize` metadata attached to the `br` instruction are stripped
>> > as well but I'm not sure it matters here.
>> >
>> >
>> > The code generated looks like:
>> > WORKING:
>> >
>> >     206e:       4c 8b 33                mov    (%rbx),%r14
>> >     2071:       48 89 c7                mov    %rax,%rdi
>> >     2074:       e8 d7 f0 ff ff          callq  1150
>> <__sanitizer_stat_report>
>> >     2079:       31 c0                   xor    %eax,%eax
>> >     207b:       a8 01                   test   $0x1,%al
>> >     207d:       75 12                   jne    2091 <vcall.cfi+0x41>
>> >     207f:       48 bf 12 14 8e 46 2e    movabs $0x6133c22e468e1412,%rdi
>> >
>> > and line table:
>> >
>> > Address            Line   Column File   ISA Discriminator Flags
>> > ------------------ ------ ------ ------ --- ------------- -------------
>> > 0x000000000000206e     23      6      1   0             0
>> > 0x000000000000207f      0      6      1   0             0
>> > 0x0000000000002089     23      6      1   0             0
>> >
>> > BROKEN:
>> >
>> >     2071:       48 89 c7                mov    %rax,%rdi
>> >     2074:       e8 d7 f0 ff ff          callq  1150
>> <__sanitizer_stat_report>
>> >     2079:       48 bf 12 14 8e 46 2e    movabs $0x6133c22e468e1412,%rdi
>> >
>> > Address            Line   Column File   ISA Discriminator Flags
>> > ------------------ ------ ------ ------ --- ------------- -------------
>> > 0x000000000000206e     23      6      1   0             0
>> > 0x0000000000002079      0      6      1   0             0
>> > 0x0000000000002083     23      6      1   0             0
>> >
>> >
>> > `sanstats` asks for 0x2079.
>> > So it just seems to me without this optimization we were able to get
>> > the correct line number because there were some instructions mapped to
>> > the same line before the `movabs`, which then are removed when we fold
>> > the branch so we map back to "0". I'm not entirely sure who's at fault
>> > here. Thoughts?
>> >
>> > --
>> > Davide
>>
>> Note, I don't know how this whole machinery works (at least not in
>> detail ;) so I could be completely off, but I expected `sanstats` to
>> ask the symbolication of the call, i.e.
>>
>>     2074:       e8 d7 f0 ff ff          callq  1150
>> <__sanitizer_stat_report>
>>
>> `0x2074` instead of `0x2079`.
>>
>
> I also believe this to be a bug in sanstats. The underlying
> instrumentation tracks only the return address, not the address of the call:
> http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/stats/stats_client.cc#72
> so we need to compensate for that in sanstats.
>
> I think the best thing to do would be to change sanstats to subtract one
> from the return address before symbolizing.
>

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...

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.

It's the backtrace tool/library, I think, that might do better for users if
it symbolized ReturnAddress - 1.

But advanced users may find that confusing...

(CC'd Chandler & Eric in case they had ideas around this)

> I'll take care of that. Sorry for the trouble!
>
> Thanks,
> --
> --
> Peter
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170713/ccdb8e46/attachment.html>