[llvm] r239552 - Generalize emitAbsoluteSymbolDiff.

[Frédéric Riss]

> On Jun 16, 2015, at 11:12 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Jun 16, 2015 at 11:01 AM, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
> 
>> On Jun 16, 2015, at 10:54 AM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Tue, Jun 16, 2015 at 10:43 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>> 
>> > On 2015-Jun-11, at 12:26, Frédéric Riss <friss at apple.com <mailto:friss at apple.com>> wrote:
>> >
>> >
>> >> On Jun 11, 2015, at 12:19 PM, Rafael Espíndola <rafael.espindola at gmail.com <mailto: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.

Doh you’re right. Duncan’s mail resonated with what I initially thought could be an issue and this made me jump to conclusions.

Fred

> & 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.
> 
> Fred
> 
>>     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/9e00b5b0/attachment.html>