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

Wed Jul 12 16:43:32 PDT 2017

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. 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/20170712/76c1cca9/attachment.html>