[Clang] Fix cleanup attribute by delaying type checks after the type is deduced #164440
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Guillot Tony (to268) ChangesPreviously, the handling of the It is also fixed in a way that the solution can be adapted for other attributes that does some type based checks.
NB: Some attributes could have been missed in my shallow search. Fixes #129631 Full diff: https://github.com/llvm/llvm-project/pull/164440.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fe77f917bb801..491cfd3351861 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -447,6 +447,7 @@ Bug Fixes to Attribute Support
- Using ``[[gnu::cleanup(some_func)]]`` where some_func is annotated with
``[[gnu::error("some error")]]`` now correctly triggers an error. (#GH146520)
- Fix a crash when the function name is empty in the `swift_name` attribute. (#GH157075)
+- Fix ``cleanup`` attribute by delaying type checks after the type is deduced. (#GH129631)
Bug Fixes to C++ Support
^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index cb21335ede075..09fb20aa54a6f 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4465,6 +4465,10 @@ class Sema final : public SemaBase {
NamedDecl *New, Decl *Old,
AvailabilityMergeKind AMK = AvailabilityMergeKind::Redeclaration);
+ /// CheckAttributesOnDeducedType - Calls Sema functions for attributes that
+ /// requires the type to be deduced.
+ void CheckAttributesOnDeducedType(Expr *E, Decl *D);
+
/// MergeTypedefNameDecl - We just parsed a typedef 'New' which has the
/// same name and scope as a previous declaration 'Old'. Figure out
/// how to resolve this situation, merging decls or emitting
@@ -15483,6 +15487,9 @@ class Sema final : public SemaBase {
std::optional<FunctionEffectMode>
ActOnEffectExpression(Expr *CondExpr, StringRef AttributeName);
+
+ void ActOnCleanupAttr(Expr *E, Decl *D, const Attr *A);
+
private:
/// The implementation of RequireCompleteType
bool RequireCompleteTypeImpl(SourceLocation Loc, QualType T,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index fc3aabf5741ca..13600218b9d83 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -3354,6 +3354,21 @@ void Sema::mergeDeclAttributes(NamedDecl *New, Decl *Old,
if (!foundAny) New->dropAttrs();
}
+void Sema::CheckAttributesOnDeducedType(Expr *E, Decl *D) {
+ if (!D->hasAttrs())
+ return;
+
+ for (const Attr *A : D->getAttrs()) {
+ switch (A->getKind()) {
+ case attr::Cleanup:
+ ActOnCleanupAttr(E, D, A);
+ break;
+ default:
+ continue;
+ }
+ }
+}
+
// Returns the number of added attributes.
template <class T>
static unsigned propagateAttribute(ParmVarDecl *To, const ParmVarDecl *From,
@@ -13797,6 +13812,8 @@ void Sema::AddInitializerToDecl(Decl *RealDecl, Expr *Init, bool DirectInit) {
return;
}
+ this->CheckAttributesOnDeducedType(Init, RealDecl);
+
// dllimport cannot be used on variable definitions.
if (VDecl->hasAttr<DLLImportAttr>() && !VDecl->isStaticDataMember()) {
Diag(VDecl->getLocation(), diag::err_attribute_dllimport_data_definition);
@@ -14587,6 +14604,7 @@ void Sema::ActOnUninitializedDecl(Decl *RealDecl) {
Var->setInit(RecoveryExpr.get());
}
+ this->CheckAttributesOnDeducedType(Init.get(), RealDecl);
CheckCompleteVariableDeclaration(Var);
}
}
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 9475b8a684082..a60ad17ed1eb0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -3511,16 +3511,6 @@ static void handleCleanupAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
return;
}
- // We're currently more strict than GCC about what function types we accept.
- // If this ever proves to be a problem it should be easy to fix.
- QualType Ty = S.Context.getPointerType(cast<VarDecl>(D)->getType());
- QualType ParamTy = FD->getParamDecl(0)->getType();
- if (!S.IsAssignConvertCompatible(S.CheckAssignmentConstraints(
- FD->getParamDecl(0)->getLocation(), ParamTy, Ty))) {
- S.Diag(Loc, diag::err_attribute_cleanup_func_arg_incompatible_type)
- << NI.getName() << ParamTy << Ty;
- return;
- }
VarDecl *VD = cast<VarDecl>(D);
// Create a reference to the variable declaration. This is a fake/dummy
// reference.
@@ -8291,3 +8281,27 @@ void Sema::redelayDiagnostics(DelayedDiagnosticPool &pool) {
assert(curPool && "re-emitting in undelayed context not supported");
curPool->steal(pool);
}
+
+void Sema::ActOnCleanupAttr(Expr *E, Decl *D, const Attr *A) {
+ FunctionDecl *FD = nullptr;
+ DeclarationNameInfo NI;
+ CleanupAttr *Attr = D->getAttr<CleanupAttr>();
+
+ // Obtains the FunctionDecl that was found when handling the attribute
+ // earlier.
+ FD = Attr->getFunctionDecl();
+ NI = FD->getNameInfo();
+
+ // We're currently more strict than GCC about what function types we accept.
+ // If this ever proves to be a problem it should be easy to fix.
+ QualType Ty = this->Context.getPointerType(cast<VarDecl>(D)->getType());
+ QualType ParamTy = FD->getParamDecl(0)->getType();
+ if (!this->IsAssignConvertCompatible(this->CheckAssignmentConstraints(
+ FD->getParamDecl(0)->getLocation(), ParamTy, Ty))) {
+ this->Diag(Attr->getArgLoc(),
+ diag::err_attribute_cleanup_func_arg_incompatible_type)
+ << NI.getName() << ParamTy << Ty;
+ D->dropAttr<CleanupAttr>();
+ return;
+ }
+}
diff --git a/clang/test/Sema/type-dependent-attrs.c b/clang/test/Sema/type-dependent-attrs.c
new file mode 100644
index 0000000000000..f99293d1cd639
--- /dev/null
+++ b/clang/test/Sema/type-dependent-attrs.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -std=c23 -fsyntax-only -verify %s
+
+// #GH129631-CleanupAttr
+int open() { return 0; }
+void close(typeof(open()) *) {}
+
+void cleanup_attr() {
+ int fd_int [[gnu::cleanup(close)]] = open();
+ auto fd_auto [[gnu::cleanup(close)]] = open();
+ float fd_invalid [[gnu::cleanup(close)]] = open(); // expected-error {{'cleanup' function 'close' parameter has type 'typeof (open()) *' (aka 'int *') which is incompatible with type 'float *'}}
+}
|
erichkeane
left a comment
erichkeane
left a comment
There was a problem hiding this comment.
1 suggestion on how to improve tablegen, else LGTM.
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
LGTM aside from a comment request, feel free to keep or ignore the suggestion.
|
As found by Aaron, there is an additional bug with templated types that will need to be fixed before landing this PR. |
|
@erichkeane I am requesting guidance for fixing this template deduction issue as you are the template code owner and I don't know the template deduction area. |
Thanks to the LLVM-Dev and ISO C++ meetings this week/next week, it'll be a while before I get a chance to take a look. I DO have this on my list of things to take a look at when I return however! If you could, please summarize the problem you're needing solved (with terse examples if possible!) in a way that doesn't require extensive reading of the history of this PR so far. That would very much help me get started on this when I get back. |
|
Basically, it is about the way template type parameters are deduced. int open() { return 0; }
void close(decltype(open()) *) {}
template <typename Ty>
void test() {
Ty fd [[gnu::cleanup(close)]] = open(); // error: 'cleanup' function 'close' parameter has type 'decltype(open()) *' (aka 'int *') which is incompatible with type 'Ty *'
}
int main() {
test<int>();
}The fix would be to trigger the deduction of the template parameter, which should transform the |
Ah, hmm... This seems more to me that the VarDecl has yet to be instantiated at the point of checking (this isn't really deduction at that point?). So I suspect we're checking the 'type' for the purpose of the attribute too soon. I'll have to build locally, but I'll see if I can figure out where we decide on that. But basically, we need to make sure the type is completely instantiated before we check. I would presume this is either a case of passing the wrong type to the attribute checking/wrong declaration, or doing the attribute checking too early. |
^^ Something like that. Please make sure you have tests for the mismatch happening AFTER template instantiation however. |
|
Thank you Erich! |
to268
force-pushed
the
issue-129631
branch
2 times, most recently
from
37a184c to
88d25e8
Compare
Ah right, since it is an 'additional members' you have to do stuff like that manually. Few nits left, else th is is pretty close. |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
Aside from a few nits, I'm happy if @erichkeane is happy
|
I have applied the small changes. |
|
Aaron/I will merge in the morning to make sure we're around to deal with fallout. |
🐧 Linux x64 Test Results
|
|
|
||
| void Sema::ActOnCleanupAttr(Decl *D, const Attr *A) { | ||
| VarDecl *VD = cast<VarDecl>(D); | ||
| if (VD->getType()->isDependentType()) |
There was a problem hiding this comment.
Based on my reading of the documentation in Attr.td this seems like it should be an assert on !VD->getType()->isDependentType() right?
There was a problem hiding this comment.
Well no, if isDependentType() is true we would delay (by exiting the function early) till the template has been instantiated before trying to do type checks. After template instantiation, this function is called again and would perform type checks as usual.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/22460 Here is the relevant piece of the build log for the reference |
6 participants
Previously, the handling of the
cleanupattribute had some checks based on the type, but we were deducing the type after handling the attribute.This PR fixes the way the are dealing with type checks for the
cleanupattribute by delaying these checks after we are deducing the type.It is also fixed in a way that the solution can be adapted for other attributes that does some type based checks.
This is the list of C/C++ attributes that are doing type based checks and will need to be fixed in additional PRs:
NB: Some attributes could have been missed in my shallow search.
Fixes #129631