Page MenuHomePhabricator

Recognize FreeBSD specific BFD names in OUTPUT_FORMAT
ClosedPublic

Authored by dim on Jan 26 2019, 9:04 AM.

Details

Summary

After rLLD344952 ("Add OUTPUT_FORMAT linker script directive support"),
using BFD names such as elf64-x86-64-freebsd the OUTPUT_FORMAT
linker script command does not work anymore, resulting in errors like:

ld: error: /home/dim/src/clang800-import/stand/efi/loader/arch/amd64/ldscript.amd64:2: unknown output format name: elf64-x86-64-freebsd
>>> OUTPUT_FORMAT("elf64-x86-64-freebsd", "elf64-x86-64-freebsd", "elf64-x86-64-freebsd")
>>>               ^

To fix this, add the following FreeBSD specific BFD names to
readBfdName:

  • elf32-i386-freebsd
  • elf64-aarch64-freebsd
  • elf64-powerpc-freebsd
  • elf64-x86-64-freebsd

and also set Configuration::OSABI to ELFOSABI_FREEBSD for those
cases.

Add and/or update several test cases to check for the correct results of
these new OUTPUT_FORMAT arguments.

Note that some more generic parsing could probably be implemented, but I
would like to get something accepted for merging into the 8.0 branch, so
we can ship it in FreeBSD.

Diff Detail

Repository
rL LLVM

Event Timeline

dim created this revision.Jan 26 2019, 9:04 AM
grimar added inline comments.Jan 28 2019, 1:47 AM
ELF/ScriptParser.cpp
96 ↗(On Diff #183706)

I think we want to start using a new struct instead of tuple then.

dim marked an inline comment as done.Jan 28 2019, 4:03 AM
dim added inline comments.
ELF/ScriptParser.cpp
96 ↗(On Diff #183706)

Yeah, this is the disadvantage of unnamed tuples. I'll make a it a struct.

ruiu added a comment.Jan 28 2019, 11:02 AM

I think this part of code needs a little bit of refactoring. I'll submit it so that you can make your change on top of it.

ELF/ScriptParser.cpp
435 ↗(On Diff #183706)

Instead of recognizing all the combinations of *-freebsd, check if -freebsd is a suffix of a given string, and set OSABI to FREEBSD and then trim the string.

ruiu added a comment.Jan 28 2019, 11:15 AM

I submitted a cleanup patch to this code, so I think you can easily implement your feature. I'd think you want to use StringRef::consume_front.

dim updated this revision to Diff 184053.Jan 29 2019, 3:07 AM

Updated to address comments:

  • Replaced tuple with BfdDesc struct
  • Parse -freebsd suffix to determine OSABI
  • Add new case for elf-aarch64 (which used to be elf-aarch64-freebsd)

Please rebase.

ELF/ScriptParser.cpp
396 ↗(On Diff #184053)

readBfdName was removed in r352407. (method was converted to a static helper)

So you can define BfdDesc in an anonymous namespace.

401 ↗(On Diff #184053)

Then I think you can avoid adding a constructor in BfdDesc and can use:

return {ELF32LEKind, EM_386, OSABI, false};

ruiu added a comment.Jan 29 2019, 9:32 AM

Yes, please rebase.

dim updated this revision to Diff 184112.Jan 29 2019, 9:41 AM

Rebased.

ruiu added inline comments.Jan 29 2019, 9:47 AM
ELF/ScriptParser.cpp
393 ↗(On Diff #184112)

Can you move this to readOutputFormat? Then you don't need to change this function at all.

Also you shouldn't change OSABI if it does not end with -freebsd, as we shouldn't add the assumption if not FreeBSD, it is always ELFOSABI_NONE.

dim updated this revision to Diff 184169.Jan 29 2019, 2:02 PM

Updated as @ruiu suggested.

ruiu accepted this revision.Jan 29 2019, 2:40 PM

LGTM

This revision is now accepted and ready to land.Jan 29 2019, 2:40 PM
This revision was automatically updated to reflect the committed changes.