Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[lld] [MinGW] Support targeting ARM64EC #78911

Merged
merged 1 commit into from
Jan 30, 2024
Merged

[lld] [MinGW] Support targeting ARM64EC #78911

merged 1 commit into from
Jan 30, 2024

Conversation

bylaws
Copy link
Contributor

@bylaws bylaws commented Jan 21, 2024

'arm64ecpe' was chosen arbitrarily as gcc MinGW doesn't provide EC support.

CC: @cjacek

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024 •
edited

@llvm/pr-subscribers-lld-macho
@llvm/pr-subscribers-lld-wasm

@llvm/pr-subscribers-lld-elf

Author: Billy Laws (bylaws)

Changes

'arm64ecpe' was chosen arbitrarily as gcc MinGW doesn't provide EC support.

CC: @cjacek

Full diff: https://github.com/llvm/llvm-project/pull/78911.diff

3 Files Affected:

  • (modified) lld/Common/DriverDispatcher.cpp (+2-1)
  • (modified) lld/MinGW/Driver.cpp (+3)
  • (modified) lld/test/MinGW/driver.test (+6)
diff --git a/lld/Common/DriverDispatcher.cpp b/lld/Common/DriverDispatcher.cpp
index 435acfd277654b..f5c8bcdef4e0f0 100644
--- a/lld/Common/DriverDispatcher.cpp
+++ b/lld/Common/DriverDispatcher.cpp
@@ -44,7 +44,8 @@ static cl::TokenizerCallback getDefaultQuotingStyle() {
 }
 
 static bool isPETargetName(StringRef s) {
-  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe";
+  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe" ||
+         s == "arm64ecpe";
 }
 
 static std::optional<bool> isPETarget(llvm::ArrayRef<const char *> args) {
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 4752d92e3b1d71..290eecaacda7d4 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -346,6 +346,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
 
   if (args.getLastArgValue(OPT_m) != "thumb2pe" &&
       args.getLastArgValue(OPT_m) != "arm64pe" &&
+      args.getLastArgValue(OPT_m) != "arm64ecpe" &&
       args.hasFlag(OPT_disable_dynamicbase, OPT_dynamicbase, false))
     add("-dynamicbase:no");
   if (args.hasFlag(OPT_disable_high_entropy_va, OPT_high_entropy_va, false))
@@ -409,6 +410,8 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-machine:arm");
     else if (s == "arm64pe")
       add("-machine:arm64");
+    else if (s == "arm64ecpe")
+      add("-machine:arm64ec");
     else
       error("unknown parameter: -m" + s);
   }
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 559a32bfa242f8..46b3b6d7a862a6 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -25,6 +25,12 @@ ARM64-SAME: -machine:arm64
 ARM64-SAME: -alternatename:__image_base__=__ImageBase
 ARM64-SAME: foo.o
 
+RUN: ld.lld -### foo.o -m arm64ecpe 2>&1 | FileCheck -check-prefix=ARM64EC %s
+ARM64EC:      -out:a.exe
+ARM64EC-SAME: -machine:arm64ec
+ARM64EC-SAME: -alternatename:__image_base__=__ImageBase
+ARM64EC-SAME: foo.o
+
 RUN: ld.lld -### foo.o -m i386pep -shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --dll 2>&1 | FileCheck -check-prefix=SHARED %s

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024

@llvm/pr-subscribers-lld-coff

Author: Billy Laws (bylaws)

Changes

'arm64ecpe' was chosen arbitrarily as gcc MinGW doesn't provide EC support.

CC: @cjacek

Full diff: https://github.com/llvm/llvm-project/pull/78911.diff

3 Files Affected:

  • (modified) lld/Common/DriverDispatcher.cpp (+2-1)
  • (modified) lld/MinGW/Driver.cpp (+3)
  • (modified) lld/test/MinGW/driver.test (+6)
