[PATCH] D149087: [lld] Allow input files from compatible architectures on EC targets.

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

Overall still looking good, thanks!



================
Comment at: lld/COFF/SymbolTable.cpp:42
+  case ARM64:
+    return mt == ARM64 || mt == ARM64X;
+  case ARM64EC:
----------------
jacek wrote:
> mstorsjo wrote:
> > Just to check that I've understood correctly here... We don't really expect the input of an individual object file to ever be `ARM64X`, but only `ARM64`, `AMD64` or `ARM64EC`, while `ARM64X` is an image level flag to indicate that it contains both `ARM64` and (`ARM64EC` or `AMD64`), more or less?
> > 
> > (I don't mind keeping the case here just for completeness anyway though.)
> We do expect ARM64X on input as well. Individual input object file with its machine value of ARM64X are rare and weird, but they do exist and llvm-cvtres can produce them with https://reviews.llvm.org/D148517 I will update patches with tests for them. According to my testing, ARM64X objects are accepted by link.exe for all targets: ARM64, ARM64EC and ARM64X.
> 
> In case of linked images, ARM64X is not used as machine type in PE COFF header. ARM64 is used instead and CHPE metadata referenced by load config distinguishes such files from pure ARM64. From linker point of view, CHPE metadata and load config are provided by CRT libraries, but there is a number of synthetic symbols that linker will have to provide that CRT uses to fill it. (I have enough of those symbols prototyped in my tree to get basic ARM64EC working, not yet ARM64X).
Oh, right, thanks for clarifying!


================
Comment at: lld/test/COFF/arm64ec.test:36
+ARM64X-DATA: 03030303 01010101 02020202
+
 #--- arm64ec-data-sym.s
----------------
Would it be worthwhile to have a few negative tests too? E.g. to test that arm64ec output refuses to include a regular arm64 input object file, or that arm64 output refuses to include arm64ec and x86_64?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149087/new/

https://reviews.llvm.org/D149087