[clang] 514e435 - inline stmt attribute diagnosing in templates
Erich Keane via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 21 08:16:59 PDT 2023
Author: Erich Keane
Date: 2023-03-21T08:16:52-07:00
New Revision: 514e4359a543ea778c7fee6908a9c6eb10d0ccd9
URL: https://github.com/llvm/llvm-project/commit/514e4359a543ea778c7fee6908a9c6eb10d0ccd9
DIFF: https://github.com/llvm/llvm-project/commit/514e4359a543ea778c7fee6908a9c6eb10d0ccd9.diff
LOG: inline stmt attribute diagnosing in templates
D146089's author discovered that our diagnostics for always/no inline
would null-dereference when used in a template. He fixed that by
skipping in the dependent case.
This patch makes sure we diagnose these after a template instantiation.
It also adds infrastructure for other statement attributes to add
checking/transformation.
Differential Revision: https://reviews.llvm.org/D146323
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaStmtAttr.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp
clang/lib/Sema/TreeTransform.h
clang/test/Sema/attr-alwaysinline.cpp
clang/test/Sema/attr-noinline.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e2e4d6f51d81a..6ae71683804d4 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -181,6 +181,8 @@ Improvements to Clang's diagnostics
- Clang now avoids duplicate warnings on unreachable ``[[fallthrough]];`` statements
previously issued from ``-Wunreachable-code`` and ``-Wunreachable-code-fallthrough``
by prioritizing ``-Wunreachable-code-fallthrough``.
+- Clang now correctly diagnoses statement attributes ``[[clang::always_inine]]`` and
+ ``[[clang::noinline]]`` when used on a statement with dependent call expressions.
Bug Fixes in This Version
-------------------------
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 7ff10a9d52e56..63ee0f0ed7fb6 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -4715,6 +4715,11 @@ class Sema final {
void CheckAlignasUnderalignment(Decl *D);
+ bool CheckNoInlineAttr(const Stmt *OrigSt, const Stmt *CurSt,
+ const AttributeCommonInfo &A);
+ bool CheckAlwaysInlineAttr(const Stmt *OrigSt, const Stmt *CurSt,
+ const AttributeCommonInfo &A);
+
/// Adjust the calling convention of a method to be the ABI default if it
/// wasn't specified explicitly. This handles method types formed from
/// function type typedefs and typename template arguments.
diff --git a/clang/lib/Sema/SemaStmtAttr.cpp b/clang/lib/Sema/SemaStmtAttr.cpp
index eeef85373ccb1..50cb5b50aa0de 100644
--- a/clang/lib/Sema/SemaStmtAttr.cpp
+++ b/clang/lib/Sema/SemaStmtAttr.cpp
@@ -215,6 +215,59 @@ static Attr *handleNoMergeAttr(Sema &S, Stmt *St, const ParsedAttr &A,
return ::new (S.Context) NoMergeAttr(S.Context, A);
}
+template <typename OtherAttr, int DiagIdx>
+static bool CheckStmtInlineAttr(Sema &SemaRef, const Stmt *OrigSt,
+ const Stmt *CurSt,
+ const AttributeCommonInfo &A) {
+ CallExprFinder OrigCEF(SemaRef, OrigSt);
+ CallExprFinder CEF(SemaRef, CurSt);
+
+ // If the call expressions lists are equal in size, we can skip
+ // previously emitted diagnostics. However, if the statement has a pack
+ // expansion, we have no way of telling which CallExpr is the instantiated
+ // version of the other. In this case, we will end up re-diagnosing in the
+ // instantiation.
+ // ie: [[clang::always_inline]] non_dependent(), (other_call<Pack>()...)
+ // will diagnose nondependent again.
+ bool CanSuppressDiag =
+ OrigSt && CEF.getCallExprs().size() == OrigCEF.getCallExprs().size();
+
+ if (!CEF.foundCallExpr()) {
+ return SemaRef.Diag(CurSt->getBeginLoc(),
+ diag::warn_attribute_ignored_no_calls_in_stmt)
+ << A;
+ }
+
+ for (auto Tup :
+ llvm::zip_longest(OrigCEF.getCallExprs(), CEF.getCallExprs())) {
+ // If the original call expression already had a callee, we already
+ // diagnosed this, so skip it here. We can't skip if there isn't a 1:1
+ // relationship between the two lists of call expressions.
+ if (!CanSuppressDiag || !(*std::get<0>(Tup))->getCalleeDecl()) {
+ const Decl *Callee = (*std::get<1>(Tup))->getCalleeDecl();
+ if (Callee &&
+ (Callee->hasAttr<OtherAttr>() || Callee->hasAttr<FlattenAttr>())) {
+ SemaRef.Diag(CurSt->getBeginLoc(),
+ diag::warn_function_stmt_attribute_precedence)
+ << A << (Callee->hasAttr<OtherAttr>() ? DiagIdx : 1);
+ SemaRef.Diag(Callee->getBeginLoc(), diag::note_conflicting_attribute);
+ }
+ }
+ }
+
+ return false;
+}
+
+bool Sema::CheckNoInlineAttr(const Stmt *OrigSt, const Stmt *CurSt,
+ const AttributeCommonInfo &A) {
+ return CheckStmtInlineAttr<AlwaysInlineAttr, 0>(*this, OrigSt, CurSt, A);
+}
+
+bool Sema::CheckAlwaysInlineAttr(const Stmt *OrigSt, const Stmt *CurSt,
+ const AttributeCommonInfo &A) {
+ return CheckStmtInlineAttr<NoInlineAttr, 2>(*this, OrigSt, CurSt, A);
+}
+
static Attr *handleNoInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
SourceRange Range) {
NoInlineAttr NIA(S.Context, A);
@@ -224,20 +277,8 @@ static Attr *handleNoInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
return nullptr;
}
- CallExprFinder CEF(S, St);
- if (!CEF.foundCallExpr()) {
- S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
- << A;
+ if (S.CheckNoInlineAttr(/*OrigSt=*/nullptr, St, A))
return nullptr;
- }
-
- for (const auto *CallExpr : CEF.getCallExprs()) {
- const Decl *Decl = CallExpr->getCalleeDecl();
- if (Decl &&
- (Decl->hasAttr<AlwaysInlineAttr>() || Decl->hasAttr<FlattenAttr>()))
- S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
- << A << (Decl->hasAttr<AlwaysInlineAttr>() ? 0 : 1);
- }
return ::new (S.Context) NoInlineAttr(S.Context, A);
}
@@ -251,19 +292,8 @@ static Attr *handleAlwaysInlineAttr(Sema &S, Stmt *St, const ParsedAttr &A,
return nullptr;
}
- CallExprFinder CEF(S, St);
- if (!CEF.foundCallExpr()) {
- S.Diag(St->getBeginLoc(), diag::warn_attribute_ignored_no_calls_in_stmt)
- << A;
+ if (S.CheckAlwaysInlineAttr(/*OrigSt=*/nullptr, St, A))
return nullptr;
- }
-
- for (const auto *CallExpr : CEF.getCallExprs()) {
- const Decl *Decl = CallExpr->getCalleeDecl();
- if (Decl && (Decl->hasAttr<NoInlineAttr>() || Decl->hasAttr<FlattenAttr>()))
- S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence)
- << A << (Decl->hasAttr<NoInlineAttr>() ? 2 : 1);
- }
return ::new (S.Context) AlwaysInlineAttr(S.Context, A);
}
diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index b4649ce4c413c..162cf3cd88344 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -1272,6 +1272,12 @@ namespace {
bool AllowInjectedClassName = false);
const LoopHintAttr *TransformLoopHintAttr(const LoopHintAttr *LH);
+ const NoInlineAttr *TransformStmtNoInlineAttr(const Stmt *OrigS,
+ const Stmt *InstS,
+ const NoInlineAttr *A);
+ const AlwaysInlineAttr *
+ TransformStmtAlwaysInlineAttr(const Stmt *OrigS, const Stmt *InstS,
+ const AlwaysInlineAttr *A);
ExprResult TransformPredefinedExpr(PredefinedExpr *E);
ExprResult TransformDeclRefExpr(DeclRefExpr *E);
@@ -1767,6 +1773,20 @@ TemplateInstantiator::TransformLoopHintAttr(const LoopHintAttr *LH) {
return LoopHintAttr::CreateImplicit(getSema().Context, LH->getOption(),
LH->getState(), TransformedExpr, *LH);
}
+const NoInlineAttr *TemplateInstantiator::TransformStmtNoInlineAttr(
+ const Stmt *OrigS, const Stmt *InstS, const NoInlineAttr *A) {
+ if (!A || getSema().CheckNoInlineAttr(OrigS, InstS, *A))
+ return nullptr;
+
+ return A;
+}
+const AlwaysInlineAttr *TemplateInstantiator::TransformStmtAlwaysInlineAttr(
+ const Stmt *OrigS, const Stmt *InstS, const AlwaysInlineAttr *A) {
+ if (!A || getSema().CheckAlwaysInlineAttr(OrigS, InstS, *A))
+ return nullptr;
+
+ return A;
+}
ExprResult TemplateInstantiator::transformNonTypeTemplateParmRef(
Decl *AssociatedDecl, const NonTypeTemplateParmDecl *parm,
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 21789f96f9154..6dacd74a99e3b 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -377,22 +377,43 @@ class TreeTransform {
/// By default, this routine transforms a statement by delegating to the
/// appropriate TransformXXXAttr function to transform a specific kind
/// of attribute. Subclasses may override this function to transform
- /// attributed statements using some other mechanism.
+ /// attributed statements/types using some other mechanism.
///
/// \returns the transformed attribute
const Attr *TransformAttr(const Attr *S);
-/// Transform the specified attribute.
-///
-/// Subclasses should override the transformation of attributes with a pragma
-/// spelling to transform expressions stored within the attribute.
-///
-/// \returns the transformed attribute.
-#define ATTR(X)
-#define PRAGMA_SPELLING_ATTR(X) \
+ // Transform the given statement attribute.
+ //
+ // Delegates to the appropriate TransformXXXAttr function to transform a
+ // specific kind of statement attribute. Unlike the non-statement taking
+ // version of this, this implements all attributes, not just pragmas.
+ const Attr *TransformStmtAttr(const Stmt *OrigS, const Stmt *InstS,
+ const Attr *A);
+
+ // Transform the specified attribute.
+ //
+ // Subclasses should override the transformation of attributes with a pragma
+ // spelling to transform expressions stored within the attribute.
+ //
+ // \returns the transformed attribute.
+#define ATTR(X) \
const X##Attr *Transform##X##Attr(const X##Attr *R) { return R; }
#include "clang/Basic/AttrList.inc"
+ // Transform the specified attribute.
+ //
+ // Subclasses should override the transformation of attributes to do
+ // transformation and checking of statement attributes. By default, this
+ // delegates to the non-statement taking version.
+ //
+ // \returns the transformed attribute.
+#define ATTR(X) \
+ const X##Attr *TransformStmt##X##Attr(const Stmt *, const Stmt *, \
+ const X##Attr *A) { \
+ return getDerived().Transform##X##Attr(A); \
+ }
+#include "clang/Basic/AttrList.inc"
+
/// Transform the given expression.
///
/// By default, this routine transforms an expression by delegating to the
@@ -7551,9 +7572,8 @@ const Attr *TreeTransform<Derived>::TransformAttr(const Attr *R) {
return R;
switch (R->getKind()) {
-// Transform attributes with a pragma spelling by calling TransformXXXAttr.
-#define ATTR(X)
-#define PRAGMA_SPELLING_ATTR(X) \
+// Transform attributes by calling TransformXXXAttr.
+#define ATTR(X) \
case attr::X: \
return getDerived().Transform##X##Attr(cast<X##Attr>(R));
#include "clang/Basic/AttrList.inc"
@@ -7562,25 +7582,45 @@ const Attr *TreeTransform<Derived>::TransformAttr(const Attr *R) {
}
}
+template <typename Derived>
+const Attr *TreeTransform<Derived>::TransformStmtAttr(const Stmt *OrigS,
+ const Stmt *InstS,
+ const Attr *R) {
+ if (!R)
+ return R;
+
+ switch (R->getKind()) {
+// Transform attributes by calling TransformStmtXXXAttr.
+#define ATTR(X) \
+ case attr::X: \
+ return getDerived().TransformStmt##X##Attr(OrigS, InstS, cast<X##Attr>(R));
+#include "clang/Basic/AttrList.inc"
+ default:
+ return R;
+ }
+ return TransformAttr(R);
+}
+
template <typename Derived>
StmtResult
TreeTransform<Derived>::TransformAttributedStmt(AttributedStmt *S,
StmtDiscardKind SDK) {
+ StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt(), SDK);
+ if (SubStmt.isInvalid())
+ return StmtError();
+
bool AttrsChanged = false;
SmallVector<const Attr *, 1> Attrs;
// Visit attributes and keep track if any are transformed.
for (const auto *I : S->getAttrs()) {
- const Attr *R = getDerived().TransformAttr(I);
+ const Attr *R =
+ getDerived().TransformStmtAttr(S->getSubStmt(), SubStmt.get(), I);
AttrsChanged |= (I != R);
if (R)
Attrs.push_back(R);
}
- StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt(), SDK);
- if (SubStmt.isInvalid())
- return StmtError();
-
if (SubStmt.get() == S->getSubStmt() && !AttrsChanged)
return S;
diff --git a/clang/test/Sema/attr-alwaysinline.cpp b/clang/test/Sema/attr-alwaysinline.cpp
index be3f74c8bfa9d..f60cb5e3adfb1 100644
--- a/clang/test/Sema/attr-alwaysinline.cpp
+++ b/clang/test/Sema/attr-alwaysinline.cpp
@@ -3,8 +3,9 @@
int bar();
[[gnu::always_inline]] void always_inline_fn(void) {}
+// expected-note at +1{{conflicting attribute is here}}
[[gnu::flatten]] void flatten_fn(void) {}
-
+// expected-note at +1{{conflicting attribute is here}}
[[gnu::noinline]] void noinline_fn(void) {}
void foo() {
@@ -32,9 +33,44 @@ int foo(int x) {
[[clang::always_inline]] return foo<D-1>(x + 1);
}
-// FIXME: This should warn that always_inline statement attribute has higher
-// precedence than the noinline function attribute.
+template<int D>
+[[gnu::noinline]]
+int dependent(int x){ return x + D;} // #DEP
+[[gnu::noinline]]
+int non_dependent(int x){return x;} // #NO_DEP
+
template<int D> [[gnu::noinline]]
-int bar(int x) {
- [[clang::always_inline]] return bar<D-1>(x + 1);
+int baz(int x) { // #BAZ
+ // expected-warning at +2{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+ // expected-note@#NO_DEP{{conflicting attribute is here}}
+ [[clang::always_inline]] non_dependent(x);
+ if constexpr (D>0) {
+ // expected-warning at +6{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+ // expected-note@#NO_DEP{{conflicting attribute is here}}
+ // expected-warning at +4 3{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+ // expected-note@#BAZ 3{{conflicting attribute is here}}
+ // expected-note@#BAZ_INST 3{{in instantiation}}
+ // expected-note at +1 3{{in instantiation}}
+ [[clang::always_inline]] return non_dependent(x), baz<D-1>(x + 1);
+ }
+ return x;
+}
+
+// We can't suppress if there is a variadic involved.
+template<int ... D>
+int variadic_baz(int x) {
+ // Diagnoses NO_DEP 2x, once during phase 1, the second during instantiation.
+ // Dianoses DEP 3x, once per variadic expansion.
+ // expected-warning at +5 2{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+ // expected-note@#NO_DEP 2{{conflicting attribute is here}}
+ // expected-warning at +3 3{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}}
+ // expected-note@#DEP 3{{conflicting attribute is here}}
+ // expected-note@#VARIADIC_INST{{in instantiation}}
+ [[clang::always_inline]] return non_dependent(x) + (dependent<D>(x) + ...);
+}
+
+void use() {
+ baz<3>(0); // #BAZ_INST
+ variadic_baz<0, 1, 2>(0); // #VARIADIC_INST
+
}
diff --git a/clang/test/Sema/attr-noinline.cpp b/clang/test/Sema/attr-noinline.cpp
index ae0f80ca296eb..b5c3fa1536681 100644
--- a/clang/test/Sema/attr-noinline.cpp
+++ b/clang/test/Sema/attr-noinline.cpp
@@ -2,9 +2,10 @@
int bar();
+// expected-note at +1{{conflicting attribute is here}}
[[gnu::always_inline]] void always_inline_fn(void) { }
+// expected-note at +1{{conflicting attribute is here}}
[[gnu::flatten]] void flatten_fn(void) { }
-
[[gnu::noinline]] void noinline_fn(void) { }
void foo() {
@@ -32,9 +33,43 @@ int foo(int x) {
[[clang::noinline]] return foo<D-1>(x + 1);
}
-// FIXME: This should warn that noinline statement attribute has higher
-// precedence than the always_inline function attribute.
+template<int D>
+[[clang::always_inline]]
+int dependent(int x){ return x + D;} // #DEP
+[[clang::always_inline]]
+int non_dependent(int x){return x;} // #NO_DEP
+
template<int D> [[clang::always_inline]]
-int bar(int x) {
- [[clang::noinline]] return bar<D-1>(x + 1);
+int baz(int x) { // #BAZ
+ // expected-warning at +2{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+ // expected-note@#NO_DEP{{conflicting attribute is here}}
+ [[clang::noinline]] non_dependent(x);
+ if constexpr (D>0) {
+ // expected-warning at +6{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+ // expected-note@#NO_DEP{{conflicting attribute is here}}
+ // expected-warning at +4 3{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+ // expected-note@#BAZ 3{{conflicting attribute is here}}
+ // expected-note@#BAZ_INST 3{{in instantiation}}
+ // expected-note at +1 3{{in instantiation}}
+ [[clang::noinline]] return non_dependent(x), baz<D-1>(x + 1);
+ }
+ return x;
+}
+
+// We can't suppress if there is a variadic involved.
+template<int ... D>
+int variadic_baz(int x) {
+ // Diagnoses NO_DEP 2x, once during phase 1, the second during instantiation.
+ // Dianoses DEP 3x, once per variadic expansion.
+ // expected-warning at +5 2{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+ // expected-note@#NO_DEP 2{{conflicting attribute is here}}
+ // expected-warning at +3 3{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}}
+ // expected-note@#DEP 3{{conflicting attribute is here}}
+ // expected-note@#VARIADIC_INST{{in instantiation}}
+ [[clang::noinline]] return non_dependent(x) + (dependent<D>(x) + ...);
+}
+
+void use() {
+ baz<3>(0); // #BAZ_INST
+ variadic_baz<0, 1, 2>(0); // #VARIADIC_INST
}
More information about the cfe-commits
mailing list