[llvm] r239552 - Generalize emitAbsoluteSymbolDiff.

[David Blaikie]

On Tue, Jun 16, 2015 at 11:18 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2015-Jun-16, at 11:12, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Tue, Jun 16, 2015 at 11:01 AM, Frédéric Riss <friss at apple.com> wrote:
> >
> >> On Jun 16, 2015, at 10:54 AM, David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >>
> >>
> >> On Tue, Jun 16, 2015 at 10:43 AM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >>
> >> > On 2015-Jun-11, at 12:26, Frédéric Riss <friss at apple.com> wrote:
> >> >
> >> >
> >> >> On Jun 11, 2015, at 12:19 PM, Rafael Espíndola <
> rafael.espindola at gmail.com> wrote:
> >> >>
> >> >>> When Duncan was working on this I was wondering if it couldn’t be
> generalized. I thought the ELF linkers relied on the relocations this would
> generate eg. for strings in the dwarf_str section. I guess all the strings
> will be in the same fragment. Without relocations, how can a standard ELF
> linker rewrite the references to the dwarf_str section when combining
> multiple input files?
> >> >>
> >> >> The elf linker needs a relocation if one section refers to another.
> If
> >> >> two symbols are in different sections, then they are in different
> >> >> fragments.
> >> >>
> >> >> So the case of dwarf_str, only if the two symbols are in dwarf_str
> >> >> would this function avoid creating the expression.
> >> >
> >> > Sorry for the noise. I thought the logic that did a difference
> between the string symbol and the start of section symbol was common to all
> platforms. I see that ELF will emits directly the string symbol which makes
> it totally irrelevant to this patch :-)
> >>
> >> What a mess.  These functions' assumptions are pretty convoluted.  I
> >> just spent some time trying to follow this as well (a little behind on
> >> email).  I'm not quite convinced this patch is right.
> >>
> >> I hadn't realized that (almost!) all the logic to use
> >> emitLabelDifference() from section starts was Darwin-specific.  With
> >> that new knowledge, here are a few useful assumptions we can make in
> >> emitLabelDifference():
> >>
> >>  1. It's only used when emitting DWARF.  (I have an out-of-tree patch
> >>     to rename it to emitDwarfLabelDifference()... I'll commit that soon
> >>     I hope.)
> >>  2. emitSectionOffset(), which calls into this function only on Mach-O,
> >>     is only used to describe DWARF sections.  (The same patch fixes its
> >>     name.)
> >>  3. On ELF, sections can't be split.  Since emitLabelDifference() is
> >>     only used between two symbols within a section (caveat below for
> >>     split-DWARF!!), we can safely emit it absolutely.
> >>  4. On Mach-O, DWARF isn't relocatable: the DWARF is always grabbed
> >>     from the original object file.  The target symbols are always
> >>     either (1) DWARF, which won't move, or (2) some offset of an atom,
> >>     and atoms can't be split.  Safe here too.
> >>
> >> So aside from my caveat, and assuming COFF somehow falls in the ELF
> >> bucket (or doesn't use DWARF), this seems good.
> >>
> >> The one case I can't quite prove to myself is "okay" is split-DWARF.
> >> There's a bunch of logic that calls `emitLabelDifference()` instead of
> >> `emitSectionOffset()` on split-DWARF/DWO.  `DIELocList::EmitValue()`
> >> is one suspicious example, since it's emitting a symbol offset into
> >> the TEXT section:
> >>
> >> Wouldn't this be emitting a relocation into the debug_loc section, not
> the text section?
> >
> > Yes, I think that’s what Duncan meant. It’s emitting a relocation into
> the debug_loc section that refers to symbols from the .text section.
> >
> > Sorry, perhaps an issue with terminology.
> >
> > This relocation will be placed in the debug_info section, and will refer
> to the debug_loc section, right? Neither of these is the text section.
>
> You're right, I read this wrong.  I thought this was DIELoc, which
> *does* refer to the TEXT section.  (Sad that I didn't even read the
> code I copy/pasted in.)
>
> >
> > & in fission, this would be the debug_info.dwo section containing
> relocations that refer to the debug_loc.dwo section (which is why it
> doesn't need relocations, just absolute offsets - because .dwo linking is
> handled by a debug-aware tool, so we don't emit any relocations into the
> .dwo file)
> >
> > And —with the new logic — if at some point the MCSymbols that generate
> this relocation end up in the same fragment, then this relocation is gone
> too.
>
> Okay, cool.  I'll scan the rest of the red flags I found and confirm
> that they're all referring to offsets from DWARF sections (I think they
> are), in which case this patch would be fine by me.  I'll report back
> otherwise.
>
> I'll try to add some header docs to explain all of this when I change
> the function names.
>

*nod* (also why the previously discussed assertions would be helpful, given
how mysterious this is when you get it wrong (& actually emitting
relocations into the .dwo sections doesn't break anything - it just makes
the files bigger for no reason, so it's even nicer to have assertions
there))


>
> >>     void DIELocList::EmitValue(const AsmPrinter *AP, dwarf::Form Form)
> const {
> >>       DwarfDebug *DD = AP->getDwarfDebug();
> >>       MCSymbol *Label = DD->getDebugLocs().getList(Index).Label;
> >>
> >>       if (AP->MAI->doesDwarfUseRelocationsAcrossSections() &&
> !DD->useSplitDwarf())
> >>         AP->emitSectionOffset(Label);
> >>       else
> >>         AP->EmitLabelDifference(Label,
> Label->getSection().getBeginSymbol(), 4);
> >>     }
> >>
> >> I *think* this case is alright, but only because, currently, every
> >> instruction happens to be in a separate RelaxableFragment, so
> >> emitAbsoluteLabelDifference() is going to "fail".  What I don't like
> >> is that whoever breaks that assumption is going to have no idea that
> >> the correctness of "emitAbsoluteLabelDifference()" depends on this.
> >>
> >> To rephrase, emitAbsoluteLabelDifference() used to only be called
> >> when it was correct to emit an absolute difference -- if it "failed",
> >> it was only a missed optimization.  It looks to me like we're now
> >> *depending* on it "failing" for split-DWARF.
> >>
> >> Or am I wrong that we're depending on that?  Does ELF not require
> >> relocations here?
> >>
> >> (There are a couple of other calls to emitLabelDifference() that come
> >> from split-DWARF/DWO logic.  I didn't look at them as deeply, but I
> >> imagine they could have similar problems?)
> >>
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150616/7a1a51f6/attachment.html>