[patch] Support multiple comdat sections with the same name but different groups

[David Blaikie]

On Fri, Oct 18, 2013 at 1:44 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> On 8 October 2013 14:50, David Blaikie <dblaikie at gmail.com> wrote:
> > Previous version produced some test failures (sorry I hadn't spotted
> those
> > before sending it out), mostly due to various bits of code in
> > ELFObjectWriter relying on simply morphing the name of a section
> (stripping
> > rel(a) or ARM.exidx, etc, prefixes) to find its linked section. I've
> added
> > some hackish code in the few places that were the source of test
> failures to
> > pass the section name as well, but that's pretty fragile and I haven't
> > attempted to look at all the places where we do this name morphing so
> > there's probably other untested broken places.
> >
> > Should we change the MCSectionELF to store a pointer to its linked
> section
> > instead of trying to derive it from the name? Otherwise we could at least
> > add a convenience function to MCSection to retrieve the group name (or
> the
> > empty StringRef) and try to ensure we pass it in all the places where we
> do
> > such derivation.
> >
> > Side note: the reason I'm not propagating the SHF_GROUP flag through when
> > creating rel(a) sections is that GCC doesn't, it was the only major
> > difference I could find when comparing output with my change to GCC's. I
> > don't have any reason to believe it's harmful to do so, except for the
> > wasted bytes - so we can leave that in if it's preferred/there's a
> reason to
> > do so. I haven't found any wording in the ELF64 or other specs to clearly
> > indicate which way that should go.
>
> I think this was an intentional change: r186034.
>

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.


> Why the change to lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp?
>

Will revert - was just lying around/snuck in.


> Can we use a DenseMap instead of a std::map?
>

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.


> Can you add a new test and leave comdat.s unchanged? Would make it
> easier to see that the new code behaves the same in the case of
> distinct names.


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.

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.

But I'm not wedded to the idea either.

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131018/4556fe60/attachment.html>