diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -181,6 +181,8 @@ - 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 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -4715,6 +4715,11 @@ 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 --- a/clang/lib/Sema/SemaStmtAttr.cpp +++ b/clang/lib/Sema/SemaStmtAttr.cpp @@ -215,6 +215,59 @@ return ::new (S.Context) NoMergeAttr(S.Context, A); } +template +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()...) + // 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() || Callee->hasAttr())) { + SemaRef.Diag(CurSt->getBeginLoc(), + diag::warn_function_stmt_attribute_precedence) + << A << (Callee->hasAttr() ? 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(*this, OrigSt, CurSt, A); +} + +bool Sema::CheckAlwaysInlineAttr(const Stmt *OrigSt, const Stmt *CurSt, + const AttributeCommonInfo &A) { + return CheckStmtInlineAttr(*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 @@ 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() || Decl->hasAttr())) - S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence) - << A << (Decl->hasAttr() ? 0 : 1); - } return ::new (S.Context) NoInlineAttr(S.Context, A); } @@ -251,19 +292,8 @@ 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() || Decl->hasAttr())) - S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence) - << A << (Decl->hasAttr() ? 2 : 1); - } return ::new (S.Context) AlwaysInlineAttr(S.Context, A); } diff --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp b/clang/lib/Sema/SemaTemplateInstantiate.cpp --- a/clang/lib/Sema/SemaTemplateInstantiate.cpp +++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp @@ -1272,6 +1272,12 @@ 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 @@ 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 --- a/clang/lib/Sema/TreeTransform.h +++ b/clang/lib/Sema/TreeTransform.h @@ -377,22 +377,43 @@ /// 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 @@ 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(R)); #include "clang/Basic/AttrList.inc" @@ -7562,25 +7582,45 @@ } } +template +const Attr *TreeTransform::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(R)); +#include "clang/Basic/AttrList.inc" + default: + return R; + } + return TransformAttr(R); +} + template StmtResult TreeTransform::TransformAttributedStmt(AttributedStmt *S, StmtDiscardKind SDK) { + StmtResult SubStmt = getDerived().TransformStmt(S->getSubStmt(), SDK); + if (SubStmt.isInvalid()) + return StmtError(); + bool AttrsChanged = false; SmallVector 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 --- 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@+1{{conflicting attribute is here}} [[gnu::flatten]] void flatten_fn(void) {} - +// expected-note@+1{{conflicting attribute is here}} [[gnu::noinline]] void noinline_fn(void) {} void foo() { @@ -32,9 +33,44 @@ [[clang::always_inline]] return foo(x + 1); } -// FIXME: This should warn that always_inline statement attribute has higher -// precedence than the noinline function attribute. +template +[[gnu::noinline]] +int dependent(int x){ return x + D;} // #DEP +[[gnu::noinline]] +int non_dependent(int x){return x;} // #NO_DEP + template [[gnu::noinline]] -int bar(int x) { - [[clang::always_inline]] return bar(x + 1); +int baz(int x) { // #BAZ + // expected-warning@+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@+6{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}} + // expected-note@#NO_DEP{{conflicting attribute is here}} + // expected-warning@+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@+1 3{{in instantiation}} + [[clang::always_inline]] return non_dependent(x), baz(x + 1); + } + return x; +} + +// We can't suppress if there is a variadic involved. +template +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@+5 2{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}} + // expected-note@#NO_DEP 2{{conflicting attribute is here}} + // expected-warning@+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(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 --- a/clang/test/Sema/attr-noinline.cpp +++ b/clang/test/Sema/attr-noinline.cpp @@ -2,9 +2,10 @@ int bar(); +// expected-note@+1{{conflicting attribute is here}} [[gnu::always_inline]] void always_inline_fn(void) { } +// expected-note@+1{{conflicting attribute is here}} [[gnu::flatten]] void flatten_fn(void) { } - [[gnu::noinline]] void noinline_fn(void) { } void foo() { @@ -32,9 +33,43 @@ [[clang::noinline]] return foo(x + 1); } -// FIXME: This should warn that noinline statement attribute has higher -// precedence than the always_inline function attribute. +template +[[clang::always_inline]] +int dependent(int x){ return x + D;} // #DEP +[[clang::always_inline]] +int non_dependent(int x){return x;} // #NO_DEP + template [[clang::always_inline]] -int bar(int x) { - [[clang::noinline]] return bar(x + 1); +int baz(int x) { // #BAZ + // expected-warning@+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@+6{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}} + // expected-note@#NO_DEP{{conflicting attribute is here}} + // expected-warning@+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@+1 3{{in instantiation}} + [[clang::noinline]] return non_dependent(x), baz(x + 1); + } + return x; +} + +// We can't suppress if there is a variadic involved. +template +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@+5 2{{statement attribute 'noinline' has higher precedence than function attribute 'always_inline'}} + // expected-note@#NO_DEP 2{{conflicting attribute is here}} + // expected-warning@+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(x) + ...); +} + +void use() { + baz<3>(0); // #BAZ_INST + variadic_baz<0, 1, 2>(0); // #VARIADIC_INST }