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

[Adrian Prantl]

================
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?


================
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/