Author: John Paul Jepko Date: 2026-03-24T12:33:33+08:00 New Revision: fd11cf430e5a9fd11f93bdcc929a1ce8dfa60f37
URL: https://github.com/llvm/llvm-project/commit/fd11cf430e5a9fd11f93bdcc929a1ce8dfa60f37 DIFF: https://github.com/llvm/llvm-project/commit/fd11cf430e5a9fd11f93bdcc929a1ce8dfa60f37.diff LOG: [clang] Extend -Wunused-but-set-variable to static globals (#178342) This PR extends the capability of -Wunused-but-set-variable to track and diagnose static global variables that are assigned values within a function but whose values are never used. This change complements -Wunused-variable, which detects static globals that are neither set nor used. I created this change with the help of claude for some initial guidance. Fixes #148361 Added: clang/test/Sema/Inputs/warn-unused-but-set-static-global-header.h clang/test/Sema/warn-unused-but-set-static-global.c clang/test/Sema/warn-unused-but-set-static-global.cpp Modified: clang/docs/ReleaseNotes.rst clang/include/clang/AST/Decl.h clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaExpr.cpp clang/lib/Sema/SemaExprCXX.cpp clang/test/C/C2y/n3622.c clang/test/SemaCXX/warn-variable-not-needed.cpp Removed: ################################################################################ diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 6e6c580fcb0ff..6a2632543d337 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -230,6 +230,9 @@ Attribute Changes in Clang Improvements to Clang's diagnostics ----------------------------------- +- ``-Wunused-but-set-variable`` now diagnoses file-scope variables with + internal linkage (``static`` storage class) that are assigned but never used. (#GH148361) + - Added ``-Wlifetime-safety`` to enable lifetime safety analysis, a CFG-based intra-procedural analysis that detects use-after-free and related temporal safety bugs. See the diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h index c3cd74a5b34db..076d9ba935583 100644 --- a/clang/include/clang/AST/Decl.h +++ b/clang/include/clang/AST/Decl.h @@ -1212,6 +1212,21 @@ class VarDecl : public DeclaratorDecl, public Redeclarable<VarDecl> { && !isFileVarDecl(); } + /// Returns true if this is a file-scope variable with internal linkage. + bool isInternalLinkageFileVar() const { + // Calling isExternallyVisible() can trigger linkage computation/caching, + // which may produce stale results when a decl's DeclContext changes after + // creation (e.g., OpenMP declare mapper variables), so here we determine + // it syntactically instead. + if (!isFileVarDecl()) + return false; + // Linkage is determined by enclosing class/namespace for static data + // members. + if (getStorageClass() == SC_Static && !isStaticDataMember()) + return true; + return isInAnonymousNamespace(); + } + /// Returns true if a variable has extern or __private_extern__ /// storage. bool hasExternalStorage() const { diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 832e46286194a..a214a7aa9147b 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -973,6 +973,10 @@ class Sema final : public SemaBase { /// unit because its type has no linkage and it's not extern "C". bool isExternalWithNoLinkageType(const ValueDecl *VD) const; + /// Determines whether the given source location is in the main file + /// and we're in a context where we should warn about unused entities. + bool isMainFileLoc(SourceLocation Loc) const; + /// Obtain a sorted list of functions that are undefined but ODR-used. void getUndefinedButUsed( SmallVectorImpl<std::pair<NamedDecl *, SourceLocation>> &Undefined); @@ -7009,6 +7013,9 @@ class Sema final : public SemaBase { /// Increment when we find a reference; decrement when we find an ignored /// assignment. Ultimately the value is 0 if every reference is an ignored /// assignment. + /// + /// Uses canonical VarDecl as key so in-class decls and out-of-class defs of + /// static data members get tracked as a single entry. llvm::DenseMap<const VarDecl *, int> RefsMinusAssignments; /// Used to control the generation of ExprWithCleanups. diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index 826277c445a1d..620b1352841d1 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -956,6 +956,12 @@ bool Sema::isExternalWithNoLinkageType(const ValueDecl *VD) const { !isFunctionOrVarDeclExternC(VD); } +bool Sema::isMainFileLoc(SourceLocation Loc) const { + if (TUKind != TU_Complete || getLangOpts().IsHeaderFile) + return false; + return SourceMgr.isInMainFile(Loc); +} + /// Obtains a sorted list of functions and variables that are undefined but /// ODR-used. void Sema::getUndefinedButUsed( @@ -1604,6 +1610,40 @@ void Sema::ActOnEndOfTranslationUnit() { emitAndClearUnusedLocalTypedefWarnings(); } + if (!Diags.isIgnored(diag::warn_unused_but_set_variable, SourceLocation())) { + // Diagnose unused-but-set static globals in a deterministic order. + // Not tracking shadowing info for static globals; there's nothing to + // shadow. + struct LocAndDiag { + SourceLocation Loc; + PartialDiagnostic PD; + }; + SmallVector<LocAndDiag, 16> DeclDiags; + auto addDiag = [&DeclDiags](SourceLocation Loc, PartialDiagnostic PD) { + DeclDiags.push_back(LocAndDiag{Loc, std::move(PD)}); + }; + + // For -Wunused-but-set-variable we only care about variables that were + // referenced by the TU end. + for (const auto &Ref : RefsMinusAssignments) { + const VarDecl *VD = Ref.first; + // Only diagnose internal linkage file vars defined in the main file to + // match -Wunused-variable behavior and avoid false positives from + // headers. + if (VD->isInternalLinkageFileVar() && isMainFileLoc(VD->getLocation())) + DiagnoseUnusedButSetDecl(VD, addDiag); + } + + llvm::sort(DeclDiags, + [](const LocAndDiag &LHS, const LocAndDiag &RHS) -> bool { + // Sorting purely for determinism; matches behavior in + // Sema::ActOnPopScope. + return LHS.Loc < RHS.Loc; + }); + for (const LocAndDiag &D : DeclDiags) + Diag(D.Loc, D.PD); + } + if (!Diags.isIgnored(diag::warn_unused_private_field, SourceLocation())) { // FIXME: Load additional unused private field candidates from the external // source. diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 52946e0ca3cb1..c862c090c50a8 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -1912,14 +1912,6 @@ bool Sema::mightHaveNonExternalLinkage(const DeclaratorDecl *D) { return !D->isExternallyVisible(); } -// FIXME: This needs to be refactored; some other isInMainFile users want -// these semantics. -static bool isMainFileLoc(const Sema &S, SourceLocation Loc) { - if (S.TUKind != TU_Complete || S.getLangOpts().IsHeaderFile) - return false; - return S.SourceMgr.isInMainFile(Loc); -} - bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const { assert(D); @@ -1946,7 +1938,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const { return false; } else { // 'static inline' functions are defined in headers; don't warn. - if (FD->isInlined() && !isMainFileLoc(*this, FD->getLocation())) + if (FD->isInlined() && !isMainFileLoc(FD->getLocation())) return false; } @@ -1957,7 +1949,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const { // Constants and utility variables are defined in headers with internal // linkage; don't warn. (Unlike functions, there isn't a convenient marker // like "inline".) - if (!isMainFileLoc(*this, VD->getLocation())) + if (!isMainFileLoc(VD->getLocation())) return false; if (Context.DeclMustBeEmitted(VD)) @@ -1971,7 +1963,7 @@ bool Sema::ShouldWarnIfUnusedFileScopedDecl(const DeclaratorDecl *D) const { VD->getMemberSpecializationInfo() && !VD->isOutOfLine()) return false; - if (VD->isInline() && !isMainFileLoc(*this, VD->getLocation())) + if (VD->isInline() && !isMainFileLoc(VD->getLocation())) return false; } else { return false; @@ -2215,6 +2207,11 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD, return; } + // Don't warn on volatile file-scope variables. They are visible beyond their + // declaring function and writes to them could be observable side effects. + if (VD->getType().isVolatileQualified() && VD->isFileVarDecl()) + return; + // Don't warn about __block Objective-C pointer variables, as they might // be assigned in the block but not used elsewhere for the purpose of lifetime // extension. @@ -2227,7 +2224,7 @@ void Sema::DiagnoseUnusedButSetDecl(const VarDecl *VD, if (VD->hasAttr<ObjCPreciseLifetimeAttr>() && Ty->isObjCObjectPointerType()) return; - auto iter = RefsMinusAssignments.find(VD); + auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl()); if (iter == RefsMinusAssignments.end()) return; @@ -2305,9 +2302,11 @@ void Sema::ActOnPopScope(SourceLocation Loc, Scope *S) { DiagnoseUnusedDecl(D, addDiag); if (const auto *RD = dyn_cast<RecordDecl>(D)) DiagnoseUnusedNestedTypedefs(RD, addDiag); - if (VarDecl *VD = dyn_cast<VarDecl>(D)) { + // Wait until end of TU to diagnose internal linkage file vars. + if (auto *VD = dyn_cast<VarDecl>(D); + VD && !VD->isInternalLinkageFileVar()) { DiagnoseUnusedButSetDecl(VD, addDiag); - RefsMinusAssignments.erase(VD); + RefsMinusAssignments.erase(VD->getCanonicalDecl()); } } diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp index 2b0a786302aab..fa1cd1ebb52de 100644 --- a/clang/lib/Sema/SemaExpr.cpp +++ b/clang/lib/Sema/SemaExpr.cpp @@ -20477,8 +20477,15 @@ static void DoMarkVarDeclReferenced( bool UsableInConstantExpr = Var->mightBeUsableInConstantExpressions(SemaRef.Context); - if (Var->isLocalVarDeclOrParm() && !Var->hasExternalStorage()) { - RefsMinusAssignments.insert({Var, 0}).first->getSecond()++; + // Only track variables with internal linkage or local scope. + // Use canonical decl so in-class declarations and out-of-class definitions + // of static data members in anonymous namespaces are tracked as a single + // entry. + const VarDecl *CanonVar = Var->getCanonicalDecl(); + if ((CanonVar->isLocalVarDeclOrParm() || + CanonVar->isInternalLinkageFileVar()) && + !CanonVar->hasExternalStorage()) { + RefsMinusAssignments.insert({CanonVar, 0}).first->getSecond()++; } // C++20 [expr.const]p12: diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp index 5553f25546c38..08bc6aa483844 100644 --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -7504,7 +7504,7 @@ static void MaybeDecrementCount( if ((IsCompoundAssign || isIncrementDecrementUnaryOp) && VD->getType().isVolatileQualified()) return; - auto iter = RefsMinusAssignments.find(VD); + auto iter = RefsMinusAssignments.find(VD->getCanonicalDecl()); if (iter == RefsMinusAssignments.end()) return; iter->getSecond()--; diff --git a/clang/test/C/C2y/n3622.c b/clang/test/C/C2y/n3622.c index 95b92e8f235a8..d90b0c51d3ccf 100644 --- a/clang/test/C/C2y/n3622.c +++ b/clang/test/C/C2y/n3622.c @@ -1,7 +1,7 @@ -// RUN: %clang_cc1 -verify=good -pedantic -Wall -std=c2y %s -// RUN: %clang_cc1 -verify=compat,expected -pedantic -Wall -Wpre-c2y-compat -std=c2y %s -// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -std=c23 %s -// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -std=c17 %s +// RUN: %clang_cc1 -verify=good -pedantic -Wall -Wno-unused-but-set-variable -std=c2y %s +// RUN: %clang_cc1 -verify=compat,expected -pedantic -Wall -Wno-unused-but-set-variable -Wpre-c2y-compat -std=c2y %s +// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -Wno-unused-but-set-variable -std=c23 %s +// RUN: %clang_cc1 -verify=ped,expected -pedantic -Wall -Wno-unused-but-set-variable -std=c17 %s // good-no-diagnostics /* WG14 N3622: Clang 22 diff --git a/clang/test/Sema/Inputs/warn-unused-but-set-static-global-header.h b/clang/test/Sema/Inputs/warn-unused-but-set-static-global-header.h new file mode 100644 index 0000000000000..40637d644f717 --- /dev/null +++ b/clang/test/Sema/Inputs/warn-unused-but-set-static-global-header.h @@ -0,0 +1,3 @@ +// Header file for testing that header-defined static globals don't warn. +static int header_set_unused = 0; +static void header_init() { header_set_unused = 1; } diff --git a/clang/test/Sema/warn-unused-but-set-static-global.c b/clang/test/Sema/warn-unused-but-set-static-global.c new file mode 100644 index 0000000000000..7c4d20a42b2d2 --- /dev/null +++ b/clang/test/Sema/warn-unused-but-set-static-global.c @@ -0,0 +1,132 @@ +// RUN: %clang_cc1 -fsyntax-only -Wunused-but-set-variable -I %S/Inputs -verify %s + +// Test that header-defined static globals don't warn. +#include "warn-unused-but-set-static-global-header.h" + +#define NULL (void*)0 + +void *set(int size); +void func_call(void *); + +static int set_unused; // expected-warning {{variable 'set_unused' set but not used}} +static int set_and_used; +static int only_used; +static int addr_taken; +extern int external_var; // No warning (external linkage). +extern int global_var; // No warning (not static). + +void f1() { + set_unused = 1; + set_and_used = 2; + + int x = set_and_used; + (void)x; + + int y = only_used; + (void)y; + + int *p = &addr_taken; + (void)p; + + external_var = 3; + global_var = 4; +} + +// Test across multiple functions. +static int set_used1; +static int set_used2; + +static int set1; // expected-warning {{variable 'set1' set but not used}} +static int set2; // expected-warning {{variable 'set2' set but not used}} + +void f2() { + set1 = 1; + set_used1 = 1; + + int x = set_used2; + (void)x; +} + +void f3() { + set2 = 2; + set_used2 = 2; + + int x = set_used1; + (void)x; +} + +static volatile int vol_set; +void f4() { + vol_set = 1; +} + +// Read and use +static int compound; // expected-warning{{variable 'compound' set but not used}} +static volatile int vol_compound; +static int unary; // expected-warning{{variable 'unary' set but not used}} +static volatile int vol_unary; +void f5() { + compound += 1; + vol_compound += 1; + unary++; + vol_unary++; +} + +struct S { + int i; +}; +static struct S s_set; // expected-warning{{variable 's_set' set but not used}} +static struct S s_used; +void f6() { + struct S t; + s_set = t; + t = s_used; +} + +// Multiple assignments +static int multi; // expected-warning{{variable 'multi' set but not used}} +void f7() { + multi = 1; + multi = 2; + multi = 3; +} + +// Unused pointers +static int *unused_ptr; // expected-warning{{variable 'unused_ptr' set but not used}} +static char *str_ptr; // expected-warning{{variable 'str_ptr' set but not used}} +void f8() { + unused_ptr = set(5); + str_ptr = "hello"; +} + +// Used pointers +void a(void *); +static int *used_ptr; +static int *param_ptr; +static int *null_check_ptr; +void f9() { + used_ptr = set(5); + *used_ptr = 5; + + param_ptr = set(5); + func_call(param_ptr); + + null_check_ptr = set(5); + if (null_check_ptr == NULL) {} +} + +// Function pointers (unused) +static void (*unused_func_ptr)(); // expected-warning {{variable 'unused_func_ptr' set but not used}} +void SetUnusedCallback(void (*f)()) { + unused_func_ptr = f; +} + +// Function pointers (used) +static void (*used_func_ptr)(); +void SetUsedCallback(void (*f)()) { + used_func_ptr = f; +} +void CallUsedCallback() { + if (used_func_ptr) + used_func_ptr(); +} diff --git a/clang/test/Sema/warn-unused-but-set-static-global.cpp b/clang/test/Sema/warn-unused-but-set-static-global.cpp new file mode 100644 index 0000000000000..1ddd7eb72ce29 --- /dev/null +++ b/clang/test/Sema/warn-unused-but-set-static-global.cpp @@ -0,0 +1,165 @@ +// RUN: %clang_cc1 -fsyntax-only -Wunused-but-set-variable -verify -std=c++17 %s + +static thread_local int tl_set_unused; // expected-warning {{variable 'tl_set_unused' set but not used}} +static thread_local int tl_set_and_used; +thread_local int tl_no_static_set_unused; + +// Warning should respect attributes. +[[maybe_unused]] static int with_maybe_unused; +__attribute__((unused)) static int with_unused_attr; + +void f0() { + tl_set_unused = 1; + tl_set_and_used = 2; + int x = tl_set_and_used; + (void)x; + + tl_no_static_set_unused = 3; + + with_maybe_unused = 4; + with_unused_attr = 5; +} + +// Named namespace. +namespace test { + static int set_unused; // expected-warning {{variable 'set_unused' set but not used}} + static int set_and_used; + + void f1() { + set_unused = 1; + set_and_used = 2; + int x = set_and_used; + (void)x; + } +} + +// Nested named namespace. +namespace outer { +namespace inner { +static int nested_unused; // expected-warning {{variable 'nested_unused' set but not used}} +void f2() { + nested_unused = 5; +} +} +} + +// Anonymous namespace (all vars have internal linkage). +namespace { + int anon_ns_unused; // expected-warning {{variable 'anon_ns_unused' set but not used}} + static int anon_ns_static_unused; // expected-warning {{variable 'anon_ns_static_unused' set but not used}} + + void f3() { + anon_ns_unused = 1; + anon_ns_static_unused = 2; + } +} + +// Function pointers at file scope. +static void (*unused_func_ptr)(); // expected-warning {{variable 'unused_func_ptr' set but not used}} +void SetUnusedCallback(void (*f)()) { + unused_func_ptr = f; +} + +static void (*used_func_ptr)(); +void SetUsedCallback(void (*f)()) { + used_func_ptr = f; +} +void CallUsedCallback() { + if (used_func_ptr) + used_func_ptr(); +} + +// Static data members (external linkage, should not warn). +class MyClass { +public: + static int unused_static_member; + static int used_static_member; +}; + +int MyClass::unused_static_member = 0; +int MyClass::used_static_member = 0; + +void f4() { + MyClass::unused_static_member = 10; + + MyClass::used_static_member = 20; + int x = MyClass::used_static_member; + (void)x; +} + +// Static data members in a named namespace (external linkage, should not warn). +namespace named { + struct NamedClass { + static int w; + }; + int NamedClass::w = 0; +} + +void f5() { + named::NamedClass::w = 4; +} + +// Static data members in anonymous namespace (internal linkage, should warn). +namespace { + class AnonClass { + public: + static int unused_member; // expected-warning {{variable 'unused_member' set but not used}} + static int used_member; + }; + + int AnonClass::unused_member = 0; + int AnonClass::used_member = 0; +} + +void f6() { + AnonClass::unused_member = 3; + AnonClass::used_member = 4; + int y = AnonClass::used_member; + (void)y; +} + +// Static data members in nested anonymous namespace (internal linkage, should warn). +namespace outer2 { + namespace { + struct NestedAnonClass { + static int v; // expected-warning {{variable 'v' set but not used}} + }; + int NestedAnonClass::v = 0; + } +} + +void f7() { + outer2::NestedAnonClass::v = 5; +} + +// Static data members set inside methods, read outside. +namespace { + struct SetInMethod { + static int x; + static int y; // expected-warning {{variable 'y' set but not used}} + void setX() { x = 1; } + void setY() { y = 1; } + }; + int SetInMethod::x; + int SetInMethod::y; +} + +void f8() { + SetInMethod s; + s.setX(); + s.setY(); + int v = SetInMethod::x; + (void)v; // only x is read +} + +// External linkage static data members set inside methods. +struct ExtSetInMethod { + static int x; + void set() { x = 1; } +}; +int ExtSetInMethod::x; + +void f9() { + ExtSetInMethod e; + e.set(); +} diff --git a/clang/test/SemaCXX/warn-variable-not-needed.cpp b/clang/test/SemaCXX/warn-variable-not-needed.cpp index 103be189068f8..272c8998d15c0 100644 --- a/clang/test/SemaCXX/warn-variable-not-needed.cpp +++ b/clang/test/SemaCXX/warn-variable-not-needed.cpp @@ -18,7 +18,7 @@ namespace test2 { }; namespace { struct foo : bah { - static char bar; + static char bar; // expected-warning {{variable 'bar' set but not used}} virtual void zed(); }; void foo::zed() { _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
