[PATCH] D53495: Add OUTPUT_FORMAT linker script directive support.

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: lld/ELF/ScriptParser.cpp:410
+  if (Config->EKind == ELFNoneKind)
+    std::tie(Config->EKind, Config->EMachine) = BfdName;
+
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > grimar wrote:
> > > > 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?
> > > I don't think we need that test case.
> > Then you should be able to simplify to
> > 
> > `std::tie(Config->EKind, Config->EMachine) = readBfdName();`
> 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.
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.


================
Comment at: lld/test/ELF/invalid-linkerscript.test:53
 # RUN: not ld.lld %t8 no-such-file 2>&1 | FileCheck -check-prefix=ERR8 %s
-# ERR8: , expected, but got y
+# RUN: not ld.lld -m elf_amd64 %t8 no-such-file 2>&1 | FileCheck -check-prefix=ERR8 %s
+# ERR8: unknown output format name: x
----------------
I agree that is not useful. See my comment.


https://reviews.llvm.org/D53495