[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 2:14 PM, Adrian Prantl <aprantl at apple.com> wrote:

> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1575
> @@ -1574,3 +1574,3 @@
>        // Regular entry.
> -      AP.EmitDwarfRegOp(Streamer, Loc);
> -    else {
> +      bool ValidReg = Loc.isIndirect()
> +        ? DwarfExpr.AddMachineRegIndirect(Loc.getReg(), Loc.getOffset())
> ----------------
> dblaikie wrote:
> > This code looks repetitious (same as the code in addAddress) - should it
> be wrapped up somewhere?
> Yes absolutely, I intentionally wrote it that way so I could factor out
> the (now) obvious similarities in an NFC follow-up commit.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:81
> @@ -93,1 +80,3 @@
> +                                         unsigned PieceOffsetInBits,
> +                                         bool MayUsePieceForSubRegMask) {
>    if (!TRI.isPhysicalRegister(MachineReg))
> ----------------
> dblaikie wrote:
> > Maybe name this flag "MayUsePiece" - I imagine that if GDB has trouble
> with piece for subregs, it'd have trouble with it for anything else too?
> I'm pretty sure we would have heard that by now. Besides, if this was a
> general flag to disable DW_OP_piece, what condition should it be triggered
> on: a command line option?
>

Sorry, I meant a general option to disable DW_OP_piece for this function -
and the frame_base caller passes "yes, disable DW_OP_piece" and everyone
else doesn't.

I just meant that using DW_OP_piece is probably broken for GDB in any place
in DW_AT_frame_base, not just for a subreg masking operation.


>
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:103
> @@ +102,3 @@
> +      uint64_t SubRegOffset = TRI.getSubRegIdxOffset(Idx);
> +      // +--------------+------+-----------+
> +      // | SubRegOffset | Size | UpperBits |
> ----------------
> dblaikie wrote:
> > Not quite sure what this asciiart is meant to add?
> Good point, renaming Size -> SubRegSize seems to carry the same
> information.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:116
> @@ +115,3 @@
> +      if (MayUsePieceForSubRegMask && SubRegOffset == 0) {
> +        // Use the smaller DW_OP_piece instead of creating a mask for a
> +        // subregister at offset 0.
> ----------------
> dblaikie wrote:
> > I'd phrase this the other way - this comment should just describe the
> obvious case in the positive "use the shorter encoding if no shifting is
> required" (paraphrased - those exact words are a bit too brief)
> "Use the shorter DW_OP_piece to describe a subregister at offset 0."
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:122
> @@ +121,3 @@
> +      }
> +      if (Size > PieceSizeInBits) {
> +        // Mask out the uninteresting higher bits.
> ----------------
> dblaikie wrote:
> > This looks like it should be an elseif - since the last line of the
> previous if block establishes a condition that can't possibly be true for
> this condition anyway, right?
> Correct.
>
> ================
> Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:550
> @@ +549,3 @@
> +  DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);
> +  bool ValidReg = Location.isIndirect()
> +    ? DwarfExpr.AddMachineRegIndirect(Location.getReg(),
> Location.getOffset())
> ----------------
> dblaikie wrote:
> > Same repetition again
> I'd like to clean this up in a follow-up commit since this is orthogonal
> to this bugfix.
>
> http://reviews.llvm.org/D8233
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150312/b9df998a/attachment.html>