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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/ScriptParser.cpp:97
   SymbolAssignment *readAssignment(StringRef Tok);
+  std::pair<ELFKind, uint16_t> readBfdName();
   void readSort();
----------------
grimar wrote:
> It was confusing naming for me. Was not clear what is "bfd name".
> Maybe `readBfdArch` or `readBfdArchName`?
It seems "BFD name" is an established technical term. You can find "BFD name" in the BFD ld man page.


================
Comment at: lld/ELF/ScriptParser.cpp:394
+    return {ELF32LEKind, EM_X86_64};
+  if (S == "elf64-littleaarch64")
+    return {ELF64LEKind, EM_AARCH64};
----------------
grimar wrote:
> 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?
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.


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


================
Comment at: lld/test/ELF/linkerscript/output-format.s:5
-# RUN: ld.lld -shared -o %t2 %t1 %t.script
-# RUN: llvm-readobj %t2 > /dev/null
-
----------------
grimar wrote:
> This tested we are ignoring the big and little parameters. I think the test was useful. Should it be moved to emulation.s?
I don't think so -- I'm not too worried about them.


https://reviews.llvm.org/D53495