[Clang] Fix argument extensions in CGBlocks.cpp #111740
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-clang Author: Jonas Paulsson (JonPsson1) ChangesAdd extension attributes in declarations of _Block_object_dispose and _Block_object_assign. In order to make this one-liners wherever needed, a new casting method FunctionCallee::getAsFunction() has been added. A problem is that it seems theoretically possible that the created Function is any other named Value in the module... Would it be ok to have the cast fail in such a case? I realize this is not the approach you had in mind, but it would at least be nice to hear the reasoning as to why something more elaborate would be preferred? Please help with the signedness for the _Block_object_dispose argument, which I have no clue about. @efriedma-quic @nikic Full diff: https://github.com/llvm/llvm-project/pull/111740.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 684fda74407313..0f71163b565d6e 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -2842,6 +2842,8 @@ llvm::FunctionCallee CodeGenModule::getBlockObjectDispose() {
llvm::FunctionType *fty
= llvm::FunctionType::get(VoidTy, args, false);
BlockObjectDispose = CreateRuntimeFunction(fty, "_Block_object_dispose");
+ // FIXME: Correct signedness of extension??
+ BlockObjectDispose.getAsFunction()->addParamAttr(1, llvm::Attribute::SExt);
configureBlocksRuntimeObject(
*this, cast<llvm::Constant>(BlockObjectDispose.getCallee()));
return BlockObjectDispose;
@@ -2855,6 +2857,7 @@ llvm::FunctionCallee CodeGenModule::getBlockObjectAssign() {
llvm::FunctionType *fty
= llvm::FunctionType::get(VoidTy, args, false);
BlockObjectAssign = CreateRuntimeFunction(fty, "_Block_object_assign");
+ BlockObjectAssign.getAsFunction()->addParamAttr(2, llvm::Attribute::SExt);
configureBlocksRuntimeObject(
*this, cast<llvm::Constant>(BlockObjectAssign.getCallee()));
return BlockObjectAssign;
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index a24801d8bdf834..efacca6d773abf 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -29,6 +29,7 @@
namespace llvm {
+class Function;
class Value;
class APInt;
class LLVMContext;
@@ -189,6 +190,8 @@ class FunctionCallee {
explicit operator bool() { return Callee; }
+ Function *getAsFunction();
+
private:
FunctionType *FnTy = nullptr;
Value *Callee = nullptr;
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index f618263f79c313..2252e4fb39fc6a 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -361,6 +361,14 @@ bool FunctionType::isValidArgumentType(Type *ArgTy) {
return ArgTy->isFirstClassType();
}
+//===----------------------------------------------------------------------===//
+// FunctionCallee Implementation
+//===----------------------------------------------------------------------===//
+
+Function* FunctionCallee::getAsFunction() {
+ return cast<llvm::Function>(Callee);
+}
+
//===----------------------------------------------------------------------===//
// StructType Implementation
//===----------------------------------------------------------------------===//
|
|
@llvm/pr-subscribers-clang-codegen Author: Jonas Paulsson (JonPsson1) ChangesAdd extension attributes in declarations of _Block_object_dispose and _Block_object_assign. In order to make this one-liners wherever needed, a new casting method FunctionCallee::getAsFunction() has been added. A problem is that it seems theoretically possible that the created Function is any other named Value in the module... Would it be ok to have the cast fail in such a case? I realize this is not the approach you had in mind, but it would at least be nice to hear the reasoning as to why something more elaborate would be preferred? Please help with the signedness for the _Block_object_dispose argument, which I have no clue about. @efriedma-quic @nikic Full diff: https://github.com/llvm/llvm-project/pull/111740.diff 3 Files Affected:
diff --git a/clang/lib/CodeGen/CGBlocks.cpp b/clang/lib/CodeGen/CGBlocks.cpp
index 684fda74407313..0f71163b565d6e 100644
--- a/clang/lib/CodeGen/CGBlocks.cpp
+++ b/clang/lib/CodeGen/CGBlocks.cpp
@@ -2842,6 +2842,8 @@ llvm::FunctionCallee CodeGenModule::getBlockObjectDispose() {
llvm::FunctionType *fty
= llvm::FunctionType::get(VoidTy, args, false);
BlockObjectDispose = CreateRuntimeFunction(fty, "_Block_object_dispose");
+ // FIXME: Correct signedness of extension??
+ BlockObjectDispose.getAsFunction()->addParamAttr(1, llvm::Attribute::SExt);
configureBlocksRuntimeObject(
*this, cast<llvm::Constant>(BlockObjectDispose.getCallee()));
return BlockObjectDispose;
@@ -2855,6 +2857,7 @@ llvm::FunctionCallee CodeGenModule::getBlockObjectAssign() {
llvm::FunctionType *fty
= llvm::FunctionType::get(VoidTy, args, false);
BlockObjectAssign = CreateRuntimeFunction(fty, "_Block_object_assign");
+ BlockObjectAssign.getAsFunction()->addParamAttr(2, llvm::Attribute::SExt);
configureBlocksRuntimeObject(
*this, cast<llvm::Constant>(BlockObjectAssign.getCallee()));
return BlockObjectAssign;
diff --git a/llvm/include/llvm/IR/DerivedTypes.h b/llvm/include/llvm/IR/DerivedTypes.h
index a24801d8bdf834..efacca6d773abf 100644
--- a/llvm/include/llvm/IR/DerivedTypes.h
+++ b/llvm/include/llvm/IR/DerivedTypes.h
@@ -29,6 +29,7 @@
namespace llvm {
+class Function;
class Value;
class APInt;
class LLVMContext;
@@ -189,6 +190,8 @@ class FunctionCallee {
explicit operator bool() { return Callee; }
+ Function *getAsFunction();
+
private:
FunctionType *FnTy = nullptr;
Value *Callee = nullptr;
diff --git a/llvm/lib/IR/Type.cpp b/llvm/lib/IR/Type.cpp
index f618263f79c313..2252e4fb39fc6a 100644
--- a/llvm/lib/IR/Type.cpp
+++ b/llvm/lib/IR/Type.cpp
@@ -361,6 +361,14 @@ bool FunctionType::isValidArgumentType(Type *ArgTy) {
return ArgTy->isFirstClassType();
}
+//===----------------------------------------------------------------------===//
+// FunctionCallee Implementation
+//===----------------------------------------------------------------------===//
+
+Function* FunctionCallee::getAsFunction() {
+ return cast<llvm::Function>(Callee);
+}
+
//===----------------------------------------------------------------------===//
// StructType Implementation
//===----------------------------------------------------------------------===//
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Manually writing addParamAttr markings doesn't scale; there's no easy way to audit whether everything has been appropriately ported. A corrected API would automatically get the right markings. Also, this isn't the same marking the standard codepath would generate in some cases, which might cause other issues. (Adding sext for int is probably mostly right for existing targets, but it's not obviously right for all targets, and you'd need a different API for any interface that takes an unsigned integer.)
The most likely case is that someone defines an alias... and in that case, I think we need the attribute on the call. |
There was a problem hiding this comment.
I realize this is not the approach you had in mind, but it would at least be nice to hear the reasoning as to why something more elaborate would be preferred?
Basically, because this is an ABI-dependent property, so we should let the existing ABI handling code deal with it.
I can't think of an example where something would go wrong in this particular case, because I don't think any target uses zeroext for int. But for unsigned there is a mix of targets using zeroext or signext (or neither of course).
I have now tried to improve my patch further by instead overloading CreateRuntimeFunction() with a version that can take a sign/zero extension attribute, process it through TargetLibraryInfo to get the right attribute for the target, and then pass this along to the other CreateRuntimeFunction(). "clang::FunctionType" makes me wonder if there is actually a source-language function type available so that the signedness of the extensions could be inferred from it..? I thought these were internal calls to functions that will be emitted directly on IR... This is at least an improvement and it makes it possible to add an attribute with a one-liner wherever needed. |
The signature exists, in the abstract sense: the implementation of the library function is written in C code, so it has a C function signature. It doesn't exist anywhere in the source code, but it's easy to synthesize. On the LLVM side, we use a lower-level abstraction that says, specifically, "this is a signed integer, figure out the correct form of extension" (or "this is an unsigned integer, figure out the extension"). We do this because we don't have a richer type system available, so we need an approximation. We could also do that in clang... but we have the real C type system, so it's probably simpler to just construct the type I mean... I guess we could go with something like the current proposal, but it seems like once we have the CreateRuntimeFunction() taking a clang signature working, it's more readable to just write C types, instead of writing an LLVM type plus a marking for whether the value is supposed to be signed or unsigned. If we are going to go with explicitly passing in the signed-ness, though, I'd prefer a slightly different interface; instead of trying to use something that looks like an attribute, it should just be a simpler list: for each parameter, specify whether it's signed, unsigned, or not an integer. |
|
Patch updated to pass an extension attribute for each retval/parameter, per your suggestion. There may possibly be cases of NoExt (struct-in-reg) that also need to be handled, but I will wait with that until I have a test case. Does this seem ok (for now at least), or would you really prefer to use the C type passed instead? |
|
gentle ping... @efriedma-quic @nikic |
|
Ping! I'd love to get this check enabled by default after fixing the issues I see... This is the only one for Objective-C (the second one here came up after fixing the first one). |
|
I looked to see how hard it would be to lower the clang type... seems like it's not hard. Pushed #113506. Let me know what you think. If there's some complication I'm not seeing, I think your suggested API in the current version of this patch makes sense... but using clang types seems much easier on the caller side. |
I think that seems to work well, thanks. |
|
I will abandon this now since @efriedma-quic's patch (#113506) is the better approach and approved. |
Add extension attributes in declarations of _Block_object_dispose and _Block_object_assign.
In order to make this one-liners wherever needed, a new casting method FunctionCallee::getAsFunction() has been added.
A problem is that it seems theoretically possible that the created Function is any other named Value in the module... Would it be ok to have the cast fail in such a case?
I realize this is not the approach you had in mind, but it would at least be nice to hear the reasoning as to why something more elaborate would be preferred?
Please help with the signedness for the _Block_object_dispose argument, which I have no clue about.
@efriedma-quic @nikic