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

Davide Italiano via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 12 17:50:25 PDT 2017


r307871. Thanks!
I'll let this one bake in the tree for a bit and if/once it's stable
I'll recommit the original globalopt patch.
Phew, I'm so relieved it's not actually a debug info drop from some
optimization.

--
Davide

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. I'll take care of that. Sorry
> for the trouble!
>
> Thanks,
> --
> --
> Peter



-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare


More information about the llvm-commits mailing list