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.

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

[CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls #89707

Merged
merged 4 commits into from
Apr 29, 2024

Conversation

kees
Copy link
Contributor

@kees kees commented Apr 23, 2024

When building the Linux kernel for i386, the -mregparm=3 option is enabled. Crashes were observed in the sanitizer handler functions, and the problem was found to be mismatched calling convention.

As was fixed in commit c167c0a ("[BuildLibCalls] infer inreg param attrs from NumRegisterParameters"), call arguments need to be marked as "in register" when -mregparm is set. Use the same helper developed there to update the function arguments.

Since CreateRuntimeFunction() is actually part of CodeGenModule, storage of the -mregparm value is also moved to the constructor, as doing this in Release() is too late.

Fixes: #89670

When building the Linux kernel for i386, the -mregparm=3 option is
enabled. Crashes were observed in the sanitizer handler functions,
and the problem was found to be mismatched calling convention.

As was fixed in commit c167c0a ("[BuildLibCalls] infer inreg param
attrs from NumRegisterParameters"), call arguments need to be marked as
"in register" when -mregparm is set. Use the same helper developed there
to update the function arguments.

Since CreateRuntimeFunction() is actually part of CodeGenModule, storage
of the -mregparm value is also moved to the constructor, as doing this
in Release() is too late.

Fixes: llvm#89670
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:codegen llvm:transforms labels Apr 23, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Apr 23, 2024

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Kees Cook (kees)

Changes

When building the Linux kernel for i386, the -mregparm=3 option is enabled. Crashes were observed in the sanitizer handler functions, and the problem was found to be mismatched calling convention.

As was fixed in commit c167c0a ("[BuildLibCalls] infer inreg param attrs from NumRegisterParameters"), call arguments need to be marked as "in register" when -mregparm is set. Use the same helper developed there to update the function arguments.

Since CreateRuntimeFunction() is actually part of CodeGenModule, storage of the -mregparm value is also moved to the constructor, as doing this in Release() is too late.

Fixes: #89670


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

3 Files Affected:

  • (modified) clang/lib/CodeGen/CodeGenModule.cpp (+7-5)
  • (modified) llvm/include/llvm/Transforms/Utils/BuildLibCalls.h (+3)
  • (modified) llvm/lib/Transforms/Utils/BuildLibCalls.cpp (+1-1)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 0c447b20cef40d..c314cd9e2e9f4c 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -73,6 +73,7 @@
 #include "llvm/Support/xxhash.h"
 #include "llvm/TargetParser/Triple.h"
 #include "llvm/TargetParser/X86TargetParser.h"
+#include "llvm/Transforms/Utils/BuildLibCalls.h"
 #include <optional>
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext &C,
       }
     ModuleNameHash = llvm::getUniqueInternalLinkagePostfix(Path);
   }
+
+  // Record mregparm value now so it is visible through all of codegen.
+  if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
+    getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
+                              CodeGenOpts.NumRegisterParameters);
 }
 
 CodeGenModule::~CodeGenModule() {}
@@ -980,11 +986,6 @@ void CodeGenModule::Release() {
       NMD->addOperand(MD);
   }
 
-  // Record mregparm value now so it is visible through rest of codegen.
-  if (Context.getTargetInfo().getTriple().getArch() == llvm::Triple::x86)
-    getModule().addModuleFlag(llvm::Module::Error, "NumRegisterParameters",
-                              CodeGenOpts.NumRegisterParameters);
-
   if (CodeGenOpts.DwarfVersion) {
     getModule().addModuleFlag(llvm::Module::Max, "Dwarf Version",
                               CodeGenOpts.DwarfVersion);
@@ -4781,6 +4782,7 @@ CodeGenModule::CreateRuntimeFunction(llvm::FunctionType *FTy, StringRef Name,
         }
       }
       setDSOLocal(F);
+      markRegisterParameterAttributes(F);
     }
   }
 
diff --git a/llvm/include/llvm/Transforms/Utils/BuildLibCalls.h b/llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
index 9ebb9500777421..a0f2f43e287c71 100644
--- a/llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
+++ b/llvm/include/llvm/Transforms/Utils/BuildLibCalls.h
@@ -62,6 +62,9 @@ namespace llvm {
                      LibFunc TheLibFunc, AttributeList AttributeList,
                      FunctionType *Invalid, ArgsTy... Args) = delete;
 
