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

[George Rimar via llvm-commits]

grimar 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 =
----------------
ruiu wrote:
> I'd give names to both pair elements.
> 
>   SymbolBody *E = I.first;
>   unsigned RelOff = I.second;
Done.

================
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();
----------------
ruiu wrote:
> remove const as it is obvious.
Well, I removed. But I just saw the same in 

```
template <class ELFT> void RelocationSection<ELFT>::writeTo(uint8_t *Buf) {
  const unsigned EntrySize = IsRela ? sizeof(Elf_Rela) : sizeof(Elf_Rel);
```

================
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);
----------------
ruiu wrote:
> 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?
Done.


http://reviews.llvm.org/D14955