[PATCH] D14955: [ELF] - Lazy relocations support for x86 target.

[Rui Ueyama via llvm-commits]

ruiu added inline comments.

================
Comment at: ELF/OutputSections.cpp:154
@@ +153,3 @@
+  for (std::pair<const SymbolBody *, size_t> &I : Entries) {
+    const SymbolBody *E = I.first;
+    uint64_t GotVA =
----------------
I'd give names to both pair elements.

  SymbolBody *E = I.first;
  unsigned RelOff = I.second;

================
Comment at: ELF/OutputSections.cpp:288
@@ +287,3 @@
+template <class ELFT> unsigned RelocationSection<ELFT>::getRelocOffset() {
+  const unsigned EntrySize = IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel);
+  return EntrySize * Relocs.size();
----------------
remove const as it is obvious.

================
Comment at: ELF/Writer.cpp:248-250
@@ -247,2 +247,5 @@
           continue;
-        Out<ELFT>::Plt->addEntry(Body);
+        unsigned RelOff = Target->supportsLazyRelocations()
+                              ? Out<ELFT>::RelaPlt->getRelocOffset()
+                              : Out<ELFT>::RelaDyn->getRelocOffset();
+        Out<ELFT>::Plt->addEntry(Body, RelOff);
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > This piece of code does not depends on any local context of Writer.cpp, so I think you can move this into a function that handles i386 PLT entries instead of propagating this unsigned value all the way to the consumer.
> > > What I need is a accordance of PLT entry and the offset in relocation table for its relocation. **It can be calculated only here I think.**
> > > 
> > > That its only needed for x86 now, thats true. But what is the way to move it from here ? Function that handles i386 PLT entries is 
> > > 
> > > ```
> > > writePltEntry(uint8_t *Buf, uint64_t GotAddr, uint64_t GotEntryAddr, uint64_t PltEntryAddr, int32_t Index, unsigned RelOff).
> > > ```
> > > It does not know anything about conformity of relocations and PLT entries.
> > > I added RelOff for it since I dont know the easier/shorter way to do what is needed.
> > I think I don't understand what you mean. What I wanted to say is since no local variables are used in the following expression, you can move this code to somewhere else.
> > 
> >   unsigned RelOff = Target->supportsLazyRelocations()
> >                         ? Out<ELFT>::RelaPlt->getRelocOffset()
> >                         : Out<ELFT>::RelaDyn->getRelocOffset();
> I understood what you want.
> But I think it is only can be moved to 
> ```
> template <class ELFT> void PltSection<ELFT>::addEntry(SymbolBody *Sym)
> ```
> from here. It is anyways common code and not just i386 specific.
> 
> Point is that getRelocOffset() returns current offset in relocation table, so it needs to be remembered for each added Plt entry. Because 
> each Out<ELFT>::RelaPlt->addReloc({&C, &RI}); call below changes it.
Yeah, so why don't you move this into addEntry?


http://reviews.llvm.org/D14955