Page MenuHomePhabricator

Recover elf32-bigmips and elf32-powerpc support in LLD
ClosedPublic

Authored by vit9696 on Feb 9 2019, 11:40 AM.

Details

Summary

This fixes a 7.0 -> 8.0 regression when parsing OUTPUT_FORMAT("elf32-powerpc"); or elf32-bigmips directive in ldscripts as well as an unknown emulation error when lld is invoked by clang due to missed elf32ppclinux case.

I request this to be backported to LLD 8.0, as it is a regression breaking ldscripts that worked fine with LLD 7.0.

Diff Detail

Event Timeline

vit9696 created this revision.Feb 9 2019, 11:40 AM
vit9696 updated this revision to Diff 186324.Feb 11 2019, 1:52 PM
vit9696 retitled this revision from Recover elf32-powerpc support in LLD to Recover elf32-bigmips and elf32-powerpc support in LLD.
vit9696 edited the summary of this revision. (Show Details)

Sorry, did not realise that we have tests for this functionality in such an undescriptive place. Added tests and merged with D58007 as suggested by Rui.

ruiu added inline comments.Feb 11 2019, 2:07 PM
lld/ELF/Driver.cpp
132

Do you know where elf32ppclinux came from? Is this really correct to handle it as a synonym for elf32ppc?

lld/ELF/ScriptParser.cpp
398

Ditto -- do you know if this correct to handle elf32-bigmips as a synonym for elf32-tradbigmips?

lld/test/ELF/emulation-ppc.s
107–108

Please test -m elf32ppclinux as well.

vit9696 updated this revision to Diff 186335.Feb 11 2019, 2:21 PM

Done with the test update. As for your other comments, there are some minor differences in section handling for different output types in GNU binutils, but I believe they have little to do with LLD abilities as of now. ppclinux one is chosen when clang is used for linking under Linux target. You can get some more details by checking ld/emulation folder, but I believe it is just fine to have these as synonyms for a long while to let most of the code continue building just fine.

ruiu added a comment.Feb 11 2019, 2:30 PM

Yeah maybe it is fine to add elf32ppclinux as a synonym for elf32ppc.

One last thing: please add a test to lld/test/ELF/emulation-mips.s for the mips synonym.

vit9696 updated this revision to Diff 186387.Feb 11 2019, 7:20 PM

Done, I must have been quite tired yesterday, thought I included it.

Would you take the effort with the merge request?

ruiu accepted this revision.Feb 12 2019, 2:22 PM

LGTM

In order to have this cherry-picked, please commit this patch first and then file a cherry pick request as a new bug and make the new bug depend on https://bugs.llvm.org/show_bug.cgi?id=40331.

This revision is now accepted and ready to land.Feb 12 2019, 2:22 PM

I am fine to create bugs, but I am yet to ask for commit access, and somebody needs to commit the patch for the time being. For this reason I thought it would be faster to leave the rest up to you.

ruiu added a comment.Feb 12 2019, 2:30 PM

Oh sorry, I'll commit and merge this to 8.0 for you.

Thanks a lot!

This revision was automatically updated to reflect the committed changes.
ruiu added a comment.Feb 13 2019, 10:53 AM

Submitted this patch and filed a merge request as https://bugs.llvm.org/show_bug.cgi?id=40721.