llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tidy Author: Yash Aggarwal (yashaggarwal85d) <details> <summary>Changes</summary> Since glibc 2.44, `assert()` is variadic and mentions `__VA_ARGS__` twice, but only one copy is evaluated. bugprone-macro-repeated-side-effects counted both, so `assert(++x == 1)` got flagged. There are two shapes. In C the extra copy is inside `sizeof(__assert_single_arg(__VA_ARGS__))`. In C++26 it's in the dead branch of `void(1 ? 1 : bool(__VA_ARGS__))`. So the check now skips args inside `sizeof`/`alignof`/`_Alignof` (like it already does for `__builtin_constant_p`), and skips the false branch of `(N ? a : b)` when N is a non-zero integer literal, up to the closing paren or a comma. `(0 ? a : b)` and non-literal conditions are unchanged. I checked it against the real glibc 2.44 assert.h with `-std=c23` and `-std=c++2c`: warning before, none after. Added tests with glibc-style macros and a release note. Fixes #<!-- -->233597 This fix was made with help from an AI tool. I reviewed and tested the change myself. --- Full diff: https://github.com/llvm/llvm-project/pull/235410.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp (+49-5) - (modified) clang-tools-extra/docs/ReleaseNotes.md (+6) - (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c (+25) ``````````diff diff --git a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp index 4306cb11ce4bac..6297e675580559 100644 --- a/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/MacroRepeatedSideEffectsCheck.cpp @@ -12,6 +12,7 @@ #include "clang/Lex/MacroArgs.h" #include "clang/Lex/PPCallbacks.h" #include "clang/Lex/Preprocessor.h" +#include <optional> #include <stack> namespace clang::tidy::bugprone { @@ -32,6 +33,8 @@ class MacroRepeatedPPCallbacks : public PPCallbacks { unsigned countArgumentExpansions(const MacroInfo *MI, const IdentifierInfo *Arg) const; + bool isNonZeroLiteral(const Token &T) const; + bool hasSideEffects(const Token *ResultArgToks) const; }; } // End of anonymous namespace. @@ -84,10 +87,30 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions( // Has a __builtin_constant_p been found? bool FoundBuiltin = false; bool PrevTokenIsHash = false; + int ParenDepth = 0; + // Paren depth of the false branch of "(1 ? a : b)" while we are skipping it. + std::optional<int> DeadBranchDepth; // Count when "?" is reached. The "Current" will get this value when the ":" - // is reached. - std::stack<unsigned, SmallVector<unsigned, 8>> CountAtQuestion; - for (const auto &T : MI->tokens()) { + // is reached. Also remembers if the condition was a non-zero literal. + std::stack<std::pair<unsigned, bool>, + SmallVector<std::pair<unsigned, bool>, 8>> + CountAtQuestion; + const ArrayRef<Token> Tokens = MI->tokens(); + for (const auto [I, T] : llvm::enumerate(Tokens)) { + if (T.is(tok::l_paren)) + ParenDepth++; + else if (T.is(tok::r_paren)) + ParenDepth--; + + // The false branch of "(1 ? a : b)" is never evaluated. Skip it until the + // closing parenthesis or a comma. + if (DeadBranchDepth) { + if (ParenDepth > *DeadBranchDepth || + (ParenDepth == *DeadBranchDepth && T.isNot(tok::comma))) + continue; + DeadBranchDepth.reset(); + } + // The result of __builtin_constant_p(x) is 0 if x is a macro argument // with side effects. If we see a __builtin_constant_p(x) followed by a // "?" "&&" or "||", then we need to reason about control flow to report @@ -108,12 +131,19 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions( // Handling of ? and :. if (T.is(tok::question)) { - CountAtQuestion.push(Current); + CountAtQuestion.emplace(Current, I >= 2 && + Tokens[I - 2].is(tok::l_paren) && + isNonZeroLiteral(Tokens[I - 1])); } else if (T.is(tok::colon)) { if (CountAtQuestion.empty()) return 0; - Current = CountAtQuestion.top(); + bool SkipFalseBranch = false; + std::tie(Current, SkipFalseBranch) = CountAtQuestion.top(); CountAtQuestion.pop(); + if (SkipFalseBranch) { + DeadBranchDepth = ParenDepth; + continue; + } } // If current token is a parenthesis, skip it. @@ -132,6 +162,12 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions( if (TII == nullptr) continue; + // Arguments of sizeof and alignof are not evaluated. + if (T.isOneOf(tok::kw_sizeof, tok::kw_alignof, tok::kw__Alignof)) { + SkipParen = true; + continue; + } + // If a __builtin_constant_p is found within the macro definition, don't // count arguments inside the parentheses and remember that it has been // seen in case there are "?", "&&" or "||" operators later. @@ -159,6 +195,14 @@ unsigned MacroRepeatedPPCallbacks::countArgumentExpansions( return Max; } +bool MacroRepeatedPPCallbacks::isNonZeroLiteral(const Token &T) const { + if (T.isNot(tok::numeric_constant)) + return false; + SmallString<16> Buffer; + uint64_t Value = 0; + return !PP.getSpelling(T, Buffer).getAsInteger(0, Value) && Value != 0; +} + bool MacroRepeatedPPCallbacks::hasSideEffects( const Token *ResultArgToks) const { for (; ResultArgToks->isNot(tok::eof); ++ResultArgToks) diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md index 037df4cd0937c4..4337ce234db855 100644 --- a/clang-tools-extra/docs/ReleaseNotes.md +++ b/clang-tools-extra/docs/ReleaseNotes.md @@ -204,6 +204,12 @@ infrastructure are described first, followed by tool-specific sections. <clang-tidy/checks/bugprone/macro-parentheses>` by fixing invalid fixes for macro arguments used as names in alias declarations. +- Improved {doc}`bugprone-macro-repeated-side-effects + <clang-tidy/checks/bugprone/macro-repeated-side-effects>` check by not + counting macro arguments that are never evaluated, such as inside `sizeof` + or in the false branch of `(1 ? a : b)`. This fixes false positives on + `assert` from glibc 2.44. + - Fixed a crash in {doc}`bugprone-misplaced-operator-in-strlen-in-alloc <clang-tidy/checks/bugprone/misplaced-operator-in-strlen-in-alloc>` when checking an array new expression without a size expression. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c index 995d872dce76a0..6657719a49c45b 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/macro-repeated-side-effects.c @@ -123,3 +123,28 @@ void log(const char *s, int v); void test_log(int a) { LOG(a++); } + +_Bool single_arg(_Bool); +void fail(const char *); +#define sizeofgood(x) (sizeof(x) + _Alignof(x) + (x)) +#define sizeofbad(x) (sizeof(x) + (x) + (x)) +#define assertc(...) (((void)sizeof(single_arg(__VA_ARGS__)), __VA_ARGS__) ? (void)0 : fail(#__VA_ARGS__)) +#define assertcpp(...) ((__VA_ARGS__) ? (void)(1 ? 1 : (_Bool)(__VA_ARGS__)) : fail(#__VA_ARGS__)) +#define deadbad1(x) ((x) + (0 ? 1 : (x))) +#define deadbad2(x) ((x) + (1 ? 1 : (x), (x))) +void unevaluated(int ret, int a) { + ret += sizeofgood(a++); + ret += sizeofbad(a++); + // CHECK-NOTES: :[[@LINE-1]]:20: warning: side effects in the 1st macro argument 'x' + // CHECK-NOTES: :[[@LINE-9]]:9: note: macro 'sizeofbad' defined here + + assertc(++a == 1); + assertcpp(++a == 1); + + ret += deadbad1(a++); + // CHECK-NOTES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x' + // CHECK-NOTES: :[[@LINE-13]]:9: note: macro 'deadbad1' defined here + ret += deadbad2(a++); + // CHECK-NOTES: :[[@LINE-1]]:19: warning: side effects in the 1st macro argument 'x' + // CHECK-NOTES: :[[@LINE-15]]:9: note: macro 'deadbad2' defined here +} `````````` </details> https://github.com/llvm/llvm-project/pull/235410 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
