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

[Eric Christopher]

Only real comment is:

+      sh_link = SectionIndexMap.lookup(Asm.getContext().getELFSection(
+          SecName.substr(sizeof(".ARM.exidx") - 1), ELF::SHT_PROGBITS,
+          ELF::SHF_EXECINSTR | ELF::SHF_ALLOC, SectionKind::getText(), 0,
+          Section.getGroup() ? Section.getGroup()->getName() : ""));

that's a heck of a block of code. Might want to split it out into a
few variable so it's more easily readable? (Ditto the other places.)

-eric

On Tue, Oct 22, 2013 at 3:27 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Sat, Oct 19, 2013 at 7:36 AM, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>>
>> >> 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.
>>
>> You would probably need to create dedicated class for the pair and
>> implement densemapinfo for it. This is probably not too hot, lets keep
>> the std::map for now and benchmark it once the rest of the patch is
>> done.
>>
>> >>
>> >> 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.
>>
>> At least for now please add a second test. Object file dumps are
>> somewhat unstable, which makes the test changes hard to read.
>
>
> Attached an updated patch that has a separate test, removes the RuntimeDyLib
> and another bit of noise in the change, and reverts the reloc section
> ungrouping change.
>
> Thanks,
> - David
>
>>
>>
>> A followup patch doing nothing but merging the tests would probably be OK.
>>
>> Cheers,
>> Rafael
>
>