+  // Handle -mregparm for the given function.
+  void markRegisterParameterAttributes(Function *F);
+
   /// Check whether the library function is available on target and also that
   /// it in the current Module is a Function with the right type.
   bool isLibFuncEmittable(const Module *M, const TargetLibraryInfo *TLI,
diff --git a/llvm/lib/Transforms/Utils/BuildLibCalls.cpp b/llvm/lib/Transforms/Utils/BuildLibCalls.cpp
index ed0ed345435c45..e97506b4bbd95d 100644
--- a/llvm/lib/Transforms/Utils/BuildLibCalls.cpp
+++ b/llvm/lib/Transforms/Utils/BuildLibCalls.cpp
@@ -1255,7 +1255,7 @@ static void setRetExtAttr(Function &F,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
     return;
 

@kees
Copy link
Contributor Author

kees commented Apr 23, 2024

This needs test cases, which I'll add tomorrow. I just wanted to get the core logic up for review before I hit EOD...
(Test cases now added.)

@bwendling bwendling self-requested a review April 23, 2024 05:28
@@ -4781,6 +4782,7 @@ CodeGenModule::CreateRuntimeFunction(llvm::FunctionType *FTy, StringRef Name,
}
}
setDSOLocal(F);
markRegisterParameterAttributes(F);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This really shouldn't work this way: we should go through CodeGenModule::SetLLVMFunctionAttributes to get all the correct attributes for a function. Unfortunately, that's a bit complicated in this context because you need to construct a CGFunctionInfo, and none of the callers currently do that. Which might be more work than you really want... CC @JonPsson1 @jdoerfert @jhuber6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to basically duplicate what was already done for the libcall functions. What is gained by using CGFunctionInfo? (Things already work as-is -- though generally it does looks like -mregparm support is kinda ad-hoc.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I think I see what you mean -- this is the common place where all functions should get their attributes set. Can the libcall code use this? (As in, can we remove markRegisterParameterAttributes() entirely and move the logic into SetLLVMFunctionAttributes() or is that just fantastic overkill?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Zero/sign-extend attributes are also missing, I think. Which probably doesn't affect x86, but could have obscure effects on some targets.

Using SetLLVMFunctionAttributes here isn't really a problem, except that it takes a clang::Type, not an llvm::Type type, and we only have a conversion in the other direction. So you'd need to modify the callers. And there are a lot of callers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To clarify, the code in llvm/ still needs to exist because we need some approximation of the calling convention even if we don't have access to the clang AST. But clang shouldn't use it; clang's native representation of calling conventions is better.

(It's a long-standing request to try to give LLVM a representation of calling conventions that's closer to clang's representation, but it's a large problem, and nobody has really made any progress on it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like a large proposed change; is it worth it for this corner case? I think I would prefer to fix this as I have it now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sign/zero-extend attributes led to real issues on some targets in the past, so it's not unlikely someone will need to address it at some point. But maybe it doesn't need to be here. I guess leave a FIXME noting it as a known issue.

I'm not really happy with exposing this interface on BuildLibCalls.h, since it's really a pretty big footgun (due to the zero/sign-extension issue I mentioned). But copy-pasting the code is worse, so... I guess leave a comment in the header.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment added. Is this what you had in mind?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested text on the clang change:

FIXME: We should use CodeGenModule::SetLLVMFunctionAttributes() instead of trying to approximate the attributes using the LLVM function signature. This requires revising the API of CreateRuntimeFunction().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah-ha, thanks! Okay, I've updated the comments with just a minor tweak.

arinc9 pushed a commit to arinc9/linux that referenced this pull request Apr 26, 2024
When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
// FIXME: This should likely be implemented in
// CodeGenModule::SetLLVMFunctionAttributes() since callers of
// markRegisterParameterAttributes() will not have gotten appropriate
// attributes for things like sign/zero-extension, etc.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't really properly capture the divide between clang, the frontend, and LLVM the optimizer.

What we want here is basically a warning: this interface shouldn't be used unless you know exactly what you're doing. Then a FIXME on the clang side to indicate we plan to revise the CreateRuntimeFunction() interface at some point.

Suggested text on the LLVM API:

Note that this function is a rough approximation that only works for simple function signatures. Note this does not apply other relevant attributes for function signatures, including sign/zero-extension for arguments and return values.

@@ -4781,6 +4782,7 @@ CodeGenModule::CreateRuntimeFunction(llvm::FunctionType *FTy, StringRef Name,
}
}
setDSOLocal(F);
markRegisterParameterAttributes(F);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested text on the clang change:

FIXME: We should use CodeGenModule::SetLLVMFunctionAttributes() instead of trying to approximate the attributes using the LLVM function signature. This requires revising the API of CreateRuntimeFunction().

roxell pushed a commit to roxell/linux that referenced this pull request Apr 29, 2024
When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Copy link
Collaborator

@efriedma-quic efriedma-quic left a comment

Choose a reason for hiding this comment

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

LGTM

@kees kees merged commit 869ffcf into llvm:main Apr 29, 2024
4 checks passed
@kees kees deleted the fix-runtime-regparm branch April 29, 2024 21:54
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 22, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 23, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 24, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 25, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 26, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 26, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
johnny-mnemonic pushed a commit to linux-ia64/linux-stable-rc that referenced this pull request Jun 27, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Whissi pushed a commit to Whissi/linux-stable that referenced this pull request Jun 27, 2024
[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
tuxedo-bot pushed a commit to tuxedocomputers/linux that referenced this pull request Aug 7, 2024
BugLink: https://bugs.launchpad.net/bugs/2075154

[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Manuel Diewald <manuel.diewald@canonical.com>
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
tuxedo-bot pushed a commit to tuxedocomputers/linux that referenced this pull request Aug 13, 2024
BugLink: https://bugs.launchpad.net/bugs/2075154

[ Upstream commit 2e431b2 ]

When generating Runtime Calls, Clang doesn't respect the -mregparm=3
option used on i386. Hopefully this will be fixed correctly in Clang 19:
llvm/llvm-project#89707
but we need to fix this for earlier Clang versions today. Force the
calling convention to use non-register arguments.

Reported-by: Erhard Furtner <erhard_f@mailbox.org>
Closes: KSPP/linux#350
Link: https://lore.kernel.org/r/20240424224026.it.216-kees@kernel.org
Acked-by: Nathan Chancellor <nathan@kernel.org>
Acked-by: Justin Stitt <justinstitt@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
Signed-off-by: Manuel Diewald <manuel.diewald@canonical.com>
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
Signed-off-by: Roxana Nicolescu <roxana.nicolescu@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:codegen clang Clang issues not falling into any other category llvm:transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanitizer handler calls emitted without regard to -mregparm
3 participants