https://github.com/bjosv updated https://github.com/llvm/llvm-project/pull/221951
From 9a2884b528460731f9ad03ef722ceffc172f636a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= <[email protected]> Date: Tue, 8 Sep 2026 11:40:07 +0200 Subject: [PATCH 1/2] [clang-tidy] Fix readability-non-const-parameter on atomic builtins MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Atomic builtins are represented by AtomicExpr rather than CallExpr, so the check did not see pointers passed to them and suggested making those parameters pointers to const. This is wrong for the 'expected' operand of atomic_compare_exchange_strong() and friends, which receives the old value when the exchange fails, and applying the fix-it produced code that no longer compiles. Match atomicExpr() and mark all of its operands, mirroring how arguments of ordinary calls are treated. Signed-off-by: Björn Svensson <[email protected]> --- .../readability/NonConstParameterCheck.cpp | 9 +++- clang-tools-extra/docs/ReleaseNotes.md | 6 +++ .../readability/non-const-parameter.c | 37 ++++++++++++++++ .../readability/non-const-parameter.cpp | 43 +++++++++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) diff --git a/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp b/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp index 12113fa3b570a..62ca5a5700837 100644 --- a/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp +++ b/clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp @@ -66,7 +66,7 @@ void NonConstParameterCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( stmt(anyOf(unaryOperator(hasAnyOperatorName("++", "--")), binaryOperator(), callExpr(), returnStmt(), cxxConstructExpr(), - cxxUnresolvedConstructExpr())) + cxxUnresolvedConstructExpr(), atomicExpr())) .bind("Mark"), this); Finder->addMatcher(varDecl(hasOwnInitializer(anything())).bind("Mark"), this); @@ -113,6 +113,13 @@ void NonConstParameterCheck::check(const MatchFinder::MatchResult &Result) { markCanNotBeConst(Arg->IgnoreParenCasts(), false); } } + } else if (const auto *AE = dyn_cast<AtomicExpr>(S)) { + // Atomic builtins may write through their pointer operands, such as the + // 'expected' operand of a compare-exchange, which receives the old value + // when the exchange fails. + for (const Expr *SubExpr : + llvm::ArrayRef(AE->getSubExprs(), AE->getNumSubExprs())) + markCanNotBeConst(SubExpr->IgnoreParenCasts(), true); } else if (const auto *CE = dyn_cast<CXXConstructExpr>(S)) { for (const auto *Arg : CE->arguments()) markCanNotBeConst(Arg->IgnoreParenCasts(), true); diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 4523e0f78035b..b5ca48de924f3 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -244,6 +244,12 @@ infrastructure are described first, followed by tool-specific sections. exclusively for overload resolution. Added the {option}`IgnoredTypes` option to allow customizing the set of ignored types. +- Improved {doc}`readability-non-const-parameter + <clang-tidy/checks/readability/non-const-parameter>` check by fixing false + positives on pointers passed to atomic builtins, whose operands may be + written to, such as the `expected` parameter of + `atomic_compare_exchange_strong()`. + - Improved {doc}`readability-trailing-comma <clang-tidy/checks/readability/trailing-comma>` check: diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c index 233ffb4baf692..d05981bc95f0e 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c @@ -9,3 +9,40 @@ int f(p) { return *p; } + +// The 'expected' operand of a compare-exchange receives the old value when the +// exchange fails. +int atomic_cas(_Atomic int *obj, int *expected, int desired) { + return __c11_atomic_compare_exchange_strong( + obj, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +int atomic_cas_weak(_Atomic int *obj, int *expected, int desired) { + return __c11_atomic_compare_exchange_weak( + obj, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +// The non-'_n' load and exchange forms write their result through an operand. +void atomic_load_out(int *obj, int *dest) { + __atomic_load(obj, dest, __ATOMIC_SEQ_CST); +} + +void atomic_exchange_out(int *obj, int *val, int *old) { + __atomic_exchange(obj, val, old, __ATOMIC_SEQ_CST); +} + +// Only the operands of the atomic builtin are affected. Other parameters are +// still reported. +// CHECK-MESSAGES: :[[@LINE+1]]:69: warning: pointer parameter 'unrelated' can be pointer to const [readability-non-const-parameter] +int atomic_cas_unrelated(int *obj, int *expected, int desired, int *unrelated) { + // CHECK-FIXES: int atomic_cas_unrelated(int *obj, int *expected, int desired, const int *unrelated) { + return __atomic_compare_exchange_n(obj, expected, desired, 0, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) + + *unrelated; +} + +// Conservatively, operands of other atomic builtins are not suggested as const +// either, matching the existing treatment of ordinary function calls. +int atomic_load_ptr(int *p) { + return __atomic_load_n(p, __ATOMIC_SEQ_CST); +} diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp index 43bd8e72cd568..3d9b8a05ae8ad 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp @@ -524,3 +524,46 @@ struct QualifiedMemberOverload { } void withConstQualifier(const int *qualifiedMemberPtr) {} }; + +// The 'expected' operand of a compare-exchange receives the old value when the +// exchange fails. +bool atomicCompareExchangeN(int *obj, int *expected, int desired) { + return __atomic_compare_exchange_n(obj, expected, desired, false, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +bool atomicCompareExchange(int *obj, int *expected, int *desired) { + return __atomic_compare_exchange(obj, expected, desired, false, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +// Operands are followed through pointer arithmetic. +bool atomicCompareExchangeOffset(int *obj, int *expected, int desired) { + return __atomic_compare_exchange_n(obj, expected + 1, desired, false, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +// The non-'_n' load and exchange forms write their result through an operand. +void atomicLoadOut(int *obj, int *dest) { + __atomic_load(obj, dest, __ATOMIC_SEQ_CST); +} + +void atomicExchangeOut(int *obj, int *val, int *old) { + __atomic_exchange(obj, val, old, __ATOMIC_SEQ_CST); +} + +// Only the operands of the atomic builtin are affected. Other parameters are +// still reported. +// CHECK-MESSAGES: :[[@LINE+1]]:66: warning: pointer parameter 'unrelated' can be pointer to const +int atomicCompareExchangeUnrelated(int *obj, int *expected, int *unrelated) { + // CHECK-FIXES: int atomicCompareExchangeUnrelated(int *obj, int *expected, const int *unrelated) { + return __atomic_compare_exchange_n(obj, expected, 0, false, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST) + + *unrelated; +} + +// Conservatively, operands of other atomic builtins are not suggested as const +// either, matching the existing treatment of ordinary function calls. +int atomicLoad(int *p) { + return __atomic_load_n(p, __ATOMIC_SEQ_CST); +} From 34ed75f6e12c0c2433b9afee1e75e2542c19332d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Svensson?= <[email protected]> Date: Thu, 17 Sep 2026 13:03:39 +0200 Subject: [PATCH 2/2] fixup: remove superfluous comments in tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Björn Svensson <[email protected]> --- .../clang-tidy/checkers/readability/non-const-parameter.c | 7 ------- .../checkers/readability/non-const-parameter.cpp | 8 -------- 2 files changed, 15 deletions(-) diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c index d05981bc95f0e..2612f084883cb 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c @@ -10,8 +10,6 @@ int f(p) return *p; } -// The 'expected' operand of a compare-exchange receives the old value when the -// exchange fails. int atomic_cas(_Atomic int *obj, int *expected, int desired) { return __c11_atomic_compare_exchange_strong( obj, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); @@ -22,7 +20,6 @@ int atomic_cas_weak(_Atomic int *obj, int *expected, int desired) { obj, expected, desired, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } -// The non-'_n' load and exchange forms write their result through an operand. void atomic_load_out(int *obj, int *dest) { __atomic_load(obj, dest, __ATOMIC_SEQ_CST); } @@ -31,8 +28,6 @@ void atomic_exchange_out(int *obj, int *val, int *old) { __atomic_exchange(obj, val, old, __ATOMIC_SEQ_CST); } -// Only the operands of the atomic builtin are affected. Other parameters are -// still reported. // CHECK-MESSAGES: :[[@LINE+1]]:69: warning: pointer parameter 'unrelated' can be pointer to const [readability-non-const-parameter] int atomic_cas_unrelated(int *obj, int *expected, int desired, int *unrelated) { // CHECK-FIXES: int atomic_cas_unrelated(int *obj, int *expected, int desired, const int *unrelated) { @@ -41,8 +36,6 @@ int atomic_cas_unrelated(int *obj, int *expected, int desired, int *unrelated) { *unrelated; } -// Conservatively, operands of other atomic builtins are not suggested as const -// either, matching the existing treatment of ordinary function calls. int atomic_load_ptr(int *p) { return __atomic_load_n(p, __ATOMIC_SEQ_CST); } diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp index 3d9b8a05ae8ad..5264a99639b36 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp @@ -525,8 +525,6 @@ struct QualifiedMemberOverload { void withConstQualifier(const int *qualifiedMemberPtr) {} }; -// The 'expected' operand of a compare-exchange receives the old value when the -// exchange fails. bool atomicCompareExchangeN(int *obj, int *expected, int desired) { return __atomic_compare_exchange_n(obj, expected, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); @@ -537,13 +535,11 @@ bool atomicCompareExchange(int *obj, int *expected, int *desired) { __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } -// Operands are followed through pointer arithmetic. bool atomicCompareExchangeOffset(int *obj, int *expected, int desired) { return __atomic_compare_exchange_n(obj, expected + 1, desired, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); } -// The non-'_n' load and exchange forms write their result through an operand. void atomicLoadOut(int *obj, int *dest) { __atomic_load(obj, dest, __ATOMIC_SEQ_CST); } @@ -552,8 +548,6 @@ void atomicExchangeOut(int *obj, int *val, int *old) { __atomic_exchange(obj, val, old, __ATOMIC_SEQ_CST); } -// Only the operands of the atomic builtin are affected. Other parameters are -// still reported. // CHECK-MESSAGES: :[[@LINE+1]]:66: warning: pointer parameter 'unrelated' can be pointer to const int atomicCompareExchangeUnrelated(int *obj, int *expected, int *unrelated) { // CHECK-FIXES: int atomicCompareExchangeUnrelated(int *obj, int *expected, const int *unrelated) { @@ -562,8 +556,6 @@ int atomicCompareExchangeUnrelated(int *obj, int *expected, int *unrelated) { *unrelated; } -// Conservatively, operands of other atomic builtins are not suggested as const -// either, matching the existing treatment of ordinary function calls. int atomicLoad(int *p) { return __atomic_load_n(p, __ATOMIC_SEQ_CST); } _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
