Page MenuHomePhabricator

Add OUTPUT_FORMAT linker script directive support.
ClosedPublic

Authored by ruiu on Oct 22 2018, 6:43 AM.

Details

Summary

This patch adds a support for OUTPUT_FORMAT linker script directive.
Since I'm not 100% confident with BFD names you can use in the directive
for all architectures, I added only a few in this patch. We can add
other names for other archtiectures later.

We still do not support triple-style OUTPUT_FORMAT directive, namely,
OUTPUT_FORMAT(bfdname, big, little). If you pass -EL (little endian)
or -EB (big endian) to the linker, GNU linkers pick up big or little
as a BFD name, correspondingly, so that you can use a single linker
script for bi-endian processor. I'm not sure if we really need to
support that, so I'll leave it alone for now.

Note that -m takes precedence over OUTPUT_FORAMT, but we always parse
a BFD name given to OUTPUT_FORMAT for error checking. You cannot write
an invalid name in the OUTPUT_FORMAT directive.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

ruiu created this revision.Oct 22 2018, 6:43 AM
grimar added inline comments.Oct 22 2018, 7:06 AM
lld/ELF/ScriptParser.cpp
97 ↗(On Diff #170402)

It was confusing naming for me. Was not clear what is "bfd name".
Maybe readBfdArch or readBfdArchName?

394 ↗(On Diff #170402)

An observation: bfd code contains both elf64-littleaarch64-cloudabi and elf64-littleaarch64 for example.
Should S == in this function be changed to S.startsWith maybe?

410 ↗(On Diff #170402)

So I think this means LLD will error out when seeing the object file with incompatible architecture after parsing the script containing OUTPUT_FORMAT. Behavior is fine, but I think there is no test provided for that case?

lld/test/ELF/linkerscript/output-format.s
5 ↗(On Diff #170402)

This tested we are ignoring the big and little parameters. I think the test was useful. Should it be moved to emulation.s?

ruiu added inline comments.Oct 22 2018, 8:07 AM
lld/ELF/ScriptParser.cpp
97 ↗(On Diff #170402)

It seems "BFD name" is an established technical term. You can find "BFD name" in the BFD ld man page.

394 ↗(On Diff #170402)

I'd like to address that in another patch, because I don't know what exactly we should do for these BFD names with extensions at the moment.

410 ↗(On Diff #170402)

I don't think we need that test case.

lld/test/ELF/linkerscript/output-format.s
5 ↗(On Diff #170402)

I don't think so -- I'm not too worried about them.

grimar added inline comments.Oct 22 2018, 8:23 AM
lld/ELF/ScriptParser.cpp
410 ↗(On Diff #170402)

Then you should be able to simplify to

std::tie(Config->EKind, Config->EMachine) = readBfdName();

ruiu added inline comments.Oct 22 2018, 8:30 AM
lld/ELF/ScriptParser.cpp
410 ↗(On Diff #170402)

I'm not too picky about aiming 100% test coverage, and I meant that we don't need a test case for that in that sense. I'll add a test for you but we don't need to test for all possible combinations of conditions.

ruiu updated this revision to Diff 170426.Oct 22 2018, 8:32 AM
  • add a test case
grimar accepted this revision.Oct 22 2018, 9:29 AM

LGTM.

lld/ELF/ScriptParser.cpp
410 ↗(On Diff #170402)

I meant the following scenario:

If we have file1_386.o (i386), script saying OUTPUT_FORMAT("elf64-x86-64") and file2_x64.o (x64).
And invocation: lld file1_386.o -script script.file file2_x64.o

Then having the if (Config->EKind == ELFNoneKind) line here allows us to the incompatible
architectures error properly. That what I suggested to test initially.

It's also a bit strange that we will accept
lld file1_386.o -script script.file
but will fail on
lld -script script.file file1_386.o

We could have a test explaining the behavior too.

Though I think it is acceptable for now. Let's go as is then.

lld/test/ELF/invalid-linkerscript.test
53 ↗(On Diff #170426)

I agree that is not useful. See my comment.

This revision is now accepted and ready to land.Oct 22 2018, 9:29 AM
grimar added inline comments.Oct 22 2018, 9:34 AM
lld/ELF/ScriptParser.cpp
410 ↗(On Diff #170402)

allows us to the incompatible -> allows us to report the incompatible

This revision was automatically updated to reflect the committed changes.