llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: ahmedkamel10 (AhmedKamel10) <details> <summary>Changes</summary> Add -Winvalid-pure-attribute, which flags common violations of the GCC-documented 'pure'/'const' attribute contract: direct writes through a pointer or reference parameter, and writes to global or static-local variables. This is a syntactic heuristic over the function's own body, not a soundness proof (interprocedural analysis for this is undecidable in general). Fixes #<!-- -->215106 --- Full diff: https://github.com/llvm/llvm-project/pull/217817.diff 4 Files Affected: - (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1) - (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+13) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+70) - (modified) clang/test/Sema/attr-const-pure.c (+53-13) ``````````diff diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 9ee0b61a96a32..5742be7a9e143 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -756,6 +756,7 @@ def ExpansionToDefined : DiagGroup<"expansion-to-defined">; def FlagEnum : DiagGroup<"flag-enum">; def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>; def InfiniteRecursion : DiagGroup<"infinite-recursion">; +def InvalidPureAttribute : DiagGroup<"invalid-pure-attribute">; def PureVirtualCallFromCtorDtor: DiagGroup<"call-to-pure-virtual-from-ctor-dtor">; def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">; def IgnoredGCH : DiagGroup<"ignored-gch">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 22e65d3205808..ee31fe72a81b0 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -811,6 +811,19 @@ def warn_const_attr_with_pure_attr : Warning< def warn_pure_function_returns_void : Warning< "'%select{pure|const}0' attribute on function returning 'void'; attribute ignored">, InGroup<IgnoredAttributes>; + +def warn_pure_function_writes_argument : Warning< + "function declared %select{'pure'|'const'}0 stores through " + "%select{pointer|reference}1 parameter %2; this violates the attribute's " + "contract and can cause miscompilation under optimization">, + InGroup<InvalidPureAttribute>; +def warn_pure_function_writes_global : Warning< + "function declared %select{'pure'|'const'}0 stores to " + "%select{global|static local}1 variable %2; this violates the attribute's " + "contract and can cause miscompilation under optimization">, + InGroup<InvalidPureAttribute>; +def note_pure_function_declared_here : Note< + "function declared %select{'pure'|'const'}0 here">; def warn_suggest_noreturn_function : Warning< "%select{function|method}0 %1 could be declared with attribute 'noreturn'">, diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index d0500a6defd64..9d2e2fdd61729 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -284,6 +284,76 @@ static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) { return foundRecursion; } +namespace { +/// Walks a 'pure'/'const' function body looking for stores through pointer +/// or reference parameters, and stores to global or static-local variables. +/// This is a syntactic heuristic, not a proof: it only catches direct writes +/// reachable via straightforward lvalue expressions in the function's own +/// body. It intentionally does not attempt interprocedural analysis (that's +/// undecidable in general), locally-allocated memory escaping, or writes +/// mediated through a level of indirection the visitor doesn't unwrap. +class PureConstWriteChecker : public DynamicRecursiveASTVisitor { +public: + PureConstWriteChecker(Sema &sema, const FunctionDecl *functionDecl, + bool isConstAttr) + : S(sema), FD(functionDecl), IsConstAttr(isConstAttr) {} + + bool VisitBinaryOperator(BinaryOperator *BO) override { + if (BO->isAssignmentOp() || BO->isCompoundAssignmentOp()) + checkLvalue(BO->getLHS()); + + return true; + } + +private: + Sema &S; + const FunctionDecl *FD; + bool IsConstAttr; + + void checkLvalue(const Expr *E) { + E = E->IgnoreParenImpCasts(); + if (const auto *UO = dyn_cast<UnaryOperator>(E)) { + if (UO->getOpcode() == UO_Deref) { + checkPointerBase(UO->getSubExpr()); + return; + } + } + if (const auto *DRE = dyn_cast<DeclRefExpr>(E)) { + if (const auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) { + if (const auto *PVD = dyn_cast<ParmVarDecl>(VD)) { + if (PVD->getType()->isReferenceType() && + !PVD->getType()->getPointeeType().isConstQualified()) { + + S.Diag(DRE->getLocation(), diag::warn_pure_function_writes_argument) + << IsConstAttr << true << PVD; + S.Diag(FD->getLocation(), diag::note_pure_function_declared_here) + << IsConstAttr; + } + } else if (VD->hasGlobalStorage()) { + S.Diag(DRE->getLocation(), diag::warn_pure_function_writes_global) + << IsConstAttr << VD->isStaticLocal() << VD; + S.Diag(FD->getLocation(), diag::note_pure_function_declared_here) + << IsConstAttr; + } + } + } + } + + void checkPointerBase(const Expr *Base) { + Base = Base->IgnoreParenCasts(); + const auto *DRE = dyn_cast<DeclRefExpr>(Base); + if (!DRE) + return; + const auto *PVD = dyn_cast<ParmVarDecl>(DRE->getDecl()); + if (!PVD) + return; + S.Diag(DRE->getLocation(), diag::warn_pure_function_writes_argument) + << IsConstAttr << false << PVD; + S.Diag(FD->getLocation(), diag::note_pure_function_declared_here) + << IsConstAttr; + } +}; +} // namespace static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD, const Stmt *Body, AnalysisDeclContext &AC) { FD = FD->getCanonicalDecl(); diff --git a/clang/test/Sema/attr-const-pure.c b/clang/test/Sema/attr-const-pure.c index 43e22eb34014d..ca8b057b30d0f 100644 --- a/clang/test/Sema/attr-const-pure.c +++ b/clang/test/Sema/attr-const-pure.c @@ -1,5 +1,5 @@ -// RUN: %clang_cc1 -fsyntax-only -verify %s -// RUN: %clang_cc1 -fsyntax-only -verify -x c++ %s +// RUN: %clang_cc1 -fsyntax-only -verify -Winvalid-pure-attribute %s +// RUN: %clang_cc1 -fsyntax-only -verify -Winvalid-pure-attribute -x c++ %s // The attributes apply to function declarations, nothing else. __attribute__((const)) int func1(void); @@ -10,16 +10,14 @@ __attribute__((pure)) int func2(void); #ifdef __cplusplus struct CppTest { - // They are fine on member functions. - __attribute__((const)) int func(); +// They are fine on member functions. +__attribute__((const)) int func(); [[gnu::pure]] int other_func(); - - int another(); - - // Constructors and destructors are not allowed though because they - // notionally return void. +int another(); +// Constructors and destructors are not allowed though because they +// notionally return void. [[__gnu__::__const__]] CppTest(); // expected-warning {{'const' attribute on function returning 'void'; attribute ignored}} - __attribute__((pure)) ~CppTest(); // expected-warning {{'pure' attribute on function returning 'void'; attribute ignored}} +__attribute__((pure)) ~CppTest(); // expected-warning {{'pure' attribute on function returning 'void'; attribute ignored}} }; // Including out-of-line member functions. @@ -40,11 +38,11 @@ int (*fp1)(void) [[gnu::const]]; // expected-warning {{attribute 'gnu::const' ig int (*fp2)(void) [[gnu::pure]]; // expected-warning {{attribute 'gnu::pure' ignored, because it cannot be applied to a type}} struct __attribute__((const)) S1 { // expected-warning {{'const' attribute only applies to functions}} - int x; +int x; }; struct __attribute__((pure)) S2 { // expected-warning {{'pure' attribute only applies to functions}} - int x; +int x; }; // Or variables, etc. @@ -61,6 +59,48 @@ __attribute__((const, pure)) int func7(void); // expected-warning {{'const' attr // FIXME: this should also be diagnosed the same as func7. __attribute__((pure)) int func8(void); [[gnu::const]] int func8(void) { - return 12; +return 12; } +// Diagnosing invalid 'pure'/'const' attributes: writes through a pointer or +// reference parameter, or to a global/static variable, violate the +// attribute's no-visible-side-effects contract. + +int lookup(int key, int *value) __attribute__((__pure__)); +int lookup(int key, int *value) { // expected-note {{function declared 'pure' here}} + if (key == 42) { + *value = 47; // expected-warning {{function declared 'pure' stores through pointer parameter 'value'}} + return 0; + } + return -1; +} + +static int global_cache; +__attribute__((pure)) int writes_global(int x) { // expected-note {{function declared 'pure' here}} + global_cache = x; // expected-warning {{function declared 'pure' stores to global variable 'global_cache'}} + return global_cache; +} + +__attribute__((pure)) int writes_static_local(int x) { // expected-note {{function declared 'pure' here}} + static int cache = 0; + cache += x; // expected-warning {{function declared 'pure' stores to static local variable 'cache'}} + return cache; +} + +// No warning: local, non-static variable. +__attribute__((pure)) int local_only(int x) { + int tmp = x * 2; + return tmp; +} + +// No warning: read-only dereference of a const pointer. +__attribute__((pure)) int reads_ptr_arg(const int *value) { + return *value; +} + +#ifdef __cplusplus +__attribute__((pure)) int writes_ref_arg(int &out) { // expected-note {{function declared 'pure' here}} + out = 1; // expected-warning {{function declared 'pure' stores through reference parameter 'out'}} + return 0; +} +#endif \ No newline at end of file `````````` </details> https://github.com/llvm/llvm-project/pull/217817 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
