Skip to content

[AArch64] Restrict .variant_pcs directive to ELF targets #138924

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.

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

Merged
merged 3 commits into from
May 16, 2025

Conversation

MacDue
Copy link
Member

@MacDue MacDue commented May 7, 2025

Directive was implemented in c87bd2d to support lazy binding and is emitted for vector PCS functions. It's specific to ELF but is currently emitted for all binary formats and crashing on non-ELF targets.

Fixes #138260

Co-authored-by: Cullen Rhodes cullen.rhodes@arm.com

@llvmbot
Copy link
Member

llvmbot commented May 7, 2025

@llvm/pr-subscribers-backend-aarch64

Author: Benjamin Maxwell (MacDue)

Changes

This was causing crashes on Mach-O targets as we don't construct a TargetStreamer for that object format. Other uses of the TargetStreamer (TS) appear to be limited to ELF and COFF platforms (where it is none null, so don't need changing).


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

2 Files Affected:

  • (modified) llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp (+2-1)
  • (modified) llvm/test/CodeGen/AArch64/variant-pcs.ll (+3)
diff --git a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
index 870df4c387ca4..1166a78bf1088 100644
--- a/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
+++ b/llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
@@ -1372,7 +1372,8 @@ void AArch64AsmPrinter::emitFunctionEntryLabel() {
       MF->getInfo<AArch64FunctionInfo>()->isSVECC()) {
     auto *TS =
         static_cast<AArch64TargetStreamer *>(OutStreamer->getTargetStreamer());
-    TS->emitDirectiveVariantPCS(CurrentFnSym);
+    if (TS)
+      TS->emitDirectiveVariantPCS(CurrentFnSym);
   }
 
   AsmPrinter::emitFunctionEntryLabel();
diff --git a/llvm/test/CodeGen/AArch64/variant-pcs.ll b/llvm/test/CodeGen/AArch64/variant-pcs.ll
index 49c504177358e..0c995b5b0e8ef 100644
--- a/llvm/test/CodeGen/AArch64/variant-pcs.ll
+++ b/llvm/test/CodeGen/AArch64/variant-pcs.ll
@@ -2,6 +2,9 @@
 ; RUN: llc -mtriple=aarch64-linux-gnu -mattr=+sve -filetype=obj -o - %s \
 ; RUN:   | llvm-readobj --symbols - | FileCheck %s --check-prefix=CHECK-OBJ
 
+; Check we don't crash when using a Mach-O object format.
+; RUN: llc -mtriple=arm64-apple-macosx15.0.0 -mattr=+sve -filetype=obj -o /dev/null %s
+
 define i32 @base_pcs() {
 ; CHECK-ASM-LABEL: base_pcs:
 ; CHECK-ASM-NOT: .variant_pcs

Copy link

github-actions bot commented May 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

auto *TS =
static_cast<AArch64TargetStreamer *>(OutStreamer->getTargetStreamer());
TS->emitDirectiveVariantPCS(CurrentFnSym);
if (auto *TS = static_cast<AArch64TargetStreamer *>(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, I think we should only emit the variant PCS attribute when the target's triple is ELF (TM.getTargetTriple().isOSBinFormatELF()) because this is specifically an ELF attribute. This means you can remove this condition, because for ELF targets it should never return a nullptr.

@gbossu
Copy link
Contributor

gbossu commented May 13, 2025

Curious: Why is it that the Mach-O format does not get a TargetStreamer? Does it really need no target-specific support?

MacDue and others added 3 commits May 15, 2025 09:13
This was causing crashes on Mach-O targets as we don't construct a
TargetStreamer for that object format. Other uses of the TargetStreamer
(TS) appear to be limited to ELF and COFF platforms (where it is none
null, so don't need changing).
Directive was implemented in c87bd2d to support lazy binding and is
emitted for vector PCS functions. It's specific to ELF but is currently
emitted for all binary formats and crashing on non-ELF targets.

Fixes llvm#138260

[1]
https://github.com/ARM-software/abi-aa/blob/master/aaelf64/aaelf64.rst#st-other-values
@MacDue MacDue changed the title [AArch64] Null check TargetStreamer before emitting .variant_pcs [AArch64] Restrict .variant_pcs directive to ELF targets May 15, 2025
@MacDue MacDue requested review from c-rhodes and davemgreen May 15, 2025 09:28
@MacDue
Copy link
Member Author

MacDue commented May 15, 2025

Curious: Why is it that the Mach-O format does not get a TargetStreamer? Does it really need no target-specific support?

Presumably not, there's currently only ELF and Windows COFF TargetStreamers. The ELF one does not add much (seems like mainly a few attributes) and the Windows one seems to only handle various unwinding instructions. I guess Mach-O handles these differently (though I don't know the details here).

@MacDue MacDue merged commit 3aaf44f into llvm:main May 16, 2025
11 checks passed
@MacDue MacDue deleted the null_check branch May 16, 2025 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler Crash with ACLE - Aarch64 Apple Darwin targets
5 participants