[llvm] r185389 - PR16493: DebugInfo with TLS on PPC crashing due to invalid relocation

[David Blaikie]

On Tue, Jul 2, 2013 at 5:08 AM, Ulrich Weigand
<Ulrich.Weigand at de.ibm.com> wrote:
> David Blaikie <dblaikie at gmail.com> wrote on 02.07.2013 02:28:08:
>
>> On Mon, Jul 1, 2013 at 5:23 PM, Ulrich Weigand
>> <Ulrich.Weigand at de.ibm.com> wrote:
>> > Could this be changed to a full MCExpr, or at least allow
>> > symbol + offset ?
>>
>> I don't see any reason this wouldn't be reasonable - you'll have to
>> plumb it through similar to how I have done so already, including for
>> fission (-gsplit-dwarf, see my recent commit to add support for this),
>> but it should be fairly obvious/mechanical.
>
> OK, so I've changed the return value of getDebugThreadLocalSymbol to
> be a MCExpr.  This flows into either a DIELabel or an AddrPool.
> The latter can simply be likewise changed from holding MCSymbolRefExpr
> to holding full MCExpr, but the former is a bit of a problem,
> due to code DwarfDebug::emitDIE that explicitly expects labels:
>
>     case dwarf::DW_AT_location: {
>       if (DIELabel *L = dyn_cast<DIELabel>(Values[i])) {
>         if (Asm->MAI->doesDwarfUseRelocationsAcrossSections())
>           Asm->EmitLabelReference(&L->getValue()->getSymbol(), 4);
>         else
>           Asm->EmitLabelDifference(&L->getValue()->getSymbol(),
> DwarfDebugLocSectionSym, 4);
>       } else {
>         Values[i]->EmitValue(Asm, Form);
>       }
>       break;
>     }
>
> Now I guess the special handling does make sense if we're
> dealing with a label refering to other places within
> debug info.  But when refering to code/data elements,
> I think allowing a full MCExpr is more useful.

Agreed - though this isn't an area I'm deeply familiar with.

> Thus I've split the current DIELabel into two variants:
> a DIELabel that now really only handles MCSymbol, and a
> new DIEExpr that handles arbitrary MCExpr expressions.

Sounds good - my previous changes to reuse DIELabel were a bit
opportunistic. Thanks for cleaning that up.

> With all those changes in place, I was able to add
> support for describing TLS variables on PowerPC in
> a straightforward manner.

Awesome.

> The attached patch hold all these changes.  Does this
> look reasonable?

Yep. Please commit - if you'd like, you could break this out into a
no-functionality-changing patch to add DIEExpr (& undo my changes to
DIELabel, as you have) then the change to generalize the
API/containers to MCExpr & add PPC TLS support. Not necessary if you
want to just do it in one go that's not a problem either.

(& please mention the PR in your commit)

- David