[win][aarch64] Always reserve frame pointers for Arm64 Windows #146582
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
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-aarch64 @llvm/pr-subscribers-clang-driver Author: Daniel Paoliello (dpaoliello) ChangesThere is no way in Arm64 Windows to indicate that a given function has used the Frame Pointer as a General Purpose Register, as such stack walks will always assume that the frame chain is valid and will follow whatever value has been saved for the Frame Pointer (even if it is pointing to data, etc.). This change makes the Frame Pointer always reserved when building for Arm64 Windows to avoid this issue. We will be updating the official Windows ABI documentation to reflect this requirement, and I will provide a link once it's available. cc @pmsjt Full diff: https://github.com/llvm/llvm-project/pull/146582.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 070901f037823..2fcf9b28dc746 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -174,7 +174,13 @@ static bool mustUseNonLeafFramePointerForTarget(const llvm::Triple &Triple) {
// even if new frame records are not created.
static bool mustMaintainValidFrameChain(const llvm::opt::ArgList &Args,
const llvm::Triple &Triple) {
- if (Triple.isARM() || Triple.isThumb()) {
+ switch (Triple.getArch()) {
+ default:
+ return false;
+ case llvm::Triple::arm:
+ case llvm::Triple::armeb:
+ case llvm::Triple::thumb:
+ case llvm::Triple::thumbeb:
// For 32-bit Arm, the -mframe-chain=aapcs and -mframe-chain=aapcs+leaf
// options require the frame pointer register to be reserved (or point to a
// new AAPCS-compilant frame record), even with -fno-omit-frame-pointer.
@@ -183,8 +189,13 @@ static bool mustMaintainValidFrameChain(const llvm::opt::ArgList &Args,
return V != "none";
}
return false;
+
+ case llvm::Triple::aarch64:
+ // Arm64 Windows requires that the frame chain is valid, as there is no
+ // way to indicate during a stack walk that a frame has used the frame
+ // pointer as a general purpose register.
+ return Triple.isOSWindows();
}
- return false;
}
// True if a target-specific option causes -fno-omit-frame-pointer to also
diff --git a/clang/test/Driver/frame-pointer-elim.c b/clang/test/Driver/frame-pointer-elim.c
index f64ff6efc7261..0dd7eb0c738db 100644
--- a/clang/test/Driver/frame-pointer-elim.c
+++ b/clang/test/Driver/frame-pointer-elim.c
@@ -4,6 +4,8 @@
// KEEP-NON-LEAF: "-mframe-pointer=non-leaf"
// KEEP-NONE-NOT: warning: argument unused
// KEEP-NONE: "-mframe-pointer=none"
+// KEEP-RESERVED-NOT: warning: argument unused
+// KEEP-RESERVED: "-mframe-pointer=reserved"
// On Linux x86, omit frame pointer when optimization is enabled.
// RUN: %clang -### --target=i386-linux -S -fomit-frame-pointer %s 2>&1 | \
@@ -215,5 +217,9 @@
// RUN: %clang -### --target=aarch64-none-elf -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
// RUN: FileCheck --check-prefix=KEEP-NON-LEAF %s
+// AArch64 Windows requires that the frame pointer be reserved
+// RUN: %clang -### --target=aarch64-pc-windows-msvc -S -fomit-frame-pointer %s 2>&1 | \
+// RUN: FileCheck --check-prefix=KEEP-RESERVED %s
+
void f0() {}
void f1() { f0(); }
|
|
@llvm/pr-subscribers-clang Author: Daniel Paoliello (dpaoliello) ChangesThere is no way in Arm64 Windows to indicate that a given function has used the Frame Pointer as a General Purpose Register, as such stack walks will always assume that the frame chain is valid and will follow whatever value has been saved for the Frame Pointer (even if it is pointing to data, etc.). This change makes the Frame Pointer always reserved when building for Arm64 Windows to avoid this issue. We will be updating the official Windows ABI documentation to reflect this requirement, and I will provide a link once it's available. cc @pmsjt Full diff: https://github.com/llvm/llvm-project/pull/146582.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 070901f037823..2fcf9b28dc746 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -174,7 +174,13 @@ static bool mustUseNonLeafFramePointerForTarget(const llvm::Triple &Triple) {
// even if new frame records are not created.
static bool mustMaintainValidFrameChain(const llvm::opt::ArgList &Args,
const llvm::Triple &Triple) {
- if (Triple.isARM() || Triple.isThumb()) {
+ switch (Triple.getArch()) {
+ default:
+ return false;
+ case llvm::Triple::arm:
+ case llvm::Triple::armeb:
+ case llvm::Triple::thumb:
+ case llvm::Triple::thumbeb:
// For 32-bit Arm, the -mframe-chain=aapcs and -mframe-chain=aapcs+leaf
// options require the frame pointer register to be reserved (or point to a
// new AAPCS-compilant frame record), even with -fno-omit-frame-pointer.
@@ -183,8 +189,13 @@ static bool mustMaintainValidFrameChain(const llvm::opt::ArgList &Args,
return V != "none";
}
return false;
+
+ case llvm::Triple::aarch64:
+ // Arm64 Windows requires that the frame chain is valid, as there is no
+ // way to indicate during a stack walk that a frame has used the frame
+ // pointer as a general purpose register.
+ return Triple.isOSWindows();
}
- return false;
}
// True if a target-specific option causes -fno-omit-frame-pointer to also
diff --git a/clang/test/Driver/frame-pointer-elim.c b/clang/test/Driver/frame-pointer-elim.c
index f64ff6efc7261..0dd7eb0c738db 100644
--- a/clang/test/Driver/frame-pointer-elim.c
+++ b/clang/test/Driver/frame-pointer-elim.c
@@ -4,6 +4,8 @@
// KEEP-NON-LEAF: "-mframe-pointer=non-leaf"
// KEEP-NONE-NOT: warning: argument unused
// KEEP-NONE: "-mframe-pointer=none"
+// KEEP-RESERVED-NOT: warning: argument unused
+// KEEP-RESERVED: "-mframe-pointer=reserved"
// On Linux x86, omit frame pointer when optimization is enabled.
// RUN: %clang -### --target=i386-linux -S -fomit-frame-pointer %s 2>&1 | \
@@ -215,5 +217,9 @@
// RUN: %clang -### --target=aarch64-none-elf -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
// RUN: FileCheck --check-prefix=KEEP-NON-LEAF %s
+// AArch64 Windows requires that the frame pointer be reserved
+// RUN: %clang -### --target=aarch64-pc-windows-msvc -S -fomit-frame-pointer %s 2>&1 | \
+// RUN: FileCheck --check-prefix=KEEP-RESERVED %s
+
void f0() {}
void f1() { f0(); }
|
There was a problem hiding this comment.
From an ABI perspective, this seems fine: if you want fp to always be valid, sure, lets enforce that.
Do we generate appropriate diagnostics for inline asm?
Does the AArch64 backend actually support this flag? I briefly tried, and it looks like this doesn't actually do anything. I think we only implemented support for 32-bit Arm.
Should we make some corresponding change to LLVM so frontends that don't explicitly request some specific frame-pointer behavior follow the ABI by default?
Good call, I'll have to look into that.
Ugh, I didn't realize that it didn't. I'll go take care of that.
That was going to be my next task. |
|
I've wired through support for "reserved" and added a hardcoding for Windows to match the existing Darwin check, plus expanding the tests for all cases (and added a negative test, since the existing test didn't use I'll do the check for inline asm in a followup (assuming that the current reservation in LLVM isn't sufficient). |
|
|
||
| // These OSes require the frame chain is valid, even if the current frame does | ||
| // not use a frame pointer. | ||
| if (TT.isOSDarwin() || TT.isOSWindows()) |
There was a problem hiding this comment.
With this change, the clang change doesn't actually do anything: we reserve the frame pointer whatever the user requests. Should we just drop the clang change?
There was a problem hiding this comment.
Personally, I'd like to keep the Clang change so that anything else in the frontend understands that frame pointers are reserved (not sure how vital or interesting that is though).
Happy to remove it if someone else has strong opinions.
There was a problem hiding this comment.
I think just removing the clang change will fix the fact that this fails for flang, so it might be worth removing for that reason
There was a problem hiding this comment.
I'm not familiar with how flang works, but if it's going to scrape(?) the command line args from clang, then it needs to be able to handle all the same command line arguments that clang does.
This is not a bug in clang: this is a bug in flang and would be hit if anyone tried building Arm32 code in flang.
|
Failing test is unrelated: #146781 |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/55/builds/13649 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/36791 Here is the relevant piece of the build log for the reference |
DavidSpickett
added a commit
that referenced
this pull request
Jul 3, 2025
…s" (#146836) Reverts #146582 Due to failures on many of Linaro's Linux flang bots: https://lab.llvm.org/buildbot/#/builders/17/builds/9292 ``` ******************** TEST 'Flang :: Semantics/windows.f90' FAILED ******************** Exit Code: 1 Command Output (stdout): -- --- +++ @@ -0,0 +1,2 @@ expect at 6: User IDs do not exist on Windows. This function will always return 1 expect at 11: Group IDs do not exist on Windows. This function will always return 1 FAIL -- Command Output (stderr): -- RUN: at line 1 has no command after substitutions "/usr/bin/python3.10" /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/test_errors.py /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/windows.f90 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/bin/flang --target=aarch64-pc-windows-msvc -Werror # RUN: at line 2 + /usr/bin/python3.10 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/test_errors.py /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/windows.f90 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/bin/flang --target=aarch64-pc-windows-msvc -Werror -- ```
|
I've reverted this due to a test failure on our Linux bots: https://lab.llvm.org/buildbot/#/builders/17/builds/9292 And if I understand correctly, flang needs another patch before it can build properly on Windows: As covered by #146802. I will get the details of the Linux failure. |
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 3, 2025
…rm64 Windows" (#146836) Reverts llvm/llvm-project#146582 Due to failures on many of Linaro's Linux flang bots: https://lab.llvm.org/buildbot/#/builders/17/builds/9292 ``` ******************** TEST 'Flang :: Semantics/windows.f90' FAILED ******************** Exit Code: 1 Command Output (stdout): -- --- +++ @@ -0,0 +1,2 @@ expect at 6: User IDs do not exist on Windows. This function will always return 1 expect at 11: Group IDs do not exist on Windows. This function will always return 1 FAIL -- Command Output (stderr): -- RUN: at line 1 has no command after substitutions "/usr/bin/python3.10" /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/test_errors.py /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/windows.f90 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/bin/flang --target=aarch64-pc-windows-msvc -Werror # RUN: at line 2 + /usr/bin/python3.10 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/test_errors.py /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/llvm/flang/test/Semantics/windows.f90 /home/tcwg-buildbot/worker/clang-aarch64-sve-vla/stage1/bin/flang --target=aarch64-pc-windows-msvc -Werror -- ```
|
The warning: Is because the test looks for the wrong target to be enabled, this will be fixed by: #146869 It's not the cause of the failure. |
|
The actual error is the same as seen elsewhere: It was obscured by this test running the compiler via a Python script. |
dpaoliello
added a commit
that referenced
this pull request
Jul 7, 2025
Fixes `#146802` #146582 started using the `Reserved` Frame Pointer kind for Arm64 Windows, but this revealed a bug in Flang where it copied the `-mframe-pointer=reserved` flag from Clang, but didn't correctly handle it in its own command line parser and subsequent compilation pipeline. This change adds support for `-mframe-pointer=reserved` and adds a test to make sure that functions are correctly marked when the flag is set.
dpaoliello
added a commit
to dpaoliello/llvm-project
that referenced
this pull request
Jul 7, 2025
…146582) There is no way in Arm64 Windows to indicate that a given function has used the Frame Pointer as a General Purpose Register, as such stack walks will always assume that the frame chain is valid and will follow whatever value has been saved for the Frame Pointer (even if it is pointing to data, etc.). This change makes the Frame Pointer always reserved when building for Arm64 Windows to avoid this issue. We will be updating the official Windows ABI documentation to reflect this requirement, and I will provide a link once it's available.
dpaoliello
added a commit
that referenced
this pull request
Jul 8, 2025
#147354) Re-land #146582 now that the Flang bugs have been fixed. There is no way in Arm64 Windows to indicate that a given function has used the Frame Pointer as a General Purpose Register, as such stack walks will always assume that the frame chain is valid and will follow whatever value has been saved for the Frame Pointer (even if it is pointing to data, etc.). This change makes the Frame Pointer always reserved when building for Arm64 Windows to avoid this issue. We will be updating the official Windows ABI documentation to reflect this requirement, and I will provide a link once it's available.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There is no way in Arm64 Windows to indicate that a given function has used the Frame Pointer as a General Purpose Register, as such stack walks will always assume that the frame chain is valid and will follow whatever value has been saved for the Frame Pointer (even if it is pointing to data, etc.).
This change makes the Frame Pointer always reserved when building for Arm64 Windows to avoid this issue.
We will be updating the official Windows ABI documentation to reflect this requirement, and I will provide a link once it's available.
cc @pmsjt