[PATCH] D16324: [ELF][MIPS] Initial support of MIPS local GOT entries

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:125
@@ +124,3 @@
+GotSection<ELFT>::getMipsLocalPageAddr(uintX_t EntryValue) {
+  return getMipsLocalEntryAddr((EntryValue + 0x8000) & ~0xffff);
+}
----------------
atanasyan wrote:
> ruiu wrote:
> > What is 0x8000?
> In fact this is a %hi() expression without right-shifting. Like `mipsHigh` or `applyPPCHa()`.
Please add a comment to describe that.

================
Comment at: ELF/OutputSections.cpp:154-156
@@ -127,4 +153,5 @@
 template <class ELFT> void GotSection<ELFT>::finalize() {
   this->Header.sh_size =
-      (Target->getGotHeaderEntriesNum() + Entries.size()) * sizeof(uintX_t);
+      (Target->getGotHeaderEntriesNum() + MipsLocalEntries + Entries.size()) *
+      sizeof(uintX_t);
 }
----------------
atanasyan wrote:
> ruiu wrote:
> > This code seems a bit tricky, and I also don't like the idea to look up a table for MIPS local symbols.
> > 
> > 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.
> It is unclear how to deal with local symbols which does not have a corresponding `SymbolBody`.
> 
> 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.
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D16324