From c8a315aa29e4439e528ed15cdea07d9f9c52f68b Mon Sep 17 00:00:00 2001 From: Balazs Benics Date: Mon, 24 Nov 2025 20:48:31 +0100 Subject: [PATCH 1/2] [analyzer] Unroll loops of compile-time upper-bounded loops Previously, only literal upper-bounded loops were recognized. This patch relaxes this matching to accept any compile-time deducible constant expression. It would be better to rely on the SVals (values from the symbolic domain), as those could potentially have more accurate answers, but this one is much simpler. Note that at the time we calculate this value, we have not evaluated the sub-exprs of the condition, consequently, we can't just query the Environment for the folded SVal. Because of this, the next best tool in our toolbox is comp-time evaluating the Expr. rdar://165363923 --- .../lib/StaticAnalyzer/Core/LoopUnrolling.cpp | 29 +++++++++++-------- clang/test/Analysis/loop-unrolling.cpp | 22 ++++++++++++-- 2 files changed, 37 insertions(+), 14 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp b/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp index 01d87b02fcdbd..6148b22d74240 100644 --- a/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp +++ b/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp @@ -84,15 +84,17 @@ ProgramStateRef processLoopEnd(const Stmt *LoopStmt, ProgramStateRef State) { static internal::Matcher simpleCondition(StringRef BindName, StringRef RefName) { + auto LoopVariable = ignoringParenImpCasts( + declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName))) + .bind(RefName)); + auto UpperBound = ignoringParenImpCasts(expr().bind("boundNum")); + return binaryOperator( anyOf(hasOperatorName("<"), hasOperatorName(">"), hasOperatorName("<="), hasOperatorName(">="), hasOperatorName("!=")), - hasEitherOperand(ignoringParenImpCasts( - declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName))) - .bind(RefName))), - hasEitherOperand( - ignoringParenImpCasts(integerLiteral().bind("boundNum")))) + anyOf(binaryOperator(hasLHS(LoopVariable), hasRHS(UpperBound)), + binaryOperator(hasRHS(LoopVariable), hasLHS(UpperBound)))) .bind("conditionOperator"); } @@ -271,23 +273,26 @@ static bool shouldCompletelyUnroll(const Stmt *LoopStmt, ASTContext &ASTCtx, if (!isLoopStmt(LoopStmt)) return false; - // TODO: Match the cases where the bound is not a concrete literal but an - // integer with known value auto Matches = match(forLoopMatcher(), *LoopStmt, ASTCtx); if (Matches.empty()) return false; const auto *CounterVarRef = Matches[0].getNodeAs("initVarRef"); - llvm::APInt BoundNum = - Matches[0].getNodeAs("boundNum")->getValue(); + const Expr *BoundNumExpr = Matches[0].getNodeAs("boundNum"); + + Expr::EvalResult BoundNumResult; + if (!BoundNumExpr || !BoundNumExpr->EvaluateAsInt(BoundNumResult, ASTCtx, + Expr::SE_NoSideEffects)) { + return false; + } llvm::APInt InitNum = Matches[0].getNodeAs("initNum")->getValue(); auto CondOp = Matches[0].getNodeAs("conditionOperator"); - unsigned MaxWidth = std::max(InitNum.getBitWidth(), BoundNum.getBitWidth()); + unsigned MaxWidth = std::max(InitNum.getBitWidth(), + BoundNumResult.Val.getInt().getBitWidth()); InitNum = InitNum.zext(MaxWidth); - BoundNum = BoundNum.zext(MaxWidth); - + llvm::APInt BoundNum = BoundNumResult.Val.getInt().zext(MaxWidth); if (CondOp->getOpcode() == BO_GE || CondOp->getOpcode() == BO_LE) maxStep = (BoundNum - InitNum + 1).abs().getZExtValue(); else diff --git a/clang/test/Analysis/loop-unrolling.cpp b/clang/test/Analysis/loop-unrolling.cpp index ebae81e000c7a..8a4690b9b6c98 100644 --- a/clang/test/Analysis/loop-unrolling.cpp +++ b/clang/test/Analysis/loop-unrolling.cpp @@ -1,5 +1,5 @@ -// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify=expected,default -std=c++14 -analyzer-config exploration_strategy=unexplored_first_queue %s -// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify=expected,dfs -std=c++14 %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true -verify=expected,default -std=c++17 -analyzer-config exploration_strategy=unexplored_first_queue %s +// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config unroll-loops=true,cfg-loopexit=true,exploration_strategy=dfs -verify=expected,dfs -std=c++17 %s void clang_analyzer_numTimesReached(); void clang_analyzer_warnIfReached(); @@ -580,3 +580,21 @@ void test_escaping_on_var_before_switch_case_no_crash(int c) { } } } + +template struct Integer { + static constexpr int value = Val; +}; + +void complicated_compile_time_upper_bound() { + static_assert((sizeof(char) * Integer<4>::value + 3) == 7); + for (int i = 0; i < (sizeof(char) * Integer<4>::value + (((3)))); ++i) { + clang_analyzer_numTimesReached(); // expected-warning {{7}} + } +} + +void complicated_compile_time_upper_bound_indirect() { + using Seven = Integer<(sizeof(char) * Integer<4>::value + 3)>; + for (int i = 0; i < ((Seven::value)); ++i) { + clang_analyzer_numTimesReached(); // expected-warning {{7}} + } +} From a7769ed2fa412b8dfab3897a10450f81a2e74aae Mon Sep 17 00:00:00 2001 From: Balazs Benics Date: Tue, 25 Nov 2025 12:24:05 +0100 Subject: [PATCH 2/2] Narrow the matcher pattern --- .../lib/StaticAnalyzer/Core/LoopUnrolling.cpp | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp b/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp index 6148b22d74240..5803cbd9f7229 100644 --- a/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp +++ b/clang/lib/StaticAnalyzer/Core/LoopUnrolling.cpp @@ -23,6 +23,8 @@ using namespace clang; using namespace ento; using namespace clang::ast_matchers; +using ast_matchers::internal::Matcher; + static const int MAXIMUM_STEP_UNROLLED = 128; namespace { @@ -69,6 +71,11 @@ struct LoopState { REGISTER_LIST_WITH_PROGRAMSTATE(LoopStack, LoopState) namespace clang { +namespace { +AST_MATCHER(QualType, isIntegralOrEnumerationType) { + return Node->isIntegralOrEnumerationType(); +} +} // namespace namespace ento { static bool isLoopStmt(const Stmt *S) { @@ -82,12 +89,12 @@ ProgramStateRef processLoopEnd(const Stmt *LoopStmt, ProgramStateRef State) { return State; } -static internal::Matcher simpleCondition(StringRef BindName, - StringRef RefName) { +static Matcher simpleCondition(StringRef BindName, StringRef RefName) { auto LoopVariable = ignoringParenImpCasts( declRefExpr(to(varDecl(hasType(isInteger())).bind(BindName))) .bind(RefName)); - auto UpperBound = ignoringParenImpCasts(expr().bind("boundNum")); + auto UpperBound = ignoringParenImpCasts( + expr(hasType(isIntegralOrEnumerationType())).bind("boundNum")); return binaryOperator( anyOf(hasOperatorName("<"), hasOperatorName(">"), @@ -98,8 +105,7 @@ static internal::Matcher simpleCondition(StringRef BindName, .bind("conditionOperator"); } -static internal::Matcher -changeIntBoundNode(internal::Matcher VarNodeMatcher) { +static Matcher changeIntBoundNode(Matcher VarNodeMatcher) { return anyOf( unaryOperator(anyOf(hasOperatorName("--"), hasOperatorName("++")), hasUnaryOperand(ignoringParenImpCasts( @@ -109,15 +115,13 @@ changeIntBoundNode(internal::Matcher VarNodeMatcher) { declRefExpr(to(varDecl(VarNodeMatcher))))))); } -static internal::Matcher -callByRef(internal::Matcher VarNodeMatcher) { +static Matcher callByRef(Matcher VarNodeMatcher) { return callExpr(forEachArgumentWithParam( declRefExpr(to(varDecl(VarNodeMatcher))), parmVarDecl(hasType(references(qualType(unless(isConstQualified()))))))); } -static internal::Matcher -assignedToRef(internal::Matcher VarNodeMatcher) { +static Matcher assignedToRef(Matcher VarNodeMatcher) { return declStmt(hasDescendant(varDecl( allOf(hasType(referenceType()), hasInitializer(anyOf( @@ -125,14 +129,13 @@ assignedToRef(internal::Matcher VarNodeMatcher) { declRefExpr(to(varDecl(VarNodeMatcher))))))))); } -static internal::Matcher -getAddrTo(internal::Matcher VarNodeMatcher) { +static Matcher getAddrTo(Matcher VarNodeMatcher) { return unaryOperator( hasOperatorName("&"), hasUnaryOperand(declRefExpr(hasDeclaration(VarNodeMatcher)))); } -static internal::Matcher hasSuspiciousStmt(StringRef NodeName) { +static Matcher hasSuspiciousStmt(StringRef NodeName) { return hasDescendant(stmt( anyOf(gotoStmt(), switchStmt(), returnStmt(), // Escaping and not known mutation of the loop counter is handled @@ -144,7 +147,7 @@ static internal::Matcher hasSuspiciousStmt(StringRef NodeName) { assignedToRef(equalsBoundNode(std::string(NodeName)))))); } -static internal::Matcher forLoopMatcher() { +static Matcher forLoopMatcher() { return forStmt( hasCondition(simpleCondition("initVarName", "initVarRef")), // Initialization should match the form: 'int i = 6' or 'i = 42'.