[LLD][COFF] Require explicit specification of ARM64EC target #116281
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
|
@llvm/pr-subscribers-platform-windows @llvm/pr-subscribers-lld-coff Author: Jacek Caban (cjacek) ChangesInferring the ARM64EC target can lead to errors. The Full diff: https://github.com/llvm/llvm-project/pull/116281.diff 2 Files Affected:
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 35d54d4945f7f4..db265e2bb37a04 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -46,6 +46,9 @@ static bool compatibleMachineType(COFFLinkerContext &ctx, MachineTypes mt) {
return COFF::isArm64EC(mt) || mt == AMD64;
case ARM64X:
return COFF::isAnyArm64(mt) || mt == AMD64;
+ case IMAGE_FILE_MACHINE_UNKNOWN:
+ // The ARM64EC target must be explicitly specified and cannot be inferred.
+ return !isArm64EC(mt);
default:
return ctx.config.machine == mt;
}
@@ -74,13 +77,18 @@ void SymbolTable::addFile(InputFile *file) {
}
MachineTypes mt = file->getMachineType();
+ if (!compatibleMachineType(ctx, mt)) {
+ if (isArm64EC(mt))
+ error(toString(file) + ": incompatible machine type " + machineToStr(mt) +
+ ", use /machine:arm64ec or /machine:arm64x");
+ else
+ error(toString(file) + ": machine type " + machineToStr(mt) +
+ " conflicts with " + machineToStr(ctx.config.machine));
+ return;
+ }
if (ctx.config.machine == IMAGE_FILE_MACHINE_UNKNOWN) {
ctx.config.machine = mt;
ctx.driver.addWinSysRootLibSearchPaths();
- } else if (!compatibleMachineType(ctx, mt)) {
- error(toString(file) + ": machine type " + machineToStr(mt) +
- " conflicts with " + machineToStr(ctx.config.machine));
- return;
}
ctx.driver.parseDirectives(file);
diff --git a/lld/test/COFF/arm64ec.test b/lld/test/COFF/arm64ec.test
index e50b14ce0184c8..16b596b432b12f 100644
--- a/lld/test/COFF/arm64ec.test
+++ b/lld/test/COFF/arm64ec.test
@@ -36,7 +36,7 @@ ARM64X-DATA: 03030303 01010101 02020202
RUN: not lld-link -out:test.dll -machine:arm64 arm64-data-sym.obj arm64ec-data-sym.obj \
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT1 %s
-INCOMPAT1: lld-link: error: arm64ec-data-sym.obj: machine type arm64ec conflicts with arm64
+INCOMPAT1: lld-link: error: arm64ec-data-sym.obj: incompatible machine type arm64ec, use /machine:arm64ec or /machine:arm64x
RUN: not lld-link -out:test.dll -machine:arm64ec arm64ec-data-sym.obj arm64-data-sym.obj \
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT2 %s
@@ -46,6 +46,10 @@ RUN: not lld-link -out:test.dll -machine:arm64 arm64-data-sym.obj x86_64-data-sy
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT3 %s
INCOMPAT3: lld-link: error: x86_64-data-sym.obj: machine type x64 conflicts with arm64
+arm64ec machine type can't be inferred, it must be specified explicitly.
+RUN: not lld-link -out:test.dll arm64ec-data-sym.obj \
+RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT1 %s
+
#--- arm64ec-data-sym.s
.data
.globl arm64ec_data_sym
|
Member
|
@llvm/pr-subscribers-lld Author: Jacek Caban (cjacek) ChangesInferring the ARM64EC target can lead to errors. The Full diff: https://github.com/llvm/llvm-project/pull/116281.diff 2 Files Affected:
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 35d54d4945f7f4..db265e2bb37a04 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -46,6 +46,9 @@ static bool compatibleMachineType(COFFLinkerContext &ctx, MachineTypes mt) {
return COFF::isArm64EC(mt) || mt == AMD64;
case ARM64X:
return COFF::isAnyArm64(mt) || mt == AMD64;
+ case IMAGE_FILE_MACHINE_UNKNOWN:
+ // The ARM64EC target must be explicitly specified and cannot be inferred.
+ return !isArm64EC(mt);
default:
return ctx.config.machine == mt;
}
@@ -74,13 +77,18 @@ void SymbolTable::addFile(InputFile *file) {
}
MachineTypes mt = file->getMachineType();
+ if (!compatibleMachineType(ctx, mt)) {
+ if (isArm64EC(mt))
+ error(toString(file) + ": incompatible machine type " + machineToStr(mt) +
+ ", use /machine:arm64ec or /machine:arm64x");
+ else
+ error(toString(file) + ": machine type " + machineToStr(mt) +
+ " conflicts with " + machineToStr(ctx.config.machine));
+ return;
+ }
if (ctx.config.machine == IMAGE_FILE_MACHINE_UNKNOWN) {
ctx.config.machine = mt;
ctx.driver.addWinSysRootLibSearchPaths();
- } else if (!compatibleMachineType(ctx, mt)) {
- error(toString(file) + ": machine type " + machineToStr(mt) +
- " conflicts with " + machineToStr(ctx.config.machine));
- return;
}
ctx.driver.parseDirectives(file);
diff --git a/lld/test/COFF/arm64ec.test b/lld/test/COFF/arm64ec.test
index e50b14ce0184c8..16b596b432b12f 100644
--- a/lld/test/COFF/arm64ec.test
+++ b/lld/test/COFF/arm64ec.test
@@ -36,7 +36,7 @@ ARM64X-DATA: 03030303 01010101 02020202
RUN: not lld-link -out:test.dll -machine:arm64 arm64-data-sym.obj arm64ec-data-sym.obj \
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT1 %s
-INCOMPAT1: lld-link: error: arm64ec-data-sym.obj: machine type arm64ec conflicts with arm64
+INCOMPAT1: lld-link: error: arm64ec-data-sym.obj: incompatible machine type arm64ec, use /machine:arm64ec or /machine:arm64x
RUN: not lld-link -out:test.dll -machine:arm64ec arm64ec-data-sym.obj arm64-data-sym.obj \
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT2 %s
@@ -46,6 +46,10 @@ RUN: not lld-link -out:test.dll -machine:arm64 arm64-data-sym.obj x86_64-data-sy
RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT3 %s
INCOMPAT3: lld-link: error: x86_64-data-sym.obj: machine type x64 conflicts with arm64
+arm64ec machine type can't be inferred, it must be specified explicitly.
+RUN: not lld-link -out:test.dll arm64ec-data-sym.obj \
+RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT1 %s
+
#--- arm64ec-data-sym.s
.data
.globl arm64ec_data_sym
|
dpaoliello
requested changes
Nov 14, 2024
lld/COFF/SymbolTable.cpp
Outdated
| MachineTypes mt = file->getMachineType(); | ||
| if (!compatibleMachineType(ctx, mt)) { | ||
| if (isArm64EC(mt)) | ||
| error(toString(file) + ": incompatible machine type " + machineToStr(mt) + |
Contributor
There was a problem hiding this comment.
Not a fan of this error message: it seems odd that if the user asked for the machine type to be inferred that the linker reports that something is incompatible. Maybe say that it is ambiguous or can't be decided/inferred?
Member
There was a problem hiding this comment.
But isn't it possible to hit this case too, e.g. if you're linking an i386 module, but you're hitting an arm64ec input file?
Member
There was a problem hiding this comment.
I.e., would it make sense to try to disambiguate these two cases here, for the sake of getting more easily understandable error messages?
Contributor
Author
There was a problem hiding this comment.
I’ve improved the error message and its logic. The updated implementation now tracks if the machine type was inferred earlier, as it could have been inferred as ARM64 or AMD64 from prior input files (as demonstrated in the tests). Additionally, I noticed that ARM64X input files are allowed to infer ARM64, so I’ve added support for that behavior as well.
Inferring the ARM64EC target can lead to errors. The `-machine:arm64ec` option may include x86_64 input files, and any valid ARM64EC input is also valid for `-machine:arm64x`. MSVC requires an explicit `-machine` argument with informative diagnostics; this patch adopts the same behavior.
cjacek
force-pushed
the
arm64ec-machine
branch
from
016e991 to
ff9f52c
Compare
mstorsjo
reviewed
Nov 15, 2024
lld/test/COFF/arm64ec.test
Outdated
| INCOMPAT5: arm64ec-data-sym.obj: machine type arm64ec conflicts with x86 | ||
|
|
||
| arm64x input implies arm64 target | ||
| RUN: lld-link -out:test.dll -machine:arm64 arm64x-resource.obj -dll -noentry |
Member
There was a problem hiding this comment.
As we're explicitly specifying -machine:arm64 here, I don't see how this triggers the implicit arm64x -> arm64 machine type handling?
Contributor
Author
There was a problem hiding this comment.
I messed up that test. After revisiting the MSVC behavior, I found that passing a single input file triggers an exception. However, when multiple files are passed, it behaves similarly to LLD without this change. I’ve now limited the impact of this change to ARM64EC, which aligns more closely with MSVC’s handling in these scenarios.
mstorsjo
reviewed
Nov 15, 2024
lld/test/COFF/arm64ec.test
Outdated
| RUN: not lld-link -out:test.dll -machine:arm64 arm64-data-sym.obj arm64ec-data-sym.obj \ | ||
| RUN: -dll -noentry 2>&1 | FileCheck -check-prefix=INCOMPAT1 %s | ||
| INCOMPAT1: lld-link: error: arm64ec-data-sym.obj: machine type arm64ec conflicts with arm64 | ||
| INCOMPAT1: arm64ec-data-sym.obj: machine type arm64ec conflicts with arm64 |
Member
There was a problem hiding this comment.
What happened with the lld-link: error: prefix here?
Contributor
Author
There was a problem hiding this comment.
An oversight, fixed, sorry.
cjacek
force-pushed
the
arm64ec-machine
branch
from
98ffbed to
82f4a1f
Compare
mstorsjo
approved these changes
Nov 16, 2024
Member
mstorsjo
left a comment
mstorsjo
left a comment
There was a problem hiding this comment.
LGTM, I think this is good now. But let's give @dpaoliello a chance to get back and comment as well.
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/7484 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Inferring the ARM64EC target can lead to errors. The
-machine:arm64ecoption may include x86_64 input files, and any valid ARM64EC input is also valid for-machine:arm64x. MSVC requires an explicit-machineargument with informative diagnostics; this patch adopts the same behavior.