Skip to content

[X86][KCFI] Do not require linux triple for kcfi-arity #148207

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 1 commit into from
Jul 11, 2025

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Jul 11, 2025

This code doesn't assume the Linux ABI, but the standard x86-64 SysV ABI, which is used (with minor variations) by all non-Windows targets.

Requiring "linux" as the OS here is problematic, because the actual OS (as opposed to users of the OS) is generally compiled against the "none" target.

This code doesn't assume the Linux ABI, but the standard x86-64
SysV ABI, which is used (with minor variations) by all non-Windows
targets.

Requiring "linux" as the OS here is problematic, because the actual
OS (as opposed to users of the OS) is generally compiled against the
"none" target.
@llvmbot
Copy link
Member

llvmbot commented Jul 11, 2025

@llvm/pr-subscribers-backend-x86

Author: Nikita Popov (nikic)

Changes

This code doesn't assume the Linux ABI, but the standard x86-64 SysV ABI, which is used (with minor variations) by all non-Windows targets.

Requiring "linux" as the OS here is problematic, because the actual OS (as opposed to users of the OS) is generally compiled against the "none" target.


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

2 Files Affected:

  • (modified) llvm/lib/Target/X86/X86AsmPrinter.cpp (+2-2)
  • (modified) llvm/test/CodeGen/X86/kcfi-arity.ll (+1)
diff --git a/llvm/lib/Target/X86/X86AsmPrinter.cpp b/llvm/lib/Target/X86/X86AsmPrinter.cpp
index 50c20fcde49ce..d406277e440bb 100644
--- a/llvm/lib/Target/X86/X86AsmPrinter.cpp
+++ b/llvm/lib/Target/X86/X86AsmPrinter.cpp
@@ -192,9 +192,9 @@ void X86AsmPrinter::emitKCFITypeId(const MachineFunction &MF) {
   unsigned DestReg = X86::EAX;
 
   if (F.getParent()->getModuleFlag("kcfi-arity")) {
-    // The ArityToRegMap assumes the 64-bit Linux kernel ABI
+    // The ArityToRegMap assumes the 64-bit SysV ABI.
     [[maybe_unused]] const auto &Triple = MF.getTarget().getTargetTriple();
-    assert(Triple.isArch64Bit() && Triple.isOSLinux());
+    assert(Triple.isArch64Bit() && !Triple.isOSWindows());
 
     // Determine the function's arity (i.e., the number of arguments) at the ABI
     // level by counting the number of parameters that are passed
diff --git a/llvm/test/CodeGen/X86/kcfi-arity.ll b/llvm/test/CodeGen/X86/kcfi-arity.ll
index 009fa7d2dc0a4..5a19bcd7835ea 100644
--- a/llvm/test/CodeGen/X86/kcfi-arity.ll
+++ b/llvm/test/CodeGen/X86/kcfi-arity.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs < %s | FileCheck %s --check-prefix=ASM
+; RUN: llc -mtriple=x86_64-unknown-none -verify-machineinstrs < %s | FileCheck %s --check-prefix=ASM
 ; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -stop-after=finalize-isel < %s | FileCheck %s --check-prefixes=MIR,ISEL
 ; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs -stop-after=kcfi < %s | FileCheck %s --check-prefixes=MIR,KCFI
 

@nikic nikic mentioned this pull request Jul 11, 2025
4 tasks
@@ -1,4 +1,5 @@
; RUN: llc -mtriple=x86_64-unknown-linux-gnu -verify-machineinstrs < %s | FileCheck %s --check-prefix=ASM
; RUN: llc -mtriple=x86_64-unknown-none -verify-machineinstrs < %s | FileCheck %s --check-prefix=ASM
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't change existing triple to none directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to check that linux-gnu still works as well.

Copy link
Contributor

@phoebewang phoebewang left a comment

Choose a reason for hiding this comment

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

LGTM.

@nikic nikic merged commit c5acb3d into llvm:main Jul 11, 2025
11 checks passed
@nikic nikic deleted the kcfi-arity-none branch July 11, 2025 13:03
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.

3 participants