-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[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
Conversation
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.
@llvm/pr-subscribers-backend-x86 Author: Nikita Popov (nikic) ChangesThis 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:
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
|
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
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.