<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jan 20, 2016 at 3:15 AM, Simon Atanasyan <span dir="ltr"><<a href="mailto:simon@atanasyan.com" target="_blank">simon@atanasyan.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">atanasyan marked an inline comment as done.<br>
<span class=""><br>
================<br>
Comment at: ELF/OutputSections.cpp:154-156<br>
@@ -127,4 +153,5 @@<br>
 template <class ELFT> void GotSection<ELFT>::finalize() {<br>
   this->Header.sh_size =<br>
-      (Target->getGotHeaderEntriesNum() + Entries.size()) * sizeof(uintX_t);<br>
+      (Target->getGotHeaderEntriesNum() + MipsLocalEntries + Entries.size()) *<br>
+      sizeof(uintX_t);<br>
 }<br>
----------------<br>
</span><span class="">ruiu wrote:<br>
> atanasyan wrote:<br>
> > ruiu wrote:<br>
> > > This code seems a bit tricky, and I also don't like the idea to look up a table for MIPS local symbols.<br>
> > ><br>
> > > How about this? We don't need to assign an index in the GOT to Sym->GotIndex() in addEntry. Instead, we can do that in GotSection::finalize(). We can also change addMipsLocalEntry(SymbolBody *) to just add a given SymbolBody to some vector (say, MipsEntries). In finalize(). we assign GotIndex to all symbols in MipsEntries first and then to all symbols in Entries.<br>
> > It is unclear how to deal with local symbols which does not have a corresponding `SymbolBody`.<br>
> ><br>
> > Now `MipsLocalEntries` holds number of entries reserved for local GOT entries. Some of these entries are for non-local symbols with `SymbolBody` and we can keep them in `MipsEntries`. But other ones are for local symbols, i.e. `section + offset` in fact.<br>
> Ah, I often forgot the fact that we did not create SymbolBody for a local symbol. This is probably a bold suggestion, but for the sake of discussion, what if we create SymbolBodies for local symbols? How much will it increase link time (I'm not requesting to do that in this patch, though)? That would increase link time, but if it is small, then that may make sense because that would simplify some code a lot.<br>
</span>It is a good idea but let's postpone it a bit. I'm going to optimize local GOT entries allocation to reduce number of such entries. To do that I will need to handle local symbols in a more complicated way than just counting GOT16 relocations against them. Maybe in that case SymbolBody for local symbol becomes mandatory.</blockquote><div><br></div><div>I spent a few hours today to experiment that idea to create SymbolBodies for local symbols. Looks like we can do that with a few percent increase in link time, and that would make code much more readable. That's a fair trade-off. So, I'll take care of this part of code complexity.</div><div><br></div><div>It will take more time to ready for code review, though.</div></div></div></div>