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

Igor Kudrin via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 9 03:07:33 PST 2015


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
----------------
ikudrin wrote:
> 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.
See the comment in the writeTo() method.

================
Comment at: ELF/OutputSections.cpp:659
@@ +658,3 @@
+    WriteVal(DT_MIPS_SYMTABNO, Out<ELFT>::DynSymTab->getNumSymbols());
+    if (const SymbolBody *B = Out<ELFT>::Got->getFirstEntry()) {
+      WriteVal(DT_MIPS_LOCAL_GOTNO, B->GotIndex);
----------------
atanasyan wrote:
> ikudrin wrote:
> > atanasyan wrote:
> > > What is the case covered by this `if` statement?
> > We can have global entries in the GOT or it may consist of reserved and local entries only. In the later case, we'll have to change the expression "Target->getGotHeaderEntriesNum()" to something more reasonable when local GOT entries are added.
> Maybe it is better at first to complete GOT implementation and add local entries support and then add dynamic entries support.
I'm afraid in that case the patch will be postponed for a while. Right now, I don't have time to implement local GOT entries.


http://reviews.llvm.org/D14450





More information about the llvm-commits mailing list