[ELF] Fix DT_MIPS_LOCAL_GOTNO value when using linker scripts to change section sizes

[jhenderson]

Summary

The MIPS GOT section has a number of local entries based on the number of pages needed for output sections referenced by GOT page relocations. The number is recorded in the DT_MIPS_LOCAL_GOTNO dynamic section tag. However, the dynamic tag is added before assignAddresses has been called, meaning that any section size used to calculate the value will not include size modifications caused by linker scripts, thunks, etc.

This change moves the calculation of DT_MIPS_LOCAL_GOTNO until writeTo, by which time the output section sizes have been finalized.

I have spun it off from D38361, because the behaviour is already broken.

[jhenderson]

Ping.

Regardless of which of D38361 or D38321 is preferred, this should be fixed independently.

[atanasyan]

ELF/SyntheticSections.cpp

1189 ↗

(On Diff #121129)

Other targets might have dynamic table tags with the same value so we have to check Config->EMachine == EM_MIPS here.

[jhenderson]

Thanks @atanasyan. I've fixed that.

[ruiu]

ELF/SyntheticSections.cpp

1169 ↗

(On Diff #122217)

I think this kind of "if (MIPS)" should be avoided. Is there any way to not add that code here? For example, can't you finalize the GOT section before this section?

[jhenderson]

ELF/SyntheticSections.cpp

1169 ↗

(On Diff #122217)

For example, can't you finalize the GOT section before this section?

Unfortunately this example isn't possible: it's what currently happens and is why we have to use this trick. The problem is that the dynamic section size is determined by the tags in it. This size affects the overall allocatable size of the executable, which means that the tag has to be added either before the new assignAddresses loop, or during, and assigned a value either during or after the loop. Adding the tag during the loop is not great, since that would mean repopulating the dynamic section either completely, or in part during each iteration, so we need to add it before, and assign a value later.

I do have a few alternative ideas though that would move the if (MIPS) out of the writeTo code, if you prefer:

1. Add a different Entry kind specific for the MipsGot local entries count.
2. Implement postThunkContents for Dynamic sections, which simply finds this tag and updates it after the Thunk loop (this would still require an "if (MIPS)" but it would not be in the Write code).
3. Implement updateAllocSize for Dynamic sections, which updates the value of the existing tag. Again, it would be necessary to add an "if (MIPS)" here.
4. Add a new member to the DynamicSection that points to the DT_MIPS_LOCAL_GOTNO tag, and is set when the tag is initialised. The tag could then be updated during postThunkContents, updateAllocSize, or in writeTo, to remove the need for "if (MIPS)" in these places.

I'm happy to do do any of these changes, if you aren't happy with the current proposal, or if you've got another alternative, I can look at that too.

[grimar]

ELF/SyntheticSections.cpp

1169 ↗

(On Diff #122217)

I also thought of:

5. Add a different Entry kind that will be of type std::function<uint64()>.

So you would write:

add({DT_MIPS_LOCAL_GOTNO, [](){ return InX::MipsGot->getLocalEntriesNum(); });

That is not ideal probably, but does not require adding MIPS specific code at least.

[jhenderson]

Rafael posted an LGTM on the mailing list. I will commit this as is, so that it unblocks D38361 but I really like @grimar's suggestion, so I plan on posting another review based on it, which should hopefully resolve any concerns @ruiu still has.

[ruiu]

ELF/SyntheticSections.cpp

1169 ↗

(On Diff #122217)

Yes, that's what I was thinking too.

James, please do this after you commit this change so that we can remove if (MIPS).

[jhenderson]

ELF/SyntheticSections.cpp

1169 ↗

(On Diff #122217)

Will do, although I'll wait for D40338 to land, I think.

lld/trunk/ELF/SyntheticSections.cpp

if (HasVerNeed) {
     add({DT_VERNEEDNUM, In<ELFT>::VerNeed->getNeedNum()});
   }
 
   if (Config->EMachine == EM_MIPS) {
     add({DT_MIPS_RLD_VERSION, 1});
     add({DT_MIPS_FLAGS, RHF_NOTPOT});
     add({DT_MIPS_BASE_ADDRESS, Target->getImageBase()});
     add({DT_MIPS_SYMTABNO, InX::DynSymTab->getNumSymbols()});
-    add({DT_MIPS_LOCAL_GOTNO, InX::MipsGot->getLocalEntriesNum()});
+
+    // The number of local GOT entries has not yet been finalized. This value
+    // will be set in writeTo().
+    add({DT_MIPS_LOCAL_GOTNO, uint64_t(0)});
+
     if (const Symbol *B = InX::MipsGot->getFirstGlobalEntry())
       add({DT_MIPS_GOTSYM, B->DynsymIndex});
     else
       add({DT_MIPS_GOTSYM, InX::DynSymTab->getNumSymbols()});
     add({DT_PLTGOT, InX::MipsGot});
     if (InX::MipsRldMap)
       add({DT_MIPS_RLD_MAP, InX::MipsRldMap});
   }
case Entry::InSecAddr:
       break;
     case Entry::SecSize:
       P->d_un.d_val = E.OutSec->Size;
       break;
     case Entry::SymAddr:
       P->d_un.d_ptr = E.Sym->getVA();
       break;
     case Entry::PlainInt:
+      if (Config->EMachine == EM_MIPS && E.Tag == DT_MIPS_LOCAL_GOTNO)
+        P->d_un.d_val = InX::MipsGot->getLocalEntriesNum();
+      else
         P->d_un.d_val = E.Val;
       break;
     }
     ++P;
   }
 }
 
 uint64_t DynamicReloc::getOffset() const {
   return InputSec->getOutputSection()->Addr + InputSec->getOffset(OffsetInSec);

lld/trunk/test/ELF/mips-got-script.s

+# Check number of got entries is adjusted for linker script-added space.
+
+# RUN: llvm-mc -filetype=obj -triple=mips-unknown-linux %s -o %t.o
+# RUN: echo "SECTIONS { .data : { *(.data.1); . += 0x10000; *(.data.2) } }" > %t.script
+# RUN: ld.lld %t.o -shared -o %t.so -T %t.script
+# RUN: llvm-readobj -mips-plt-got -dynamic-table %t.so | FileCheck %s
+
+# REQUIRES: mips
+
+# CHECK: 0x7000000A MIPS_LOCAL_GOTNO 5
+#                                    ^-- 2 * header + 3 local entries
+# CHECK:      Local entries [
+# CHECK-NEXT:   Entry {
+# CHECK-NEXT:     Address:
+# CHECK-NEXT:     Access: -32744
+# CHECK-NEXT:     Initial: 0x0
+#                          ^-- loc1
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Entry {
+# CHECK-NEXT:     Address:
+# CHECK-NEXT:     Access: -32740
+# CHECK-NEXT:     Initial: 0x10000
+#                          ^-- loc2
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Entry {
+# CHECK-NEXT:     Address:
+# CHECK-NEXT:     Access: -32736
+# CHECK-NEXT:     Initial: 0x20000
+#                          ^-- redundant
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+  .text
+  .globl  foo
+foo:
+  lw      $t0, %got(loc1)($gp)
+  addi    $t0, $t0, %lo(loc1)
+  lw      $t0, %got(loc2)($gp)
+  addi    $t0, $t0, %lo(loc2)
+
+  .section .data.1,"aw",%progbits
+loc1:
+  .word 0
+
+  .section .data.2,"aw",%progbits
+loc2:
+  .word 0