[ELF][MIPS] Fix primary GOT sometimes overflowing by one or two words

[jrtc27]

Summary

If we fail to merge a secondary GOT with the primary GOT but so far only
one merged GOT has been created (the primary one), the final element in
MergedGots is the primary GOT. Thus we should not try to merge with this
final element passing IsPrimary=false, since this will ignore the fact
that the destination GOT does in fact need a header, and those extra two
entries can be enough to allow the merge to incorrectly occur. Instead
we should check for this case before attempting the second merge.

[atanasyan]

LGTM. Thanks.

[atanasyan]

BTW it would be nice if you add a test case

[jrtc27]

Added regression test

[jrtc27]

Removed redundant check prefixes from an earlier revision of the test case

[atanasyan]

Do you have commit access?

[jrtc27]

In D49422#1171814, @atanasyan wrote:

Do you have commit access?

No, I don't, so please do so on my behalf if it's ready to land.

[atanasyan]

Thanks for the patch.

ELF/SyntheticSections.cpp

template <class ELFT> void MipsGotSection::build() {
   // the primary GOT can be accessed in the most effective way. If it
   // is not possible, try to fill the last GOT in the list, and finally
   // create a new GOT if both attempts failed.
   for (FileGot &SrcGot : Gots) {
     InputFile *File = SrcGot.File;
     if (tryMergeGots(MergedGots.front(), SrcGot, true)) {
       File->MipsGotIndex = 0;
     } else {
-      if (!tryMergeGots(MergedGots.back(), SrcGot, false)) {
+      // If this is the first time we failed to merge with the primary GOT,
+      // MergedGots.back() will also be the primary GOT. We must make sure not
+      // to try to merge again with IsPrimary=false, as otherwise, if the
+      // inputs are just right, we could allow the primary GOT to become 1 or 2
+      // words too big due to ignoring the header size.
+      if (MergedGots.size() == 1 ||
+          !tryMergeGots(MergedGots.back(), SrcGot, false)) {
         MergedGots.emplace_back();
         std::swap(MergedGots.back(), SrcGot);
       }
       File->MipsGotIndex = MergedGots.size() - 1;
     }
   }
   std::swap(Gots, MergedGots);
 

test/ELF/Inputs/mips-64-got-load.s

+  .text
+  .global foo1
+foo1:
+  ld $2, %got_disp(local1)($gp)
+
+  .bss
+local1:
+  .word 0

test/ELF/mips-64-got-overflow.s

+# REQUIRES: mips
+# Check the primary GOT cannot be made to overflow
+
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux \
+# RUN:         %p/Inputs/mips-64-got-load.s -o %t1.so.o
+# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-linux %s -o %t2.so.o
+# RUN: ld.lld -shared -mips-got-size 32 %t1.so.o %t2.so.o -o %t-sgot.so
+# RUN: ld.lld -shared -mips-got-size 24 %t1.so.o %t2.so.o -o %t-mgot.so
+# RUN: llvm-readobj -r -dt -mips-plt-got %t-sgot.so | FileCheck -check-prefixes=GOT,SGOT %s
+# RUN: llvm-readobj -r -dt -mips-plt-got %t-mgot.so | FileCheck -check-prefixes=GOT,MGOT %s
+
+# SGOT:      Primary GOT {
+# SGOT-NEXT:   Canonical gp value: 0x27FF0
+# SGOT-NEXT:   Reserved entries [
+# SGOT-NEXT:     Entry {
+# SGOT-NEXT:       Address:
+# SGOT-NEXT:       Access: -32752
+# SGOT-NEXT:       Initial: 0x0
+# SGOT-NEXT:       Purpose: Lazy resolver
+# SGOT-NEXT:     }
+# SGOT-NEXT:     Entry {
+# SGOT-NEXT:       Address:
+# SGOT-NEXT:       Access: -32744
+# SGOT-NEXT:       Initial: 0x80000000
+# SGOT-NEXT:       Purpose: Module pointer (GNU extension)
+# SGOT-NEXT:     }
+# SGOT-NEXT:   ]
+# SGOT-NEXT:   Local entries [
+# SGOT-NEXT:     Entry {
+# SGOT-NEXT:       Address:
+# SGOT-NEXT:       Access: -32736
+# SGOT-NEXT:       Initial: 0x20020
+# SGOT-NEXT:     }
+# SGOT-NEXT:     Entry {
+# SGOT-NEXT:       Address:
+# SGOT-NEXT:       Access: -32728
+# SGOT-NEXT:       Initial: 0x20030
+# SGOT-NEXT:     }
+# SGOT-NEXT:   ]
+# SGOT-NEXT:   Global entries [
+# SGOT-NEXT:   ]
+# SGOT-NEXT:   Number of TLS and multi-GOT entries: 0
+# SGOT-NEXT: }
+
+# MGOT:      Primary GOT {
+# MGOT-NEXT:   Canonical gp value: 0x27FF0
+# MGOT-NEXT:   Reserved entries [
+# MGOT-NEXT:     Entry {
+# MGOT-NEXT:       Address:
+# MGOT-NEXT:       Access: -32752
+# MGOT-NEXT:       Initial: 0x0
+# MGOT-NEXT:       Purpose: Lazy resolver
+# MGOT-NEXT:     }
+# MGOT-NEXT:     Entry {
+# MGOT-NEXT:       Address:
+# MGOT-NEXT:       Access: -32744
+# MGOT-NEXT:       Initial: 0x80000000
+# MGOT-NEXT:       Purpose: Module pointer (GNU extension)
+# MGOT-NEXT:     }
+# MGOT-NEXT:   ]
+# MGOT-NEXT:   Local entries [
+# MGOT-NEXT:     Entry {
+# MGOT-NEXT:       Address:
+# MGOT-NEXT:       Access: -32736
+# MGOT-NEXT:       Initial: 0x20020
+# MGOT-NEXT:     }
+# MGOT-NEXT:   ]
+# MGOT-NEXT:   Global entries [
+# MGOT-NEXT:   ]
+# MGOT-NEXT:   Number of TLS and multi-GOT entries: 1
+# MGOT-NEXT: }
+
+  .text
+  .global foo2
+foo2:
+  ld $2, %got_disp(local2)($gp)
+
+  .bss
+local2:
+  .word 0