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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/ScriptParser.cpp:410
+  if (Config->EKind == ELFNoneKind)
+    std::tie(Config->EKind, Config->EMachine) = BfdName;
+
----------------
grimar wrote:
> 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.
allows us to the incompatible -> allows us to report the incompatible


https://reviews.llvm.org/D53495