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

[David Blaikie]

On Tue, Oct 22, 2013 at 4:25 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> On Tue, Oct 22, 2013 at 4:18 PM, Eric Christopher <echristo at gmail.com>wrote:
>
>> 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.)
>>
>
> Yep - they weren't intended to be final due to the possibility that it
> would be preferred to actually keep the links around rather than playing
> substring tricks to find linked sections.
>
> If everyone's OK keeping the substring tricks and just adding the group
> name (where there's a group) to the lookup then I'll tidy those up a bit
> and commit. (see my original email with some more thoughts on that issue -
> if we kept the linked sections as actual section pointers rather than
> deriving them by name, we'd avoid extra lookups in the section maps, for
> example)
>

Committed as r193209.

If we want to switch to having sections reference other sections with
pointers rather than name derivation via string manipulation, I'd be happy
to do that instead.

Thanks for the feedback/reviews/thoughts,

- David


>
>
> - David
>
>
>>
>> -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
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131022/69236093/attachment.html>