<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 11, 2015 at 2:14 PM, Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1575<br>
@@ -1574,3 +1574,3 @@<br>
// Regular entry.<br>
- AP.EmitDwarfRegOp(Streamer, Loc);<br>
- else {<br>
+ bool ValidReg = Loc.isIndirect()<br>
+ ? DwarfExpr.AddMachineRegIndirect(Loc.getReg(), Loc.getOffset())<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> This code looks repetitious (same as the code in addAddress) - should it be wrapped up somewhere?<br>
</span>Yes absolutely, I intentionally wrote it that way so I could factor out the (now) obvious similarities in an NFC follow-up commit.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:81<br>
@@ -93,1 +80,3 @@<br>
+ unsigned PieceOffsetInBits,<br>
+ bool MayUsePieceForSubRegMask) {<br>
if (!TRI.isPhysicalRegister(MachineReg))<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> 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?<br>
</span>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?<br></blockquote><div><br>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.<br><br>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.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span><br>
<br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:103<br>
@@ +102,3 @@<br>
+ uint64_t SubRegOffset = TRI.getSubRegIdxOffset(Idx);<br>
+ // +--------------+------+-----------+<br>
+ // | SubRegOffset | Size | UpperBits |<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> Not quite sure what this asciiart is meant to add?<br>
</span>Good point, renaming Size -> SubRegSize seems to carry the same information.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:116<br>
@@ +115,3 @@<br>
+ if (MayUsePieceForSubRegMask && SubRegOffset == 0) {<br>
+ // Use the smaller DW_OP_piece instead of creating a mask for a<br>
+ // subregister at offset 0.<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> 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)<br>
</span>"Use the shorter DW_OP_piece to describe a subregister at offset 0."<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfExpression.cpp:122<br>
@@ +121,3 @@<br>
+ }<br>
+ if (Size > PieceSizeInBits) {<br>
+ // Mask out the uninteresting higher bits.<br>
----------------<br>
</span><span>dblaikie wrote:<br>
> 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?<br>
</span>Correct.<br>
<span><br>
================<br>
Comment at: lib/CodeGen/AsmPrinter/DwarfUnit.cpp:550<br>
@@ +549,3 @@<br>
+ DIEDwarfExpression DwarfExpr(*Asm, *this, *Loc);<br>
+ bool ValidReg = Location.isIndirect()<br>
+ ? DwarfExpr.AddMachineRegIndirect(Location.getReg(), Location.getOffset())<br>
----------------<br>
</span>dblaikie wrote:<br>
> Same repetition again<br>
I'd like to clean this up in a follow-up commit since this is orthogonal to this bugfix.<br>
<div><div><br>
<a href="http://reviews.llvm.org/D8233" target="_blank">http://reviews.llvm.org/D8233</a><br>
<br>
EMAIL PREFERENCES<br>
<a href="http://reviews.llvm.org/settings/panel/emailpreferences/" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>