[clang-tools-extra] [clang-tidy] Fix macro-repeated-side-effects false positive on assert (PR #235410)
Yash Aggarwal via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 25 00:02:28 PDT 2026
https://github.com/yashaggarwal85d created https://github.com/llvm/llvm-project/pull/235410
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.
>From 9000ec377170861540ada74420712e8d23c55e2e Mon Sep 17 00:00:00 2001
From: Yash Aggarwal <68061195+yashaggarwal85d at users.noreply.github.com>
Date: Sun, 25 Oct 2026 05:00:26 +0000
Subject: [PATCH] [clang-tidy] Fix macro-repeated-side-effects false positive
on assert
---
.../MacroRepeatedSideEffectsCheck.cpp | 54 +++++++++++++++++--
clang-tools-extra/docs/ReleaseNotes.md | 6 +++
.../bugprone/macro-repeated-side-effects.c | 25 +++++++++
3 files changed, 80 insertions(+), 5 deletions(-)
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
+}
More information about the cfe-commits
mailing list