[PATCH] D14450: [ELF2] Add mandatory .dynamic section entries on MIPS.

[Igor Kudrin via llvm-commits]

ikudrin added inline comments.

================
Comment at: ELF/OutputSections.cpp:491
@@ -480,3 +490,3 @@
     ++NumEntries; // DT_PLTRELSZ
-    ++NumEntries; // DT_PLTGOT
+    ++NumEntries; // DT_PLTGOT or DT_MIPS_PLTGOT
     ++NumEntries; // DT_PLTREL
----------------
atanasyan wrote:
> ikudrin wrote:
> > atanasyan wrote:
> > > ikudrin wrote:
> > > > atanasyan wrote:
> > > > > ikudrin wrote:
> > > > > > atanasyan wrote:
> > > > > > > Do you read MIPS ABI? DT_MIPS_PLTGOT is used in the non-PIC MIPS code only. PIC MIPS code uses DT_PLTGOT entry but this entry points to the .got section.
> > > > > > The logic of this line: if we have .plt.got section, then store its address to DT_MIPS_PLTGOT entry. Is something wrong here?
> > > > > > Please note, the DT_PLTGOT entry for MIPS target is mentioned a bit later in this function.
> > > > > If MIPS dynamic binary has a .got section, DT_PLTGOT entry should point to it. The DT_MIPS_PLTGOT entry should point to the .got.plt entry if MIPS dynamic binary is non-PIC.
> > > > > 
> > > > > https://sourceware.org/ml/binutils/2008-07/txt00000.txt
> > > > Moreover, the logic here is the same as in this patch for the old lld: http://reviews.llvm.org/rL200630.
> > > No, old lld works correctly at least in that case. Do you use any MIPS toolchain as a reference implementation?
> > > ```
> > > $ cat test.c
> > > void foo() {}
> > > 
> > > $ mips-linux-gnu-gcc -fPIC test.c -shared
> > > $ readelf -a a.out
> > > ...
> > > [16] .got              PROGBITS        000107c0
> > > ...
> > > 0x00000003 (PLTGOT)                     0x107c0
> > > ```
> > I can't catch. This patch even includes the test which checks that DT_PLTGOT holds an address of .got section:
> > 
> > ```
> > # EXE:          Name: .got
> > . . .
> > # EXE-NEXT:     Address: [[GOTADDR:0x[0-9a-f]+]]
> > . . .
> > # EXE-DAG:    0x00000003 PLTGOT               [[GOTADDR]]
> > ```
> Oh, my bad. Handling of dynamic entries are spread among the code. I did not catch that you handle both `DT_PLTGOT` and `DT_MIPS_PLTGOT` entries. You are right.
OK, I'll add a comment to explain it.

================
Comment at: ELF/OutputSections.h:121
@@ -120,2 +120,3 @@
   uintX_t getEntryAddr(const SymbolBody &B) const;
+  const SymbolBody *getFirstEntry() const;
 
----------------
atanasyan wrote:
> ikudrin wrote:
> > atanasyan wrote:
> > > ikudrin wrote:
> > > > atanasyan wrote:
> > > > > What is the meaning of the "First" here: Lazy resolver, first local entry of first global entry?
> > > > For MIPS, it's the first global entry, because we don't have SymbolBody for anything else.
> > > > For all other targets it's just the first entry of the GOT, although nobody is interested in it.
> > > In that case "First" might confuse. Because you have to read code to realize that the "First" means first global but sometimes just first entry. If you ask somebody familiar with MIPS what is the first GOT entry he or she answers that it is either "Lazy resolver" entry or "first local entry"
> > Let's rename it to something like getFirstGlobal() maybe?
> It is okay to me. Is it acceptable for other targets?
I think it over till Monday.


http://reviews.llvm.org/D14450