<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Oct 18, 2013 at 1:44 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.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="im">On 8 October 2013 14:50, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>

> Previous version produced some test failures (sorry I hadn't spotted those<br>
> before sending it out), mostly due to various bits of code in<br>
> ELFObjectWriter relying on simply morphing the name of a section (stripping<br>
> rel(a) or ARM.exidx, etc, prefixes) to find its linked section. I've added<br>
> some hackish code in the few places that were the source of test failures to<br>
> pass the section name as well, but that's pretty fragile and I haven't<br>
> attempted to look at all the places where we do this name morphing so<br>
> there's probably other untested broken places.<br>
><br>
> Should we change the MCSectionELF to store a pointer to its linked section<br>
> instead of trying to derive it from the name? Otherwise we could at least<br>
> add a convenience function to MCSection to retrieve the group name (or the<br>
> empty StringRef) and try to ensure we pass it in all the places where we do<br>
> such derivation.<br>
><br>
> Side note: the reason I'm not propagating the SHF_GROUP flag through when<br>
> creating rel(a) sections is that GCC doesn't, it was the only major<br>
> difference I could find when comparing output with my change to GCC's. I<br>
> don't have any reason to believe it's harmful to do so, except for the<br>
> wasted bytes - so we can leave that in if it's preferred/there's a reason to<br>
> do so. I haven't found any wording in the ELF64 or other specs to clearly<br>
> indicate which way that should go.<br>
<br>
</div>I think this was an intentional change: r186034.<br></blockquote><div><br></div><div>Fair enough - I'll see if I can find some justification from Tim or the original contributor as to why that's needed, but it's pretty much orthogonal to my change, just something I noticed while comparing the integrated assembler's output to gas's.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Why the change to lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp?<br></blockquote><div><br></div><div>Will revert - was just lying around/snuck in.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Can we use a DenseMap instead of a std::map?<br></blockquote><div><br></div><div>Would require a bit more work - DenseMap can't handle a pair<string, string> key (no tombstone value), not sure if there's something I can do to address that.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Can you add a new test and leave comdat.s unchanged? Would make it<br>
easier to see that the new code behaves the same in the case of<br>
distinct names.</blockquote><div><br></div><div>I tend to prefer avoiding adding new test files if possible (keeping testing related to the same feature together) to avoid excess process execution overhead.<br><br>It seems fairly easy to split out examples of different names and the same name so they're all tested, but tested separately, in the same file.<br>
<br>But I'm not wedded to the idea either. <br><br>- David</div></div></div></div>