This is an archive of the discontinued LLVM Phabricator instance.

Differential D27770

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

Authored by arichardson on Dec 14 2016, 1:25 PM.

Details

Reviewers
atanasyan
Commits
rG86dc60d8d4c7: [ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct
rLLD290237: [ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct
rL290237: [ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct
Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

arichardson updated this revision to Diff 81451.Dec 14 2016, 1:25 PM
arichardson retitled this revision from to [ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct.
arichardson updated this object.
arichardson added a reviewer: atanasyan.
arichardson added a comment.Dec 14 2016, 1:27 PM

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 added a reviewer: llvm-commits.Dec 15 2016, 2:14 AM
atanasyan added a subscriber: llvm-commits.
atanasyan edited edge metadata.Dec 15 2016, 2:18 AM
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.

atanasyan removed a reviewer: llvm-commits.Dec 15 2016, 2:23 AM
atanasyan set the repository for this revision to rL LLVM.
atanasyan added a project: lld.
arichardson updated this revision to Diff 81561.Dec 15 2016, 3:54 AM
arichardson removed rL LLVM as the repository for this revision.

Added a test

Herald added a subscriber: sdardis. · View Herald TranscriptDec 15 2016, 3:54 AM
atanasyan added inline comments.Dec 15 2016, 4:39 AM
ELF/SyntheticSections.cpp
138 ↗(On Diff #81561)

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 added inline comments.Dec 15 2016, 5:46 AM
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 this revision to Diff 81572.Dec 15 2016, 6:32 AM

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

atanasyan added inline comments.Dec 16 2016, 2:26 AM
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 added a subscriber: ruiu.Dec 16 2016, 9:09 AM

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 added a comment.Dec 16 2016, 10:19 AM
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 added inline comments.Dec 16 2016, 10:34 AM
ELF/SyntheticSections.cpp
139–143 ↗(On Diff #81572)

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

138 ↗(On Diff #81561)

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.

arichardson updated this revision to Diff 81946.Dec 19 2016, 6:56 AM
emaste added a subscriber: emaste.Dec 19 2016, 6:20 PM
ruiu added inline comments.Dec 19 2016, 11:46 PM
ELF/SyntheticSections.cpp
137 ↗(On Diff #81946)

Can you please use size_t instead of auto?

arichardson updated this revision to Diff 82095.Dec 20 2016, 4:49 AM
Use size_t
atanasyan accepted this revision.Dec 20 2016, 6:43 AM
atanasyan edited edge metadata.

LGTM

I'll commit this patch soon.

This revision is now accepted and ready to land.Dec 20 2016, 6:43 AM
Closed by commit rL290237: [ELF][MIPS] Allow .MIPS.abiflags larger than one Elf_Mips_ABIFlags struct (authored by atanasyan). · Explain WhyDec 20 2016, 9:42 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lld/
trunk/
ELF/
9 lines
test/
ELF/
Inputs/
63 lines

Diff 82199

lld/trunk/ELF/SyntheticSections.cpp

Show First 20 Lines • Show All 128 Lines • ▼ Show 20 LinesMipsAbiFlagsSection<ELFT> *MipsAbiFlagsSection<ELFT>::create() {
for (InputSectionBase<ELFT> *Sec : Symtab<ELFT>::X->Sections) { for (InputSectionBase<ELFT> *Sec : Symtab<ELFT>::X->Sections) {
if (!Sec->Live || Sec->Type != SHT_MIPS_ABIFLAGS) if (!Sec->Live || Sec->Type != SHT_MIPS_ABIFLAGS)
continue; continue;
Sec->Live = false; Sec->Live = false;
Create = true; Create = true;
std::string Filename = toString(Sec->getFile()); std::string Filename = toString(Sec->getFile());
if (Sec->Data.size() != sizeof(Elf_Mips_ABIFlags)) { const size_t Size = Sec->Data.size();
error(Filename + ": invalid size of .MIPS.abiflags section"); // Older version of BFD (such as the default FreeBSD linker) concatenate
// .MIPS.abiflags instead of merging. To allow for this case (or potential
// zero padding) we ignore everything after the first Elf_Mips_ABIFlags
if (Size < sizeof(Elf_Mips_ABIFlags)) {
error(Filename + ": invalid size of .MIPS.abiflags section: got " +
Twine(Size) + " instead of " + Twine(sizeof(Elf_Mips_ABIFlags)));
return nullptr; return nullptr;
} }
auto *S = reinterpret_cast<const Elf_Mips_ABIFlags *>(Sec->Data.data()); auto *S = reinterpret_cast<const Elf_Mips_ABIFlags *>(Sec->Data.data());
if (S->version != 0) { if (S->version != 0) {
error(Filename + ": unexpected .MIPS.abiflags version " + error(Filename + ": unexpected .MIPS.abiflags version " +
Twine(S->version)); Twine(S->version));
return nullptr; return nullptr;
} }
▲ Show 20 Lines • Show All 1,839 Lines • Show Last 20 Lines

lld/trunk/test/ELF/Inputs/mips-concatenated-abiflags.o

  • This is a binary file.

lld/trunk/test/ELF/mips-merge-abiflags.s

# Test that lld handles input files with concatenated .MIPS.abiflags sections
# This happens e.g. with the FreeBSD BFD (BFD 2.17.50 [FreeBSD] 2007-07-03)
# RUN: llvm-mc -filetype=obj -triple=mips64-unknown-freebsd %s -o %t.o
# RUN: ld.lld %t.o %p/Inputs/mips-concatenated-abiflags.o -o %t.exe
# RUN: llvm-readobj -sections -mips-abi-flags %t.exe | FileCheck %s
# RUN: llvm-readobj -sections -mips-abi-flags \
# RUN: %p/Inputs/mips-concatenated-abiflags.o | \
# RUN: FileCheck --check-prefix=INPUT-OBJECT %s
# REQUIRES: mips
.globl __start
__start:
nop
# CHECK: Section {
# CHECK: Index: 1
# CHECK-NEXT: Name: .MIPS.abiflags
# CHECK-NEXT: Type: SHT_MIPS_ABIFLAGS
# CHECK-NEXT: Flags [
# CHECK-NEXT: SHF_ALLOC
# CHECK-NEXT: ]
# CHECK-NEXT: Address:
# CHECK-NEXT: Offset:
# CHECK-NEXT: Size: 24
# CHECK-NEXT: Link: 0
# CHECK-NEXT: Info: 0
# CHECK-NEXT: AddressAlignment: 8
# CHECK-NEXT: EntrySize: 24
# CHECK-NEXT: }
# CHECK: MIPS ABI Flags {
# CHECK-NEXT: Version: 0
# CHECK-NEXT: ISA: MIPS64
# CHECK-NEXT: ISA Extension: None
# CHECK-NEXT: ASEs [
# CHECK-NEXT: ]
# CHECK-NEXT: FP ABI: Hard float (double precision)
# CHECK-NEXT: GPR size: 64
# CHECK-NEXT: CPR1 size: 64
# CHECK-NEXT: CPR2 size: 0
# CHECK-NEXT: Flags 1 [
# CHECK-NEXT: ODDSPREG
# CHECK-NEXT: ]
# CHECK-NEXT: Flags 2: 0x0
# CHECK-NEXT: }
# INPUT-OBJECT: Section {
# INPUT-OBJECT: Index: 3
# INPUT-OBJECT-NEXT: Name: .MIPS.abiflags
# INPUT-OBJECT-NEXT: Type: SHT_MIPS_ABIFLAGS
# INPUT-OBJECT-NEXT: Flags [
# INPUT-OBJECT-NEXT: SHF_ALLOC
# INPUT-OBJECT-NEXT: ]
# INPUT-OBJECT-NEXT: Address:
# INPUT-OBJECT-NEXT: Offset:
# INPUT-OBJECT-NEXT: Size: 48
# INPUT-OBJECT-NEXT: Link: 0
# INPUT-OBJECT-NEXT: Info: 0
# INPUT-OBJECT-NEXT: AddressAlignment: 8
# INPUT-OBJECT-NEXT: EntrySize: 0
# INPUT-OBJECT-NEXT: }
# INPUT-OBJECT: The .MIPS.abiflags section has a wrong size.