diff --git a/lld/Common/DriverDispatcher.cpp b/lld/Common/DriverDispatcher.cpp
index 435acfd277654b1..f5c8bcdef4e0f09 100644
--- a/lld/Common/DriverDispatcher.cpp
+++ b/lld/Common/DriverDispatcher.cpp
@@ -44,7 +44,8 @@ static cl::TokenizerCallback getDefaultQuotingStyle() {
 }
 
 static bool isPETargetName(StringRef s) {
-  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe";
+  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe" ||
+         s == "arm64ecpe";
 }
 
 static std::optional<bool> isPETarget(llvm::ArrayRef<const char *> args) {
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 4752d92e3b1d71d..290eecaacda7d49 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -346,6 +346,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
 
   if (args.getLastArgValue(OPT_m) != "thumb2pe" &&
       args.getLastArgValue(OPT_m) != "arm64pe" &&
+      args.getLastArgValue(OPT_m) != "arm64ecpe" &&
       args.hasFlag(OPT_disable_dynamicbase, OPT_dynamicbase, false))
     add("-dynamicbase:no");
   if (args.hasFlag(OPT_disable_high_entropy_va, OPT_high_entropy_va, false))
@@ -409,6 +410,8 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-machine:arm");
     else if (s == "arm64pe")
       add("-machine:arm64");
+    else if (s == "arm64ecpe")
+      add("-machine:arm64ec");
     else
       error("unknown parameter: -m" + s);
   }
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 559a32bfa242f8c..46b3b6d7a862a65 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -25,6 +25,12 @@ ARM64-SAME: -machine:arm64
 ARM64-SAME: -alternatename:__image_base__=__ImageBase
 ARM64-SAME: foo.o
 
+RUN: ld.lld -### foo.o -m arm64ecpe 2>&1 | FileCheck -check-prefix=ARM64EC %s
+ARM64EC:      -out:a.exe
+ARM64EC-SAME: -machine:arm64ec
+ARM64EC-SAME: -alternatename:__image_base__=__ImageBase
+ARM64EC-SAME: foo.o
+
 RUN: ld.lld -### foo.o -m i386pep -shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --dll 2>&1 | FileCheck -check-prefix=SHARED %s

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 21, 2024

@llvm/pr-subscribers-lld

Author: Billy Laws (bylaws)

Changes

'arm64ecpe' was chosen arbitrarily as gcc MinGW doesn't provide EC support.

CC: @cjacek

Full diff: https://github.com/llvm/llvm-project/pull/78911.diff

3 Files Affected:

  • (modified) lld/Common/DriverDispatcher.cpp (+2-1)
  • (modified) lld/MinGW/Driver.cpp (+3)
  • (modified) lld/test/MinGW/driver.test (+6)
