[llvm] r239552 - Generalize emitAbsoluteSymbolDiff.

[Duncan P. N. Exon Smith]

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

>>     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?)
>> 
> 
>