[PATCH] Fix the remainder of PR22762 (GDB is crashing on DW_OP_piece being used inside of DW_AT_frame_base)

[David Blaikie]

On Wed, Mar 11, 2015 at 1:08 PM, Adrian Prantl <aprantl at apple.com> wrote:

>
> > On Mar 11, 2015, at 12:41 PM, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Mar 11, 2015 at 11:26 AM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >
> >> On Mar 11, 2015, at 11:22 AM, Adrian Prantl <aprantl at apple.com> wrote:
> >>
> >>
> >>> On Mar 11, 2015, at 11:16 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>
> >>>
> >>>
> >>> On Tue, Mar 10, 2015 at 3:53 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >>>
> >>>> On Mar 10, 2015, at 3:35 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On Tue, Mar 10, 2015 at 3:26 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >>>>
> >>>>> On Mar 10, 2015, at 3:14 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Tue, Mar 10, 2015 at 2:50 PM, Adrian Prantl <aprantl at apple.com>
> wrote:
> >>>>> Hi echristo, dblaikie,
> >>>>>
> >>>>> http://llvm.org/bugs/show_bug.cgi?id=22762
> >>>>> The symptom was that DW_AT_frame_base should never use a
> DW_OP_(bit)_piece, the bug was that AddMachineRegPiece incorrectly created
> pieces to describe values that occupy only a subregister. Change this to
> emit a bit mask instead.
> >>>>>
> >>>>> I'm posting this for review because emitting the bit mask increases
> the size occupied for DWARF expressions for sub-registers (~5 bytes for a
> 32-bit subregister). Previously we would (incorrectly!) use DW_OP_piece to
> describe a value occupying part of a register. However, "DW_OP_piece
> provides a way of describing how large a part of a variable a particular
> DWARF location description refers to.",
> >>>>>
> >>>>> The spec also says "If the piece is located in a register, but does
> not occupy the entire register, the placement of the piece within that
> register is defined by the ABI. " - so we can use this in some cases at
> least. Should we? I assume it just means if we say _piece of size 1 in a
> register of size 4 we get the low byte (whatever definition of 'low' there
> is)?
> >>>>>
> >>>>> not the size and offset of an entire variable inside a
> super-register. The way that most debuggers implement DW_OP_piece this sort
> of works out for subregisters that are at offset 0, but it causes confusion
> if the expression needs to be composed (such as in DW_AT_frame_base, or if
> the subregister contains only a part of the variable).
> >>>>
> >>>> Yes this is exactly the edge case that we were relying on up to now.
> There are two problems I have with that:
> >>>> a) how do we implement the “defined by the ABI” predicate correctly?
> Assume that it’s always the subregister at offset 0 and wait until someone
> complains?
> >>>>
> >>>> I'd probably be OK with this, open to other opinions, but it seems
> pretty simply like "we stuffed this number in a big register, but we don't
> need all the bits in the big register, so take the lowest bits that we
> specify" - any platform that did anything stranger than that... well, we
> should probably have a talk with/about them anyway, so I wouldn't mind if
> them running into this feature was what caused us to have that discussion
> >>>
> >>> Fine. I’ll do it this way then.
> >>>
> >>>> .
> >>>>
> >>>> b) it doesn’t compose well, so we’d need to explicitly forbid it
> inside of DW_AT_frame_base and inside of a larger piece expression.
> >>>>
> >>>> This bit I don't really understand - perhaps you could provide some
> expression examples?
> >>>
> >>> As for DW_AT_frame_base, according to the PR, gdb just crashes if it
> encounters a DW_OP_piece in there. More generally, since the expression
> inside DW_AT_frame_base is just going to be evaluated when a DW_OP_fbreg is
> encountered, it makes the result of an expression like "DW_OP_fbreg 4
> DW_OP_piece 4 DW_OP_fbreg 8 DW_OP_piece 4” highly ambiguous.
> >>>
> >>> I'm still really not following how piece, frame_base, and fbreg are
> all connected to this issue.
> >>>
> >>> When/where do we use a piece to describe the frame_base?
> >>
> >> On x86_64-pc-linux-gnux32 the frame base is ebp, but in 64-bit mode ebp
> doesn’t have DWARF register number, so we need a way saying “the lower 32
> bits or rbp”. Before this patch we would emit "DW_OP_reg(rbp) DW_OP_piece
> 4", after this patch it is "DW_OP_reg(rbp) DW_OP_constu 0xffffff DW_OP_and".
> >>
> >>> Is GDB's (which version?) implementation of fbreg not respecting the
> size of the piece specified in a _piece in frame_base?
> >>
> >> According to the PR (I didn’t verify that) gdb crashes when it
> encounter a DW_OP_piece in DW_AT_frame_base.
> >> -> http://llvm.org/bugs/show_bug.cgi?id=22762
> > This might just be the sort of thing we should punt to GDB - but,
> admittedly, might need to workaround it in the interim :/ (would be good to
> document as such, if that's the case)
>
> Agreed, details below.
>
> >
> > ( Even if gdb wouldn’t be crashing we cannot use a DW_OP_piece in
> DW_AT_frame_base because of the ambiguity outlined in
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150309/265207.html.
> )
> >
> > I don't really understand the ambiguity you're referring to - could you
> provide more details?
>
> Let’s assume
> DW_AT_frame_base( DW_OP_reg5 DW_OP_piece 4 ) // lower 32 bits of rbp
>
> My argument was based on the assumption that a debugger might expand
>   DW_OP_fbreg N
> to
>   [frame_base] DW_OP_consts N DW_OP_add DW_OP_deref
>
> which would cause a problem in a location that describes multiple pieces:
>   DW_AT_location( DW_OP_fbreg 0 DW_OP_piece 4 DW_OP_fbreg 4 DW_OP_piece 4 )
>
> However, after reading the definition of DW_AT_frame_base again, it seems
> as if the expectation is more that the expression in DW_AT_frame_base has
> to be fully evaluated separately first before the result is pushed onto the
> stack (and at least LLDB appears to implement it this way, too): "On more
> sophisticated systems [DW_AT_frame_base] might be a location list that
> adjusts the offset according to changes in the stack pointer as the PC
> changes.”
> So I’m feeling less confident about this argument now.
>

Yeah, I don't /think/ that's a reasonable implementation.

So, yeah, I have about ~65% confidence that this should just work and it's
a bug in GDB, so I'd encourage you to file a GDB bug, commit the workaround
as you have it (few comments incoming) with some note in the commit message
and/or a description in a comment about what we think we're working around
here.

- David


>
> -- adrian
>
> >
> >
> >>
> >> -- adrian
> >>
> >>>
> >>>
> >>> For the larger expression, I suppose it actually works with the
> “defined by the ABI”-strategy. Consider a 2-byte struct on x86_64 where the
> lower byte is in al and the higher byte is in bl: DW_OP_reg0 DW_OP_piece 1
> DW_OP_reg4 DW_OP_piece 1.
> >>> And the same struct in al:bh needs some shifting either way.
> >>>
> >>>
> >>>>
> >>>> b) is doable, if ugly, but I’d need some help with a).
> >>>
> >>> Ugly (but compact) it is, then! :-)
> >>>
> >>> thanks!
> >>> adrian
> >>>
> >>>
> >>
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150311/a3f28949/attachment.html>