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

Robinson, Paul Paul.Robinson at am.sony.com
Thu Jun 21 11:22:44 PDT 2012


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.
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




More information about the llvm-commits mailing list