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

[Robinson, Paul]

I certainly _meant_ to click reply-all.....

________________________________________
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