[PATCH] D27550: Fix LLVM's use of DW_OP_bit_piece in DWARF expressions.

[David Blaikie via llvm-commits]

On Fri, Dec 9, 2016 at 11:34 AM Adrian Prantl <aprantl at apple.com> wrote:

> To verify this you would need to replicate all the buildLocationList()
> logic in the Verifier. I'm also not sure if this isn't something that could
> happen legitimately because of optimizations.


Right - but (much like that infamous assertion I added) knowing why/what
the right behavior rather than allowing it implicitly might be worthwhile.

Adding a check until we discover a reasonable case where it should occur
and decide what should happen there may be worthwhile.


> I can't think of a way this would happen today, but I wouldn't be
> surprised if it were possible due to, e.g, repeated SROA.


Yeah, that's somewhat my concern - if it's valid to have multiple
simultaneous locations (which it is, and DWARF supports - but it supports
it with location lists, I believe, not with a direct location description?
- not sure whether the location list applies to global variables, or how
we'd describe the address range (since it might cover comdats, other
functions not in this CU, etc)... )

Just a thought.


> The backend could certainly become smarter about which fragment to choose
> to get optimal coverage though.
>
> -- adrian
>
> > On Dec 9, 2016, at 11:13 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Should we assert/fail verification on this? (probably a separate
> question from this patch)
> >
> >
> > On Fri, Dec 9, 2016 at 11:08 AM Adrian Prantl <aprantl at apple.com> wrote:
> >> On Dec 9, 2016, at 11:01 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >>
> >>
> >> On Fri, Dec 9, 2016 at 10:43 AM Adrian Prantl via Phabricator <
> reviews at reviews.llvm.org> wrote:
> >> aprantl marked 4 inline comments as done.
> >> aprantl added a comment.
> >>
> >> In https://reviews.llvm.org/D27550#618335, @dblaikie wrote:
> >>
> >> > Not quite following all the changes here, but they look roughly
> plausible.
> >> >
> >> > Is there some code I couldn't find that sorts all the fragments
> before emitting them? (the old scheme wouldn't've needed this, since each
> bit_piece could specify where it appears - but maybe we were already
> sorting them anyway so pieces were linearly increasing in position? - the
> new scheme would require such sorting because the positions are implicit
> relative to prior pieces)
> >>
> >>
> >> Yes, DebugLocEntry::sortUniqueValues() (and possibly others locations)
> is sorting the fragments by offset.
> >>
> >> Ah, thanks - and what happens if there's overlap? Is there some error
> handling?
> >
> > DwarfDebug::buildLocationList() collects looks at the fragments and and
> then chops up their ranges into the smallest continuous region that is
> covered by a set of non-overlapping fragments. So what will likely happen
> is that it will produce a zero-sized region for the first overlapping
> fragment that will then be silently dropped.
> >
> > -- adrian
> >
> >>
> >>
> >>
> >>
> >> ================
> >> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:505
> >>    }
> >> +  Expr.AddExpression(ArrayRef<uint64_t>(DIExpr));
> >> +  Expr.finalize();
> >> ----------------
> >> dblaikie wrote:
> >> > makeArrayRef?
> >> Nice. Didn't know we had that.
> >>
> >>
> >> ================
> >> Comment at: test/DebugInfo/AArch64/frameindices.ll:9
> >> +; CHECK: DW_AT_location [DW_FORM_block1]    (<0x0c> 93 01 91 51 93 0f
> 93 01 91 4a 93 07 )
> >> +;  -- piece 0x00000001, fbreg -47, piece 0x0000000f, piece 0x00000001,
> fbreg -54, piece 0x00000007 ------^
> >>  ; CHECK: DW_AT_abstract_origin {{.*}} "p1"
> >> ----------------
> >> dblaikie wrote:
> >> > Took me a bit to go through both these examples (keeping both the
> DW_OP_bit_piece/piece interpretations in my head, etc) - but yes, great to
> see this, as it looks good/right/etc :)
> >> Thanks, from past experience,  I really appreciate someone
> double-checking this!
> >>
> >>
> >> ================
> >> Comment at: test/DebugInfo/MIR/X86/bit-piece-dh.mir:1
> >> +# RUN: llc -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
> >> +# CHECK: .debug_info contents:
> >> ----------------
> >> dblaikie wrote:
> >> > what's 'dh' stand for in the title of this test?
> >> >
> >> > What codepath does this test? (looks like the expression isn't in
> DIExpression - it's created in some MI/CodeGen layer?)
> >> The 8-bit x86 register "dh" hi-part of (r)dx.
> >>
> >> ```
> >> 0    8   16    32       64
> >> +-----------------------+
> >> |              rdx      |
> >> |        edx    |       |
> >> |    dx   |     |       |
> >> | dl | dh |     |       |
> >> +----+----+-----+-------+
> >> ```
> >>
> >> This is testing DwarfExpression.cpp:111 (Walk up the super-register
> chain until we find a valid number and emit a DW_OP_bit_piece for it).
> >>
> >>
> >> ================
> >> Comment at: test/DebugInfo/X86/dw_op_minus_direct.ll:9-12
> >> +; CHECK: Beginning address offset: 0x0000000000000000
> >> +; CHECK:    Ending address offset: 0x0000000000000004
> >> +; CHECK:     Location description: 50 10 01 1c 93 04
> >> +;                                  rax, constu 0x00000001, minus,
> piece 0x00000004
> >> ----------------
> >> dblaikie wrote:
> >> > This is test coverage for DwarfExpression.cpp:244-ish? Probably
> better as a separate commit?
> >> It tests a bugfix that fell out of this rewrite/factoring. The code in
> trunk would have crashed when it encountered a DW_OP_minus without a
> DW_OP_deref. I would leave it in here.
> >>
> >>
> >> https://reviews.llvm.org/D27550
> >>
> >>
> >>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161209/d5d52e94/attachment.html>