Author: Björn Svensson Date: 2026-09-18T12:48:35+03:00 New Revision: db349a40f94f8d3fe089b396d688f38dcb794ebf
URL: https://github.com/llvm/llvm-project/commit/db349a40f94f8d3fe089b396d688f38dcb794ebf DIFF: https://github.com/llvm/llvm-project/commit/db349a40f94f8d3fe089b396d688f38dcb794ebf.diff LOG: [clang-tidy] Fix false positive in readability-non-const-parameter for atomic builtins (#221951) Atomic builtins are represented in the AST by `AtomicExpr` rather than `CallExpr`, so `readability-non-const-parameter` never analysed their operands and assumed the pointed-to data was only read. It then suggested making those pointer parameters point to const, and applying the fix-it produced code that no longer compiles. The most visible case is the `expected` operand of a compare-exchange, which per the C standard must be a pointer to non-const because it receives the old value when the exchange fails: https://godbolt.org/z/sovKf3G3c Before this change the check reported `expected` as a candidate for pointer-to-const. The GNU builtins are affected the same way, and there the address operand `obj` was reported too, as were the destination operand of `__atomic_load()` and the old-value operand of `__atomic_exchange()`. Match `atomicExpr()` and mark all of its operands, mirroring how arguments of ordinary calls are already treated. --------- Signed-off-by: Björn Svensson <[email protected]> Added: Modified: clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp clang-tools-extra/docs/ReleaseNotes.md clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.c clang-tools-extra/test/clang-tidy/checkers/readability/non-const-parameter.cpp Removed: ################################################################################ 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 a702721a0d757..b77d0b5f5b7ec 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -278,6 +278,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-redundant-parentheses <clang-tidy/checks/readability/redundant-parentheses>` check by fixing a false positive on the required parentheses of `typeof` and `typeof_unqual` operands. 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..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 @@ -9,3 +9,33 @@ int f(p) { return *p; } + +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); +} + +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); +} + +// 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; +} + +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..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 @@ -524,3 +524,38 @@ struct QualifiedMemberOverload { } void withConstQualifier(const int *qualifiedMemberPtr) {} }; + +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); +} + +bool atomicCompareExchangeOffset(int *obj, int *expected, int desired) { + return __atomic_compare_exchange_n(obj, expected + 1, desired, false, + __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST); +} + +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); +} + +// 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; +} + +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
