<div style="font-family:arial,helvetica,sans-serif"><font><div class="gmail_quote">On 21 June 2012 11:22, Robinson, Paul <span dir="ltr"><<a href="mailto:Paul.Robinson@am.sony.com" target="_blank">Paul.Robinson@am.sony.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I don't have commit access, could you commit for me please?<br>
I think it's straightforward enough that post-commit from Eric would<br>
be fine.<br></blockquote><div><br></div><div>Done, thanks!!</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks,<br>
--paulr<br>
<br>
From: Nick Lewycky [<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>]<br>
Sent: Wednesday, June 20, 2012 5:18 PM<br>
To: Robinson, Paul<br>
Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
Subject: Re: [llvm-commits] [PATCH] PR9493, Fix for DW_AT_location<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On 20 June 2012 16:58, Robinson, Paul <<a href="mailto:Paul.Robinson@am.sony.com">Paul.Robinson@am.sony.com</a>> wrote:<br>
<br>
Revised patch attached.<br>
<br>
I ended up deciding to create EmitLabelReference(Label, Size) as<br>
EmitLabelPlusOffset(Label, 0, Size) and making EmitLabelPlusOffset not do<br>
the Plus part for zero offsets.<br>
<br>
Further research showed there are 3 separate flags for deciding whether to<br>
emit relocations for different attributes. They are all true (default) or<br>
all false (Darwin). Feh.<br>
<br>
<br>
Debug info emission needs more attention than just bugs fixes. This is a symptom of it not getting enough attention for a long time.<br>
<br>
<br>
I renamed the one, and added a FIXME wondering<br>
whether to use it for one of the others.<br>
I did not have the FIXME ask about string pooling, because there is a<br>
proposal before the DWARF committee to revise string pooling to eliminate<br>
a whole pile of relocations, and we'll probably need the separate flag<br>
in the long run.<br>
<br>
Let me know how it looks...<br>
<br>
<br>
<br>
This looks great to me!<br>
<br>
<br>
I'm still at the stage where I'm asking Eric Christopher to also look over debug info changes to ensure it doesn't break Darwin, old-gdb's or lldb before committing my own code. I'll let you decide whether you're confident enough to commit it. :) Eric can do post-commit review too, of course.<br>
<br>
<br>
Nick<br>
<br>
--paulr<br>
<br>
<br>
<br>
________________________________________<br>
From: Robinson, Paul<br>
Sent: Wednesday, June 20, 2012 9:29 AM<br>
To: Nick Lewycky<br>
<br>
Subject: RE: [llvm-commits] [PATCH] PR9493, Fix for DW_AT_location<br>
<br>
<br>
From: Nick Lewycky [<a href="mailto:nlewycky@google.com">nlewycky@google.com</a>]<br>
> --- test/DebugInfo/X86/DW_AT_location-reference.ll (revision 0)<br>
> +++ test/DebugInfo/X86/DW_AT_location-reference.ll (revision 0)<br>
> @@ -0,0 +1,108 @@<br>
> +; RUN: llc -O1 -mtriple=x86_64-apple-darwin < %s | FileCheck -check-prefix=DARWIN %s<br>
> +; RUN: llc -O1 -mtriple=x86_64-pc-linux-gnu < %s | FileCheck -check-prefix=LINUX %s<br>
> +;Bug 9493<br>
> +;Adapted from the original test case in r127757.<br>
><br>
><br>
> It's standard to say "; PR9493" instead of "Bug 9493". There used to be<br>
> code in the test runner that would parse it and emit which PR's had<br>
> regressed, but I think that code is gone?<br>
<br>
Will do.<br>
<br>
> I'm also concerned about the fact that this test runs llc -O1. It looks<br>
> like this will work even if we change the register allocator only because<br>
> we don't actually test the contents in the DWARF location anyhow. I don't<br>
> know how easy this is to do, but it'd be great if you could simplify the<br>
> .ll file or not run it with -O1. It's acceptable as-is if you can't<br>
> simplify it, but I'm not looking forward to debugging why this test fails<br>
> some day in the future.<br>
<br>
The point of the test is to generate a reference to the .debug_loc section,<br>
which won't happen unless there are multiple locations assigned to the<br>
variable, which won't happen unless there is optimization. We don't care<br>
_what_ the locations are, just that there are multiple locations.<br>
If there's a more reliable way to achieve this than "-O1" I'll be happy<br>
to adopt it. I'll add a comment in any case.<br>
<br>
> Index: lib/CodeGen/AsmPrinter/DwarfDebug.cpp<br>
> ===================================================================<br>
> --- lib/CodeGen/AsmPrinter/DwarfDebug.cpp (revision 158734)<br>
> +++ lib/CodeGen/AsmPrinter/DwarfDebug.cpp (working copy)<br>
> @@ -1637,10 +1637,14 @@<br>
> break;<br>
> }<br>
> case dwarf::DW_AT_location: {<br>
> - if (DIELabel *L = dyn_cast<DIELabel>(Values[i]))<br>
> - Asm->EmitLabelDifference(L->getValue(), DwarfDebugLocSectionSym, 4);<br>
> - else<br>
> + if (DIELabel *L = dyn_cast<DIELabel>(Values[i])) {<br>
> + if (Asm->MAI->doesDwarfUseLabelOffsetForRanges())<br>
> + Asm->EmitReference(L->getValue(), dwarf::DW_EH_PE_udata4);<br>
> + else<br>
> + Asm->EmitLabelDifference(L->getValue(), DwarfDebugLocSectionSym, 4);<br>
> + } else {<br>
> Values[i]->EmitValue(Asm, Form);<br>
> + }<br>
> break;<br>
> }<br>
><br>
><br>
> Why do we ignore "Form" when doesDwarfUseLabelOffsetForRanges() is true?<br>
<br>
We ignore "Form" when the value is a label, this has not changed.<br>
<br>
> EH_PE_udata4 can't possibly be right, it's prefixed with EH (exception<br>
> handling) for starters. In DWARF2 which is (still) the only version of<br>
> dwarf we support, your choices for form here are any of DW_FORM_data{1,2,4,8}<br>
> or DW_FORM_block{1,2}. If the problem is GetSizeOfEncodedValue(), please<br>
> extend that.<br>
<br>
That line is copy-pasted from the original patch, just so you know.<br>
I wasn't thrilled with the EH_PE business either, but:<br>
EmitReference passes the encoding to both GetSizeOfEncodedValue and<br>
TargetLoweringObjectFile::getExprForDwarfReference, and I didn't want<br>
to start messing with piles of infrastructure. AFAICT this EH_PE<br>
symbol is an internal LLVM invention, not something specified by DWARF<br>
itself. So I just went with it.<br>
<br>
I wonder if I wouldn't be better off using EmitLabelPlusOffset(...., 0)<br>
even though it seems a little silly. I suppose I could try to conjure up<br>
a new EmitLabelReference instead.<br>
<br>
> Feel free to rename to DwarfUsesRelocationsAcrossSections. Thanks!<br>
><br>
><br>
> Nick<br>
<br>
Okay, will do. With any luck I'll have a revised patch later today.<br>
Thanks,<br>
--paulr<br>
</div></div></blockquote></div><br></font></div>