<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 16, 2015 at 11:18 AM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Jun-16, at 11:12, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Jun 16, 2015 at 11:01 AM, Frédéric Riss <<a href="mailto:friss@apple.com">friss@apple.com</a>> wrote:<br>
><br>
>> On Jun 16, 2015, at 10:54 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>><br>
>><br>
>> On Tue, Jun 16, 2015 at 10:43 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> > On 2015-Jun-11, at 12:26, Frédéric Riss <<a href="mailto:friss@apple.com">friss@apple.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> >> On Jun 11, 2015, at 12:19 PM, Rafael Espíndola <<a href="mailto:rafael.espindola@gmail.com">rafael.espindola@gmail.com</a>> wrote:<br>
>> >><br>
>> >>> 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?<br>
>> >><br>
>> >> The elf linker needs a relocation if one section refers to another. If<br>
>> >> two symbols are in different sections, then they are in different<br>
>> >> fragments.<br>
>> >><br>
>> >> So the case of dwarf_str, only if the two symbols are in dwarf_str<br>
>> >> would this function avoid creating the expression.<br>
>> ><br>
>> > 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 :-)<br>
>><br>
>> What a mess. These functions' assumptions are pretty convoluted. I<br>
>> just spent some time trying to follow this as well (a little behind on<br>
>> email). I'm not quite convinced this patch is right.<br>
>><br>
>> I hadn't realized that (almost!) all the logic to use<br>
>> emitLabelDifference() from section starts was Darwin-specific. With<br>
>> that new knowledge, here are a few useful assumptions we can make in<br>
>> emitLabelDifference():<br>
>><br>
>> 1. It's only used when emitting DWARF. (I have an out-of-tree patch<br>
>> to rename it to emitDwarfLabelDifference()... I'll commit that soon<br>
>> I hope.)<br>
>> 2. emitSectionOffset(), which calls into this function only on Mach-O,<br>
>> is only used to describe DWARF sections. (The same patch fixes its<br>
>> name.)<br>
>> 3. On ELF, sections can't be split. Since emitLabelDifference() is<br>
>> only used between two symbols within a section (caveat below for<br>
>> split-DWARF!!), we can safely emit it absolutely.<br>
>> 4. On Mach-O, DWARF isn't relocatable: the DWARF is always grabbed<br>
>> from the original object file. The target symbols are always<br>
>> either (1) DWARF, which won't move, or (2) some offset of an atom,<br>
>> and atoms can't be split. Safe here too.<br>
>><br>
>> So aside from my caveat, and assuming COFF somehow falls in the ELF<br>
>> bucket (or doesn't use DWARF), this seems good.<br>
>><br>
>> The one case I can't quite prove to myself is "okay" is split-DWARF.<br>
>> There's a bunch of logic that calls `emitLabelDifference()` instead of<br>
>> `emitSectionOffset()` on split-DWARF/DWO. `DIELocList::EmitValue()`<br>
>> is one suspicious example, since it's emitting a symbol offset into<br>
>> the TEXT section:<br>
>><br>
>> Wouldn't this be emitting a relocation into the debug_loc section, not the text section?<br>
><br>
> 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.<br>
><br>
> Sorry, perhaps an issue with terminology.<br>
><br>
> 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.<br>
<br>
</div></div>You're right, I read this wrong. I thought this was DIELoc, which<br>
*does* refer to the TEXT section. (Sad that I didn't even read the<br>
code I copy/pasted in.)<br>
<span class=""><br>
><br>
> & 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)<br>
><br>
> 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.<br>
<br>
</span>Okay, cool. I'll scan the rest of the red flags I found and confirm<br>
that they're all referring to offsets from DWARF sections (I think they<br>
are), in which case this patch would be fine by me. I'll report back<br>
otherwise.<br>
<br>
I'll try to add some header docs to explain all of this when I change<br>
the function names.<br></blockquote><div><br>*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))<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
>> void DIELocList::EmitValue(const AsmPrinter *AP, dwarf::Form Form) const {<br>
>> DwarfDebug *DD = AP->getDwarfDebug();<br>
>> MCSymbol *Label = DD->getDebugLocs().getList(Index).Label;<br>
>><br>
>> if (AP->MAI->doesDwarfUseRelocationsAcrossSections() && !DD->useSplitDwarf())<br>
>> AP->emitSectionOffset(Label);<br>
>> else<br>
>> AP->EmitLabelDifference(Label, Label->getSection().getBeginSymbol(), 4);<br>
>> }<br>
>><br>
>> I *think* this case is alright, but only because, currently, every<br>
>> instruction happens to be in a separate RelaxableFragment, so<br>
>> emitAbsoluteLabelDifference() is going to "fail". What I don't like<br>
>> is that whoever breaks that assumption is going to have no idea that<br>
>> the correctness of "emitAbsoluteLabelDifference()" depends on this.<br>
>><br>
>> To rephrase, emitAbsoluteLabelDifference() used to only be called<br>
>> when it was correct to emit an absolute difference -- if it "failed",<br>
>> it was only a missed optimization. It looks to me like we're now<br>
>> *depending* on it "failing" for split-DWARF.<br>
>><br>
>> Or am I wrong that we're depending on that? Does ELF not require<br>
>> relocations here?<br>
>><br>
>> (There are a couple of other calls to emitLabelDifference() that come<br>
>> from split-DWARF/DWO logic. I didn't look at them as deeply, but I<br>
>> imagine they could have similar problems?)<br>
>><br>
><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>