<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Oct 22, 2013 at 4:25 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">
<div class="im">On Tue, Oct 22, 2013 at 4:18 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Only real comment is:<br>
<br>
+ sh_link = SectionIndexMap.lookup(Asm.getContext().getELFSection(<br>
+ SecName.substr(sizeof(".ARM.exidx") - 1), ELF::SHT_PROGBITS,<br>
+ ELF::SHF_EXECINSTR | ELF::SHF_ALLOC, SectionKind::getText(), 0,<br>
+ Section.getGroup() ? Section.getGroup()->getName() : ""));<br>
<br>
that's a heck of a block of code. Might want to split it out into a<br>
few variable so it's more easily readable? (Ditto the other places.)<br></blockquote><div><br></div></div><div>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.<br>
<br>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)</div>
</div></div></div></blockquote><div><br></div><div>Committed as r193209.<br><br>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.<br>
<br>Thanks for the feedback/reviews/thoughts,<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><span class=""><font color="#888888"><br>
<br>- David</font></span></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span><font color="#888888"><br>
-eric<br>
</font></span><div><div><br>
On Tue, Oct 22, 2013 at 3:27 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Sat, Oct 19, 2013 at 7:36 AM, Rafael Espíndola<br>
> <<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>> wrote:<br>
>><br>
>> >> Can we use a DenseMap instead of a std::map?<br>
>> ><br>
>> ><br>
>> > Would require a bit more work - DenseMap can't handle a pair<string,<br>
>> > string><br>
>> > key (no tombstone value), not sure if there's something I can do to<br>
>> > address<br>
>> > that.<br>
>><br>
>> You would probably need to create dedicated class for the pair and<br>
>> implement densemapinfo for it. This is probably not too hot, lets keep<br>
>> the std::map for now and benchmark it once the rest of the patch is<br>
>> done.<br>
>><br>
>> >><br>
>> >> 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.<br>
>> ><br>
>> ><br>
>> > I tend to prefer avoiding adding new test files if possible (keeping<br>
>> > testing<br>
>> > related to the same feature together) to avoid excess process execution<br>
>> > overhead.<br>
>> ><br>
>> > It seems fairly easy to split out examples of different names and the<br>
>> > same<br>
>> > 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>
>> At least for now please add a second test. Object file dumps are<br>
>> somewhat unstable, which makes the test changes hard to read.<br>
><br>
><br>
> Attached an updated patch that has a separate test, removes the RuntimeDyLib<br>
> and another bit of noise in the change, and reverts the reloc section<br>
> ungrouping change.<br>
><br>
> Thanks,<br>
> - David<br>
><br>
>><br>
>><br>
>> A followup patch doing nothing but merging the tests would probably be OK.<br>
>><br>
>> Cheers,<br>
>> Rafael<br>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>