Page MenuHomePhabricator

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

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

Diff Detail

Repository
rL LLVM

Event Timeline

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.

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

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 set the repository for this revision to rL LLVM.
atanasyan added a project: lld.
arichardson removed rL LLVM as the repository for this revision.

Added a test

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?

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.

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
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.

139–143 ↗(On Diff #81572)

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

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?

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
This revision was automatically updated to reflect the committed changes.