[llvm-commits] [PATCH] PR9493, Fix for DW_AT_location

[Nick Lewycky]

On 21 June 2012 11:22, Robinson, Paul <Paul.Robinson at am.sony.com> wrote:

> I don't have commit access, could you commit for me please?
> I think it's straightforward enough that post-commit from Eric would
> be fine.
>

Done, thanks!!

Nick


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