diff --git a/lld/Common/DriverDispatcher.cpp b/lld/Common/DriverDispatcher.cpp
index 435acfd277654b..f5c8bcdef4e0f0 100644
--- a/lld/Common/DriverDispatcher.cpp
+++ b/lld/Common/DriverDispatcher.cpp
@@ -44,7 +44,8 @@ static cl::TokenizerCallback getDefaultQuotingStyle() {
 }
 
 static bool isPETargetName(StringRef s) {
-  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe";
+  return s == "i386pe" || s == "i386pep" || s == "thumb2pe" || s == "arm64pe" ||
+         s == "arm64ecpe";
 }
 
 static std::optional<bool> isPETarget(llvm::ArrayRef<const char *> args) {
diff --git a/lld/MinGW/Driver.cpp b/lld/MinGW/Driver.cpp
index 4752d92e3b1d71..290eecaacda7d4 100644
--- a/lld/MinGW/Driver.cpp
+++ b/lld/MinGW/Driver.cpp
@@ -346,6 +346,7 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
 
   if (args.getLastArgValue(OPT_m) != "thumb2pe" &&
       args.getLastArgValue(OPT_m) != "arm64pe" &&
+      args.getLastArgValue(OPT_m) != "arm64ecpe" &&
       args.hasFlag(OPT_disable_dynamicbase, OPT_dynamicbase, false))
     add("-dynamicbase:no");
   if (args.hasFlag(OPT_disable_high_entropy_va, OPT_high_entropy_va, false))
@@ -409,6 +410,8 @@ bool link(ArrayRef<const char *> argsArr, llvm::raw_ostream &stdoutOS,
       add("-machine:arm");
     else if (s == "arm64pe")
       add("-machine:arm64");
+    else if (s == "arm64ecpe")
+      add("-machine:arm64ec");
     else
       error("unknown parameter: -m" + s);
   }
diff --git a/lld/test/MinGW/driver.test b/lld/test/MinGW/driver.test
index 559a32bfa242f8..46b3b6d7a862a6 100644
--- a/lld/test/MinGW/driver.test
+++ b/lld/test/MinGW/driver.test
@@ -25,6 +25,12 @@ ARM64-SAME: -machine:arm64
 ARM64-SAME: -alternatename:__image_base__=__ImageBase
 ARM64-SAME: foo.o
 
+RUN: ld.lld -### foo.o -m arm64ecpe 2>&1 | FileCheck -check-prefix=ARM64EC %s
+ARM64EC:      -out:a.exe
+ARM64EC-SAME: -machine:arm64ec
+ARM64EC-SAME: -alternatename:__image_base__=__ImageBase
+ARM64EC-SAME: foo.o
+
 RUN: ld.lld -### foo.o -m i386pep -shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --shared 2>&1 | FileCheck -check-prefix=SHARED %s
 RUN: ld.lld -### foo.o -m i386pep --dll 2>&1 | FileCheck -check-prefix=SHARED %s

@bylaws bylaws mentioned this pull request Jan 21, 2024
Merged
@mstorsjo
Copy link
Member

I think this looks good, code wise, and it does have tests, so that's good.

I'd like to explicitly defer merging it for a couple days though, as 18.x is being branched on Tuesday. I think it's best to leave this change out of this release round, so we have another 6 months before freezing the naming harder, in case we have second thoughts on it.

'arm64ecpe' was chosen arbitrarily as gcc MinGW doesn't provide EC support.

FWIW, it can still be good to keep them in mind even if they don't support it yet.

We settled on arm64pe as emulation name for this target in Clang/LLD in 2017, in 31cac7a and 894dbbe. Recently (2022-2023), binutils did start to get support for regular aarch64 on Windows. But even despite the prior art on the area, they couldn't accept arm64pe as name for it, it had to be aarch64pe. They did accept adding arm64pe as an alias though, see bminor/binutils-gdb@c60b380 and bminor/binutils-gdb@beb6b61, and https://sourceware.org/pipermail/binutils/2023-January/125435.html, https://sourceware.org/pipermail/binutils/2023-January/125437.html and more in that thread.

But this particular feature here is normally called arm64ec, so I'm not really sure what's the best way to go - keeping it arm64ec and risk yet more divergence if binutils ever wants to implement the same, or preemptively try to go to aarch64ecpe?

@cjacek
Copy link
Contributor

cjacek commented Jan 22, 2024

Looks good to me, including name choice. For the naming consideration, we will also be need something for ARM64X and I guess it will be arm64xpe.

@bylaws
Copy link
Contributor Author

bylaws commented Jan 23, 2024

@mstorsjo I think even with what MinGW is doing arm64ecpe makes more sense over aarch64ecpe, for the same reason that __arm64ec__ is defined and not __aarch64ec__

@mstorsjo mstorsjo merged commit a9ffdc1 into llvm:main Jan 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstorsjo mstorsjo Awaiting requested review from mstorsjo

@cjacek cjacek Awaiting requested review from cjacek

Assignees
No one assigned
Labels
lld:COFF lld:ELF lld:MachO lld:wasm lld
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bylaws @llvmbot @mstorsjo @cjacek