[ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct

[arichardson]

Summary

Older versions of BFD generate libraries with .MIPS.abiflags that only
concatenate the individual .MIPS.abiflags sections instead of merging.

[arichardson]

I'm not sure how to add a testcase for this. I guess somehow define the .MIPS.abiflags section in the .s file?

Here is what the .MIPS.abiflags section looks like for a binary created with an old BFD (it does the same for libraries)

Contents of section .MIPS.abiflags:
 1200bb9a8 00000400 02000003 00000000 00000000  ................
 1200bb9b8 00000001 00000000 00000400 02000003  ................
 1200bb9c8 00000000 00000000 00000001 00000000  ................
 1200bb9d8 00000400 02000003 00000000 00000000  ................
 1200bb9e8 00000001 00000000 00000400 02000003  ................
 1200bb9f8 00000000 00000000 00000001 00000000  ................
 1200bba08 00000400 02000003 00000000 00000000  ................
 1200bba18 00000001 00000000 00000400 02000003  ................
 1200bba28 00000000 00000000 00000001 00000000  ................
 1200bba38 00000400 02000003 00000000 00000000  ................
 1200bba48 00000001 00000000 00000400 02000003  ................
 1200bba58 00000000 00000000 00000001 00000000  ................
 1200bba68 00000400 02000003 00000000 00000000  ................
 1200bba78 00000001 00000000 00000400 02000003  ................
 1200bba88 00000000 00000000 00000001 00000000  ................
 1200bba98 00000400 02000003 00000000 00000000  ................
 1200bbaa8 00000001 00000000 00000400 02000003  ................
 1200bbab8 00000000 00000000 00000001 00000000  ................
 1200bbac8 00000400 02000003 00000000 00000000  ................
 1200bbad8 00000001 00000000 00000400 02000003  ................
 1200bbae8 00000000 00000000 00000001 00000000  ................
 1200bbaf8 00000400 02000003 00000000 00000000  ................
 1200bbb08 00000001 00000000 00000400 02000003  ................
 1200bbb18 00000000 00000000 00000001 00000000  ................
 1200bbb28 00000400 02000003 00000000 00000000  ................
 1200bbb38 00000001 00000000 00000400 02000003  ................
 1200bbb48 00000000 00000000 00000001 00000000  ................
 1200bbb58 00000400 02000003 00000000 00000000  ................
 1200bbb68 00000001 00000000 00000400 02000003  ................
 1200bbb78 00000000 00000000 00000001 00000000  ................
 1200bbb88 00000400 02000003 00000000 00000000  ................
 1200bbb98 00000001 00000000 00000400 02000003  ................
 1200bbba8 00000000 00000000 00000001 00000000  ................
 1200bbbb8 00000400 02000003 00000000 00000000  ................
 1200bbbc8 00000001 00000000 00000400 02000003  ................
 1200bbbd8 00000000 00000000 00000001 00000000  ................
 1200bbbe8 00000400 02000003 00000000 00000000  ................
 1200bbbf8 00000001 00000000 00000400 02000003  ................
 1200bbc08 00000000 00000000 00000001 00000000  ................
 1200bbc18 00000400 02000003 00000000 00000000  ................
 1200bbc28 00000001 00000000 00000400 02000003  ................
 1200bbc38 00000000 00000000 00000001 00000000  ................
 1200bbc48 00000400 02000003 00000000 00000000  ................
 1200bbc58 00000001 00000000 00000400 02000003  ................
 1200bbc68 00000000 00000000 00000001 00000000  ................
 1200bbc78 00000400 02000003 00000000 00000000  ................

.....

 1200bd038 00000001 00000000 00000400 02000003  ................
 1200bd048 00000000 00000000 00000001 00000000  ................
 1200bd058 00000400 02000003 00000000 00000000  ................
 1200bd068 00000001 00000000 00000400 02000003  ................
 1200bd078 00000000 00000000 00000001 00000000  ................
 1200bd088 00000400 02000003 00000000 00000000  ................
 1200bd098 00000001 00000000 00000400 02000003  ................
 1200bd0a8 00000000 00000000 00000001 00000000  ................
Contents of section .tdata:

[atanasyan]

In D27770#622659, @arichardson wrote:

I'm not sure how to add a testcase for this. I guess somehow define the .MIPS.abiflags section in the .s file?

In case of testing an "invalid input", you can put an object file into the test/ELF/Inputs folder and use this file in the test case. Take a look at the test/ELF/bad-archive.s for example.

[arichardson]

Added a test

[atanasyan]

ELF/SyntheticSections.cpp

138

I do not think we need constexpr here. Plain const is enough.

test/ELF/mips-merge-abiflags.s

6 ↗

(On Diff #81561)

1. I guess you want to check %t.exe (file produced by LLD) not mips-concatenated-abiflags.so here.
2. It is enough to use the -mips-abi-flags flag only and do not check ELF header and sections list.
3. Does LLD really show the "invalid size of .MIPS.abiflags section" error message if concatenated .MIPS.abiflags sections are in a DSO (not a regular object file)?

[arichardson]

test/ELF/mips-merge-abiflags.s

6 ↗

(On Diff #81561)

1. I wanted to check that the .so file generates "The .MIPS.abiflags section has a wrong size." and that .MIPS.abiflags size is 48. I guess I can remove the other checks like testing the ELF header
2. I also wanted to test the size of the .MIPS.abiflags, but I guess I can remove testing the header
3. Seems you are right it only happens with .o files that have multiple abiflags structs. Will update the test case. Should I remove the .so file? or also test that linking against such a file works?

[arichardson]

Updated test to check linking an .o file with concatenated .MIPS.abiflags

[atanasyan]

test/ELF/mips-merge-abiflags.s

6 ↗

(On Diff #81561)

I think we do not need to check .so file at all. Linker never reads an .MIPS.abiflags section from .so file. We need to check that LLD reads .o file with "invalid" .MIPS.abiflags section and generates a valid output.

I take a look at BFD linker's code. As far as I can see this linker accepts any .MIPS.abiflags sections with size equal or greater than sizeof(Elf_Mips_ABIFlags), but reads and uses only the first sizeof(Elf_Mips_ABIFlags) bytes. I agree that LLD should be tolerant to the .MIPS.abiflags section with a size greater than sizeof(Elf_Mips_ABIFlags) if the version field is valid. Maybe the section just padding by zero bytes for some reasons. But we do not need to create a workaround for "invalid" object files (created by another linker) more complicated than provided by BFD.

As to me I would check that a section size is equal or greater than sizeof(Elf_Mips_ABIFlags), then check the version field, and then read the first sizeof(Elf_Mips_ABIFlags) bytes from the section.

[ruiu]

Because this patch is to workaround for a old tool's bug, I want to know how important/severe that is first. We should generally avoid bug compatibility, so it needs a stronger reason than a ABI-compliant feature.

[arichardson]

In D27770#625122, @ruiu wrote:

Because this patch is to workaround for a old tool's bug, I want to know how important/severe that is first. We should generally avoid bug compatibility, so it needs a stronger reason than a ABI-compliant feature.

The problem is that this bug happens to be in the default linker for FreeBSD on MIPS (and therefore in many static libraries built with that linker) so for many programs it is not possible to link them using LLD.

test/ELF/mips-merge-abiflags.s

6 ↗

(On Diff #81561)

Thanks for looking. I'll update the patch to do that.
Should I emit a warning if the size is not equal to sizeof(Elf_Mips_ABIFlags)?

[ruiu]

ELF/SyntheticSections.cpp

138

We don't even sprinkle const that much, but we avoid auto as long as its type is obvious by RHS. So please avoid auto here.

139–143

I think the next if covers this case, no? If Size < ABIFlagsSize, its modulo is not zero.

[ruiu]

ELF/SyntheticSections.cpp

137–141

Can you please use size_t instead of auto?

[arichardson]

Use size_t

[atanasyan]

LGTM

I'll commit this patch soon.

ELF/SyntheticSections.cpp

MipsAbiFlagsSection<ELFT> *MipsAbiFlagsSection<ELFT>::create() {
 
   for (InputSectionBase<ELFT> *Sec : Symtab<ELFT>::X->Sections) {
     if (!Sec->Live || Sec->Type != SHT_MIPS_ABIFLAGS)
       continue;
     Sec->Live = false;
     Create = true;
 
     std::string Filename = toString(Sec->getFile());
-    if (Sec->Data.size() != sizeof(Elf_Mips_ABIFlags)) {
-      error(Filename + ": invalid size of .MIPS.abiflags section");
+    const auto Size = Sec->Data.size();
+    constexpr auto ABIFlagsSize = sizeof(Elf_Mips_ABIFlags);
+    if (Size < ABIFlagsSize) {
+      error(Filename + ": invalid size of .MIPS.abiflags section: got " +
+            Twine(Size) + " instead of " + Twine(ABIFlagsSize));
       return nullptr;
     }
-    auto *S = reinterpret_cast<const Elf_Mips_ABIFlags *>(Sec->Data.data());
+    // Older version of BFD concatenate .MIPS.abiflags instead of merging
+    if ((Size % ABIFlagsSize) != 0) {
+      error(Filename + ": invalid size of .MIPS.abiflags section: " +
+           Twine(Size) + " is not a multiple of " + Twine(ABIFlagsSize));
+    }
+
+    for (size_t Offset = 0; Offset < Size; Offset += ABIFlagsSize) {
+      auto *S = reinterpret_cast<const Elf_Mips_ABIFlags *>(Sec->Data.data() + Offset);
       if (S->version != 0) {
         error(Filename + ": unexpected .MIPS.abiflags version " +
               Twine(S->version));
         return nullptr;
       }
 
       // LLD checks ISA compatibility in getMipsEFlags(). Here we just
       // select the highest number of ISA/Rev/Ext.
       Flags.isa_level = std::max(Flags.isa_level, S->isa_level);
       Flags.isa_rev = std::max(Flags.isa_rev, S->isa_rev);
       Flags.isa_ext = std::max(Flags.isa_ext, S->isa_ext);
       Flags.gpr_size = std::max(Flags.gpr_size, S->gpr_size);
       Flags.cpr1_size = std::max(Flags.cpr1_size, S->cpr1_size);
       Flags.cpr2_size = std::max(Flags.cpr2_size, S->cpr2_size);
       Flags.ases |= S->ases;
       Flags.flags1 |= S->flags1;
       Flags.flags2 |= S->flags2;
       Flags.fp_abi = elf::getMipsFpAbiFlag(Flags.fp_abi, S->fp_abi, Filename);
+    }
   };
 
   if (Create)
     return make<MipsAbiFlagsSection<ELFT>>(Flags);
   return nullptr;
 }
 
 // .MIPS.options section.