[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

https://github.com/kees closed https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

https://github.com/efriedma-quic approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

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

kees wrote:

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

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

https://github.com/kees updated https://github.com/llvm/llvm-project/pull/89707

>From c061c8f49f2b916bb5e60ec35d3e448ac13f2b72 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Mon, 22 Apr 2024 17:53:32 -0700
Subject: [PATCH 1/4] [CodeGen][i386] Move -mregparm storage earlier and fix
 Runtime calls

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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/89670
---
 clang/lib/CodeGen/CodeGenModule.cpp| 12 +++-
 llvm/include/llvm/Transforms/Utils/BuildLibCalls.h |  3 +++
 llvm/lib/Transforms/Utils/BuildLibCalls.cpp|  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

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 
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext ,
   }
 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 ,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
 return;
 

>From b7abf7c63e7169096401b958b767520a5301e417 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Tue, 23 Apr 2024 17:16:36 -0700
Subject: [PATCH 2/4] add tests for -mregparm vs runtime calls

---
 clang/test/CodeGen/regparm-flag.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/clang/test/CodeGen/regparm-flag.c 
b/clang/test/CodeGen/regparm-flag.c
index c35b53cd4e1990..d888c1e344c03b 100644
--- a/clang/test/CodeGen/regparm-flag.c
+++ b/clang/test/CodeGen/regparm-flag.c
@@ -1,4 +1,8 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 4 %s -emit-llvm -o - 
| FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fsanitize=array-bounds %s 
-emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME0
+// RUN: %clang_cc1 -triple 

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

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

efriedma-quic wrote:

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().

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

@@ -62,6 +62,13 @@ namespace llvm {
  LibFunc TheLibFunc, AttributeList AttributeList,
  FunctionType *Invalid, ArgsTy... Args) = delete;
 
+  // Handle -mregparm for the given function.
+  // 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.

efriedma-quic wrote:

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.

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

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

kees wrote:

Comment added. Is this what you had in mind?

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

https://github.com/kees updated https://github.com/llvm/llvm-project/pull/89707

>From c061c8f49f2b916bb5e60ec35d3e448ac13f2b72 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Mon, 22 Apr 2024 17:53:32 -0700
Subject: [PATCH 1/3] [CodeGen][i386] Move -mregparm storage earlier and fix
 Runtime calls

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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/89670
---
 clang/lib/CodeGen/CodeGenModule.cpp| 12 +++-
 llvm/include/llvm/Transforms/Utils/BuildLibCalls.h |  3 +++
 llvm/lib/Transforms/Utils/BuildLibCalls.cpp|  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

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 
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext ,
   }
 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 ,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
 return;
 

>From b7abf7c63e7169096401b958b767520a5301e417 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Tue, 23 Apr 2024 17:16:36 -0700
Subject: [PATCH 2/3] add tests for -mregparm vs runtime calls

---
 clang/test/CodeGen/regparm-flag.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/clang/test/CodeGen/regparm-flag.c 
b/clang/test/CodeGen/regparm-flag.c
index c35b53cd4e1990..d888c1e344c03b 100644
--- a/clang/test/CodeGen/regparm-flag.c
+++ b/clang/test/CodeGen/regparm-flag.c
@@ -1,4 +1,8 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 4 %s -emit-llvm -o - 
| FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fsanitize=array-bounds %s 
-emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME0
+// RUN: %clang_cc1 -triple 

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

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

efriedma-quic wrote:

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.

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

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

kees wrote:

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.

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

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

efriedma-quic wrote:

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.)

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

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

efriedma-quic wrote:

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.

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

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

kees wrote:

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?

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

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

kees wrote:

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.)

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

https://github.com/kees updated https://github.com/llvm/llvm-project/pull/89707

>From c061c8f49f2b916bb5e60ec35d3e448ac13f2b72 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Mon, 22 Apr 2024 17:53:32 -0700
Subject: [PATCH 1/2] [CodeGen][i386] Move -mregparm storage earlier and fix
 Runtime calls

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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/89670
---
 clang/lib/CodeGen/CodeGenModule.cpp| 12 +++-
 llvm/include/llvm/Transforms/Utils/BuildLibCalls.h |  3 +++
 llvm/lib/Transforms/Utils/BuildLibCalls.cpp|  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

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 
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext ,
   }
 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 ,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
 return;
 

>From b7abf7c63e7169096401b958b767520a5301e417 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Tue, 23 Apr 2024 17:16:36 -0700
Subject: [PATCH 2/2] add tests for -mregparm vs runtime calls

---
 clang/test/CodeGen/regparm-flag.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/clang/test/CodeGen/regparm-flag.c 
b/clang/test/CodeGen/regparm-flag.c
index c35b53cd4e1990..d888c1e344c03b 100644
--- a/clang/test/CodeGen/regparm-flag.c
+++ b/clang/test/CodeGen/regparm-flag.c
@@ -1,4 +1,8 @@
 // RUN: %clang_cc1 -triple i386-unknown-unknown -mregparm 4 %s -emit-llvm -o - 
| FileCheck %s
+// RUN: %clang_cc1 -triple i386-unknown-unknown -fsanitize=array-bounds %s 
-emit-llvm -o - | FileCheck %s --check-prefix=RUNTIME0
+// RUN: %clang_cc1 -triple 

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Eli Friedman via cfe-commits]

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

efriedma-quic wrote:

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

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

kees wrote:

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

https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[via cfe-commits]

llvmbot wrote:




@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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/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
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 
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext ,
   }
 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 ,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
 return;
 

``




https://github.com/llvm/llvm-project/pull/89707
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [CodeGen][i386] Move -mregparm storage earlier and fix Runtime calls (PR #89707)

[Kees Cook via cfe-commits]

https://github.com/kees created https://github.com/llvm/llvm-project/pull/89707

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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/89670

>From c061c8f49f2b916bb5e60ec35d3e448ac13f2b72 Mon Sep 17 00:00:00 2001
From: Kees Cook 
Date: Mon, 22 Apr 2024 17:53:32 -0700
Subject: [PATCH] [CodeGen][i386] Move -mregparm storage earlier and fix
 Runtime calls

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 c167c0a4dcdb ("[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: https://github.com/llvm/llvm-project/issues/89670
---
 clang/lib/CodeGen/CodeGenModule.cpp| 12 +++-
 llvm/include/llvm/Transforms/Utils/BuildLibCalls.h |  3 +++
 llvm/lib/Transforms/Utils/BuildLibCalls.cpp|  2 +-
 3 files changed, 11 insertions(+), 6 deletions(-)

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 
 
 using namespace clang;
@@ -442,6 +443,11 @@ CodeGenModule::CodeGenModule(ASTContext ,
   }
 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 ,
 }
 
 // Modeled after X86TargetLowering::markLibCallAttributes.
-static void markRegisterParameterAttributes(Function *F) {
+void llvm::markRegisterParameterAttributes(Function *F) {
   if (!F->arg_size() || F->isVarArg())
 return;
 

___
cfe-commits mailing list
cfe-commits@lists.llvm.org