diff --git a/lib/analyzer.h b/lib/analyzer.h index a4546eb7dc4..9b87310530f 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -153,9 +153,13 @@ struct Analyzer { struct Assume { enum Flags : std::uint8_t { None = 0, - Quiet = (1 << 0), - Absolute = (1 << 1), - ContainerEmpty = (1 << 2), + Quiet = (1u << 0), + Absolute = (1u << 1), + ContainerEmpty = (1u << 2), + // The branch this condition guards is not traversed yet (a separate path walks it), so + // the assume must not record the program state at the branch boundaries - they would be + // premature. When unset, the branch has been traversed and control is leaving it. + Pending = (1u << 3), }; }; diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 8700d2d2cd9..034ba20fae0 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2230,6 +2230,8 @@ bool isEscapeFunction(const Token* ftok, const Library& library) { if (!Token::Match(ftok, "%name% (")) return false; + if (Token::Match(ftok, "exit|abort")) + return true; const Function* function = ftok->function(); if (function) { if (function->isEscapeFunction()) diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 670db0b1090..4609e044b97 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -89,6 +89,9 @@ namespace { bool isDead() const { return action.isModified() || action.isInconclusive() || isEscape(); } + bool hasGoto() const { + return endBlock ? ForwardTraversal::hasGoto(endBlock) : false; + } }; bool stopUpdates() { @@ -348,18 +351,6 @@ namespace { return Token::findmatch(endBlock->link(), "goto|break", endBlock); } - bool hasInnerReturnScope(const Token* start, const Token* end) const { - for (const Token* tok=start; tok != end; tok = tok->previous()) { - if (Token::simpleMatch(tok, "}")) { - const Token* ftok = nullptr; - const bool r = isReturnScope(tok, settings.library, &ftok); - if (r) - return true; - } - } - return false; - } - bool isEscapeScope(const Token* endBlock, bool& unknown) const { const Token* ftok = nullptr; const bool r = isReturnScope(endBlock, settings.library, &ftok); @@ -383,30 +374,34 @@ namespace { return a; } - bool checkBranch(Branch& branch) const { - Analyzer::Action a = analyzeScope(branch.endBlock); - branch.action = a; - std::vector ft1 = tryForkUpdateScope(branch.endBlock, a.isModified()); - const bool bail = hasGoto(branch.endBlock); - if (!a.isModified() && !bail) { - if (ft1.empty()) { - // Traverse into the branch to see if there is a conditional escape - if (!branch.escape && hasInnerReturnScope(branch.endBlock->previous(), branch.endBlock->link())) { - ForwardTraversal ft2 = fork(true); - ft2.updateScope(branch.endBlock); - if (ft2.terminate == Analyzer::Terminate::Escape) { - branch.escape = true; - branch.escapeUnknown = false; - } - } - } else { - if (ft1.front().terminate == Analyzer::Terminate::Escape) { - branch.escape = true; - branch.escapeUnknown = false; - } + Progress updateBranch(Branch& branch, int depth) + { + // Save and reset actions + Analyzer::Action prevActions = actions; + actions = Analyzer::Action::None; + Progress p = updateRange(branch.endBlock->link(), branch.endBlock, depth); + branch.action |= actions; + // Restore actions + actions |= prevActions; + + if (terminate == Analyzer::Terminate::Escape) { + branch.escape = true; + // The traversal followed an escaping path, but if the scope does not structurally + // always escape then another path (e.g. a modified fork) falls through, so the escape + // is only conditional - keep isModified() meaningful by not treating it as conclusive. + bool structuralUnknown = false; + const bool structuralEscape = isEscapeScope(branch.endBlock, structuralUnknown); + branch.escapeUnknown = !structuralEscape || structuralUnknown; + } else { + // Detect an escape the traversal did not flag (e.g. an unknown noreturn call); + // escapeUnknown reports a possible (unknown) escape. + branch.escape = isEscapeScope(branch.endBlock, branch.escapeUnknown); + if (terminate != Analyzer::Terminate::None && terminate != Analyzer::Terminate::Modified) { + branch.action |= analyzeScope(branch.endBlock); } } - return bail; + + return p; } bool reentersLoop(Token* endBlock, const Token* condTok, const Token* stepTok) const { @@ -529,14 +524,12 @@ namespace { forkContinue = false; } - if (allAnalysis.isModified() || !forkContinue) { - // TODO: Don't bail on missing condition - if (!condTok) - return Break(Analyzer::Terminate::Bail); - if (analyzer->isConditional() && stopUpdates()) - return Break(Analyzer::Terminate::Conditional); - analyzer->assume(condTok, false); - } + // TODO: Don't bail on missing condition + if (!condTok) + return Break(Analyzer::Terminate::Bail); + if (analyzer->isConditional() && stopUpdates()) + return Break(Analyzer::Terminate::Conditional); + analyzer->assume(condTok, false); if (forkContinue) { for (ForwardTraversal& ft : ftv) { if (!ft.actions.isIncremental()) @@ -646,11 +639,20 @@ namespace { const bool inElse = scope->type == ScopeType::eElse; const bool inDoWhile = scope->type == ScopeType::eDo; const bool inLoop = contains({ScopeType::eDo, ScopeType::eFor, ScopeType::eWhile}, scope->type); + const bool hasElse = Token::simpleMatch(tok, "} else {"); Token* condTok = getCondTokFromEnd(tok); if (!condTok) return Break(); + // When the 'else' branch escapes (e.g. returns), control can only continue + // here via the 'then' branch, so the value established there is still + // definite - keep it known instead of lowering to possible. + bool elseEscape = false; + if (!inLoop && !inElse && hasElse) { + bool unknownEscape = false; + elseEscape = isEscapeScope(tok->linkAt(2), unknownEscape); + } if (!condTok->hasKnownIntValue() || inLoop) { - if (!analyzer->lowerToPossible()) + if (!elseEscape && !analyzer->lowerToPossible()) return Break(Analyzer::Terminate::Bail); } else if (condTok->getKnownIntValue() == inElse) { return Break(); @@ -675,7 +677,7 @@ namespace { } analyzer->assume(condTok, !inElse, Analyzer::Assume::Quiet); assert(!inDoWhile || Token::simpleMatch(tok, "} while (")); - if (Token::simpleMatch(tok, "} else {") || inDoWhile) + if (hasElse || inDoWhile) tok = tok->linkAt(2); } else if (contains({ScopeType::eTry, ScopeType::eCatch}, scope->type)) { if (!analyzer->lowerToPossible()) @@ -731,71 +733,73 @@ namespace { if (!thenBranch.check && !elseBranch.check && stopOnCondition(condTok) && stopUpdates()) return Break(Analyzer::Terminate::Conditional); const bool hasElse = Token::simpleMatch(endBlock, "} else {"); - bool bail = false; - - // Traverse then block - thenBranch.escape = isEscapeScope(endBlock, thenBranch.escapeUnknown); + tok = hasElse ? endBlock->linkAt(2) : endBlock; if (thenBranch.check) { - thenBranch.active = true; - if (updateScope(endBlock, depth - 1) == Progress::Break) + // The condition is only "known" because of an earlier assumption, so the + // skipped else block could still modify the value -> lower to possible + if (!condTok->hasKnownIntValue() && hasElse && + analyzeScope(elseBranch.endBlock).isModified() && !analyzer->lowerToPossible()) + return Break(Analyzer::Terminate::Bail); + if (updateScope(thenBranch.endBlock, depth - 1) == Progress::Break) + return Break(); + } else if (elseBranch.check) { + // Likewise the skipped then block could still modify the value + if (!condTok->hasKnownIntValue() && analyzeScope(thenBranch.endBlock).isModified() && + !analyzer->lowerToPossible()) + return Break(Analyzer::Terminate::Bail); + if (elseBranch.endBlock && updateScope(elseBranch.endBlock, depth - 1) == Progress::Break) return Break(); - } else if (!elseBranch.check) { - thenBranch.active = true; - if (checkBranch(thenBranch)) - bail = true; - } - // Traverse else block - if (hasElse) { - elseBranch.escape = isEscapeScope(endBlock->linkAt(2), elseBranch.escapeUnknown); - if (elseBranch.check) { - elseBranch.active = true; - const Progress result = updateScope(endBlock->linkAt(2), depth - 1); - if (result == Progress::Break) - return Break(); - } else if (!thenBranch.check) { - elseBranch.active = true; - if (checkBranch(elseBranch)) - bail = true; - } - tok = endBlock->linkAt(2); } else { - tok = endBlock; - } - if (thenBranch.active) + const bool conditional = stopOnCondition(condTok); + // The value only flows into the then-branch when the condition can split + // it; for an opaque or correlated condition (e.g. 'if (f(x))') it does + // not, so fork in analyze-only mode: the branch's effect is still tracked + // but nothing is reported in it. + ForwardTraversal ft = fork(!analyzer->updateScope(thenBranch.endBlock, false)); + // The branch is traversed below, so don't record its boundary state here. + ft.analyzer->assume(condTok, true, Analyzer::Assume::Pending); + Progress pThen = ft.updateBranch(thenBranch, depth - 1); + // Merge the fork's actions so a modification in the then-branch bubbles up + // to the enclosing branch's isModified(). actions |= thenBranch.action; - if (elseBranch.active) - actions |= elseBranch.action; - if (bail) - return Break(Analyzer::Terminate::Bail); - if (thenBranch.isDead() && elseBranch.isDead()) { - if (thenBranch.isModified() && elseBranch.isModified()) - return Break(Analyzer::Terminate::Modified); - if (thenBranch.isConclusiveEscape() && elseBranch.isConclusiveEscape()) - return Break(Analyzer::Terminate::Escape); - return Break(Analyzer::Terminate::Bail); - } - // Conditional return - if (thenBranch.active && thenBranch.isEscape() && !hasElse) { - if (!thenBranch.isConclusiveEscape()) { - if (!analyzer->lowerToInconclusive()) - return Break(Analyzer::Terminate::Bail); - } else if (thenBranch.check) { - return Break(); - } else { - if (stopOnCondition(condTok) && stopUpdates()) - return Break(Analyzer::Terminate::Conditional); - analyzer->assume(condTok, false); + + // Commit the condition as false on the main path only when the then-branch + // is dead. The else block, if any, is traversed separately (Pending); with + // no else the false path continues past the closing brace, so record the + // assumed state there (None). + if (thenBranch.isDead()) + analyzer->assume(condTok, + false, + hasElse ? Analyzer::Assume::Pending : Analyzer::Assume::None); + // The else block is traversed on the main path. If it kills the value + // (modified) the main path stops, but the then-fork may still carry the + // value forward, so defer the break until after the fork continues. + Progress pElse = Progress::Continue; + if (hasElse) + pElse = updateBranch(elseBranch, depth - 1); + if (thenBranch.isDead() || elseBranch.isDead()) { + if (conditional && stopUpdates()) + pElse = Break(Analyzer::Terminate::Conditional); } - } - if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) { - if (!analyzer->lowerToInconclusive()) - return Break(Analyzer::Terminate::Bail); - } else if (thenBranch.isModified() || elseBranch.isModified()) { - if (!hasElse && analyzer->isConditional() && stopUpdates()) - return Break(Analyzer::Terminate::Conditional); - if (!analyzer->lowerToPossible()) + if (thenBranch.isModified() || elseBranch.isModified()) { + if (!ft.analyzer->lowerToPossible()) + pThen = Progress::Break; + if (pElse != Progress::Break && !analyzer->lowerToPossible()) + pElse = Break(Analyzer::Terminate::Bail); + } + if (thenBranch.isInconclusive() || elseBranch.isInconclusive()) { + if (!ft.analyzer->lowerToInconclusive()) + pThen = Progress::Break; + if (pElse != Progress::Break && !analyzer->lowerToInconclusive()) + pElse = Break(Analyzer::Terminate::Bail); + } + if (thenBranch.hasGoto() || elseBranch.hasGoto()) { return Break(Analyzer::Terminate::Bail); - analyzer->assume(condTok, elseBranch.isModified()); + } + if (pThen != Progress::Break && !thenBranch.isEscape()) + ft.updateRange(thenBranch.endBlock, end, depth - 1); + if (pElse == Progress::Break) + return Break(); } } } else if (Token::simpleMatch(tok, "try {")) { diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index b86ca2d8812..c062b5f9ade 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -262,26 +262,33 @@ ProgramMemory::Map::iterator ProgramMemory::find(nonneg int exprid) return mValues->find(ExprIdToken::create(exprid)); } -static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings); - -static bool evaluateCondition(MathLib::bigint r, const Token* condition, ProgramMemory& pm, const Settings& settings) +static ValueFlow::Value execute(const Token* expr, + ProgramMemory& pm, + const Settings& settings, + const ProgramMemory::Map& vars = {}); + +static bool evaluateCondition(MathLib::bigint r, + const Token* condition, + ProgramMemory& pm, + const Settings& settings, + const ProgramMemory::Map& vars = {}) { if (!condition) return false; MathLib::bigint result = 0; bool error = false; - execute(condition, pm, &result, &error, settings); + execute(condition, pm, &result, &error, settings, vars); return !error && result == r; } -bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings) +bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings, const ProgramMemory::Map& vars) { - return evaluateCondition(0, condition, pm, settings); + return evaluateCondition(0, condition, pm, settings, vars); } -bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings) +bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings, const ProgramMemory::Map& vars) { - return evaluateCondition(1, condition, pm, settings); + return evaluateCondition(1, condition, pm, settings, vars); } static bool frontIs(const std::vector& v, bool i) @@ -429,22 +436,12 @@ static void fillProgramMemoryFromAssignments(ProgramMemory& pm, const Token* tok for (const Token *tok2 = tok; tok2; tok2 = tok2->previous()) { if ((Token::simpleMatch(tok2, "=") || Token::Match(tok2->previous(), "%var% (|{")) && tok2->astOperand1() && tok2->astOperand2()) { - bool setvar = false; const Token* vartok = tok2->astOperand1(); - for (const auto& p:vars) { - if (p.first.getExpressionId() != vartok->exprId()) - continue; - if (vartok == tok) - continue; - pm.setValue(vartok, p.second); - setvar = true; - } - if (!setvar) { - if (!pm.hasValue(vartok->exprId())) { - const Token* valuetok = tok2->astOperand2(); - ProgramMemory local = state; - pm.setValue(vartok, execute(valuetok, local, settings)); - } + if (!pm.hasValue(vartok->exprId())) { + const Token* valuetok = tok2->astOperand2(); + ProgramMemory local = state; + // Tracked values are substituted by execute() when the expression is evaluated. + pm.setValue(vartok, execute(valuetok, local, settings, vars)); } } else if (Token::simpleMatch(tok2, ")") && tok2->link() && Token::Match(tok2->link()->previous(), "assert|ASSERT ( !!)")) { @@ -547,19 +544,21 @@ void ProgramMemoryState::addState(const Token* tok, const ProgramMemory::Map& va replace(std::move(local), tok); } -void ProgramMemoryState::assume(const Token* tok, bool b, bool isEmpty) +void ProgramMemoryState::assume(const Token* tok, bool b, bool isEmpty, const Token* origin) { ProgramMemory pm = state; if (isEmpty) pm.setContainerSizeValue(tok, 0, b); else programMemoryParseCondition(pm, tok, nullptr, settings, b); - const Token* origin = tok; - const Token* top = tok->astTop(); - if (Token::Match(top->previous(), "for|while|if (") && !Token::simpleMatch(tok->astParent(), "?")) { - origin = top->link()->next(); - if (!b && origin->link()) { - origin = origin->link(); + if (!origin) { + origin = tok; + const Token* top = tok->astTop(); + if (Token::Match(top->previous(), "for|while|if (") && !Token::simpleMatch(tok->astParent(), "?")) { + origin = top->link()->next(); + if (!b && origin->link()) { + origin = origin->link(); + } } } replace(std::move(pm), origin); @@ -1316,6 +1315,9 @@ namespace { struct Executor { ProgramMemory* pm; const Settings& settings; + // Values tracked by the forward/reverse analysis. A tracked value is the authoritative + // current value of its expression and takes precedence over the program memory. + const ProgramMemory::Map* vars = nullptr; int fdepth = 4; int depth = 10; @@ -1324,6 +1326,26 @@ namespace { assert(pm != nullptr); } + // Is the tracked value for this expression available? + const ValueFlow::Value* getTrackedValue(const Token* expr) const + { + if (!vars || expr->exprId() == 0) + return nullptr; + const auto it = vars->find(ExprIdToken::create(expr->exprId())); + return it == vars->end() ? nullptr : &it->second; + } + + // Does the expression read a tracked value? If so, any value cached for it may be stale + // (the tracked value may have changed since), so it must be re-evaluated, not served cached. + bool dependsOnTrackedValue(const Token* expr) const + { + if (!vars || vars->empty()) + return false; + return findAstNode(expr, [&](const Token* tok) { + return getTrackedValue(tok) != nullptr; + }) != nullptr; + } + static ValueFlow::Value unknown() { return ValueFlow::Value::unknown(); } @@ -1622,7 +1644,15 @@ namespace { } return execute(expr->astOperand1()); } - if (expr->exprId() > 0 && pm->hasValue(expr->exprId())) { + // Return the tracked value and write it back when it differs, so later reads see the + // same value (as fillProgramMemoryFromAssignments used to do). + if (const ValueFlow::Value* tracked = getTrackedValue(expr)) { + const ValueFlow::Value* stored = pm->getValue(expr->exprId(), /*impossible*/ true); + if (!stored || *stored != *tracked) + pm->setValue(expr, *tracked); + return *tracked; + } + if (expr->exprId() > 0 && pm->hasValue(expr->exprId()) && !dependsOnTrackedValue(expr)) { ValueFlow::Value result = utils::as_const(*pm).at(expr->exprId()); if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr, settings)) { result.intvalue = !result.intvalue; @@ -1815,9 +1845,13 @@ namespace { }; } // namespace -static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings) +static ValueFlow::Value execute(const Token* expr, + ProgramMemory& pm, + const Settings& settings, + const ProgramMemory::Map& vars) { Executor ex{&pm, settings}; + ex.vars = &vars; return ex.execute(expr); } @@ -1907,9 +1941,10 @@ void execute(const Token* expr, ProgramMemory& programMemory, MathLib::bigint* result, bool* error, - const Settings& settings) + const Settings& settings, + const ProgramMemory::Map& vars) { - ValueFlow::Value v = execute(expr, programMemory, settings); + ValueFlow::Value v = execute(expr, programMemory, settings, vars); if (!v.isIntValue() || v.isImpossible()) { if (error) *error = true; diff --git a/lib/programmemory.h b/lib/programmemory.h index ec24c0df59f..e4d48c74f11 100644 --- a/lib/programmemory.h +++ b/lib/programmemory.h @@ -171,7 +171,7 @@ struct ProgramMemoryState { void addState(const Token* tok, const ProgramMemory::Map& vars); - void assume(const Token* tok, bool b, bool isEmpty = false); + void assume(const Token* tok, bool b, bool isEmpty = false, const Token* origin = nullptr); void removeModifiedVars(const Token* tok); @@ -184,21 +184,30 @@ void execute(const Token* expr, ProgramMemory& programMemory, MathLib::bigint* result, bool* error, - const Settings& settings); + const Settings& settings, + const ProgramMemory::Map& vars = {}); /** * Is condition always false when variable has given value? * \param condition top ast token in condition * \param pm program memory + * \param vars optional tracked values that take precedence over the program memory */ -bool conditionIsFalse(const Token* condition, ProgramMemory pm, const Settings& settings); +bool conditionIsFalse(const Token* condition, + ProgramMemory pm, + const Settings& settings, + const ProgramMemory::Map& vars = {}); /** * Is condition always true when variable has given value? * \param condition top ast token in condition * \param pm program memory + * \param vars optional tracked values that take precedence over the program memory */ -bool conditionIsTrue(const Token* condition, ProgramMemory pm, const Settings& settings); +bool conditionIsTrue(const Token* condition, + ProgramMemory pm, + const Settings& settings, + const ProgramMemory::Map& vars = {}); /** * Get program memory by looking backwards from given token. diff --git a/lib/vf_analyzers.cpp b/lib/vf_analyzers.cpp index 10efc6d6f39..645f508d1ce 100644 --- a/lib/vf_analyzers.cpp +++ b/lib/vf_analyzers.cpp @@ -678,16 +678,20 @@ struct ValueFlowAnalyzer : Analyzer { if (const ValueFlow::Value* v = tok->getKnownValue(ValueFlow::Value::ValueType::INT)) return {v->intvalue}; std::vector result; - ProgramMemory pm = getProgramMemoryFunc(); + // Pass the tracked values so a cached program-memory value that depends on one (e.g. 'h(p)' + // after 'p' was reassigned) is re-evaluated rather than served stale. The memory is built + // from the same state, so compute it once and hand it to the builder. + const ProgramState vars = getProgramState(); + ProgramMemory pm = getProgramMemoryFunc(vars); if (Token::Match(tok, "&&|%oror%")) { - if (conditionIsTrue(tok, pm, getSettings())) + if (conditionIsTrue(tok, pm, getSettings(), vars)) result.push_back(1); - if (conditionIsFalse(tok, std::move(pm), getSettings())) + if (conditionIsFalse(tok, std::move(pm), getSettings(), vars)) result.push_back(0); } else { MathLib::bigint out = 0; bool error = false; - execute(tok, pm, &out, &error, getSettings()); + execute(tok, pm, &out, &error, getSettings(), vars); if (!error) result.push_back(out); } @@ -696,16 +700,16 @@ struct ValueFlowAnalyzer : Analyzer { std::vector evaluateInt(const Token* tok) const { - return evaluateInt(tok, [&] { - return ProgramMemory{getProgramState()}; + return evaluateInt(tok, [](const ProgramState& vars) { + return ProgramMemory{vars}; }); } std::vector evaluate(Evaluate e, const Token* tok, const Token* ctx = nullptr) const override { if (e == Evaluate::Integral) { - return evaluateInt(tok, [&] { - return pms.get(tok, ctx, getProgramState()); + return evaluateInt(tok, [&](const ProgramState& vars) { + return pms.get(tok, ctx, vars); }); } if (e == Evaluate::ContainerEmpty) { @@ -723,30 +727,43 @@ struct ValueFlowAnalyzer : Analyzer { return {}; } - void assume(const Token* tok, bool state, unsigned int flags) override { - // Update program state - pms.removeModifiedVars(tok); - pms.addState(tok, getProgramState()); - pms.assume(tok, state, flags & Assume::ContainerEmpty); - + void assume(const Token* tok, bool state, unsigned int flags) override + { bool isCondBlock = false; const Token* parent = tok->astParent(); if (parent) { isCondBlock = Token::Match(parent->previous(), "if|while ("); } + const Token* endBlock = nullptr; if (isCondBlock) { const Token* startBlock = parent->link()->next(); if (Token::simpleMatch(startBlock, ";") && Token::simpleMatch(parent->tokAt(-2), "} while (")) startBlock = parent->linkAt(-2); - const Token* endBlock = startBlock->link(); - if (state) { - pms.removeModifiedVars(endBlock); - pms.addState(endBlock->previous(), getProgramState()); - } else { - if (Token::simpleMatch(endBlock, "} else {")) - pms.addState(endBlock->linkAt(2)->previous(), getProgramState()); - } + endBlock = startBlock->link(); + } + + // Without Pending the 'then' block has been traversed and control is leaving it, so anchor + // the assumed state at the block end instead of the condition. That keeps assumptions on + // variables modified inside the block (e.g. an 'if' narrowing a value computed there) from + // being discarded as "modified" once control leaves the block. + const bool scopeEnd = !(flags & Assume::Pending) && state && endBlock; + const Token* anchor = scopeEnd ? endBlock : tok; + const Token* origin = scopeEnd ? endBlock : nullptr; + + // Update program state + pms.removeModifiedVars(anchor); + pms.addState(anchor, getProgramState()); + pms.assume(tok, state, flags & Assume::ContainerEmpty, origin); + + // The false path (the true path uses scopeEnd above): record the assumed state where control + // continues - the end of the else block, or the closing brace when there is no else - so it + // reaches the enclosing scope. + if (isCondBlock && !(flags & Assume::Pending) && !state) { + if (Token::simpleMatch(endBlock, "} else {")) + pms.addState(endBlock->linkAt(2)->previous(), getProgramState()); + else + pms.addState(endBlock, getProgramState()); } if (!(flags & Assume::Quiet)) { diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index 9fe3e126660..9eba6ca7aaf 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -4848,7 +4848,9 @@ class TestAutoVariables : public TestFixture { " return *iPtr;\n" " return 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:5:16] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n", errout_str()); + ASSERT_EQUALS( + "[test.cpp:5:16] -> [test.cpp:7:10] -> [test.cpp:4:13] -> [test.cpp:8:17]: (error) Using pointer to local variable 'x' that is out of scope. [invalidLifetime]\n", + errout_str()); // #11753 check("int main(int argc, const char *argv[]) {\n" diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 3143b6eb201..837cd4e78ba 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -4911,6 +4911,30 @@ class TestCondition : public TestFixture { ASSERT_EQUALS("[test.cpp:3:10]: (style) Condition 'b()' is always false [knownConditionTrueFalse]\n" "[test.cpp:4:9]: (style) Condition '!b()' is always true [knownConditionTrueFalse]\n", errout_str()); + + check("int g();\n" // a value modified inside a nested branch must be lowered to possible + "void f(int outer, int inner) {\n" + " int bits = 0;\n" + " if (outer) {\n" + " if (inner == 1)\n" + " bits = g();\n" + " }\n" + " if (bits > 0) {}\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("int g();\n" // the modifying branch has an escaping sibling - still must be lowered + "void f(int t, int u) {\n" + " int v = 0;\n" + " if (t) {\n" + " if (u == 2)\n" + " v = g();\n" + " else\n" + " return;\n" + " }\n" + " if (v > 0) {}\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); } void alwaysTrueSymbolic() diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 369c04a2101..2483a967947 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -2450,6 +2450,9 @@ class TestNullPointer : public TestFixture { void nullpointer77() { + // No warning: 'i' is passed to the unknown function 'h' in the same condition that guards the + // dereference. 'h' may validate the pointer (e.g. return false for null), so '*i' can be safe + // - this is the common "if (check(p) && p->...)" pattern, so we must not assume 'i' is null. check("bool h(int*);\n" "void f(int* i) {\n" " int* i = nullptr;\n" @@ -2465,6 +2468,8 @@ class TestNullPointer : public TestFixture { "}\n"); ASSERT_EQUALS("", errout_str()); + // Likewise here, even though 'i' is null when the first 'h(i)' was true: the second 'h(i)' is an + // independent call that may validate 'i', so '*i' is not necessarily a null dereference. check("bool h(int*);\n" "void f(int* x) {\n" " int* i = x;\n" diff --git a/test/testother.cpp b/test/testother.cpp index b8134623a37..09ac2c69f21 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -11491,8 +11491,9 @@ class TestOther : public TestFixture { " x = a + b;\n" " return x;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:2:11] -> [test.cpp:4:9]: (style) Variable 'x' is assigned an expression that holds the same value. [redundantAssignment]\n", - errout_str()); + ASSERT_EQUALS( + "[test.cpp:2:11] -> [test.cpp:3:9] -> [test.cpp:4:9]: (style) Variable 'x' is assigned an expression that holds the same value. [redundantAssignment]\n", + errout_str()); } void varFuncNullUB() { // #4482 diff --git a/test/teststl.cpp b/test/teststl.cpp index 35941f6422c..93b53b43671 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -368,7 +368,9 @@ class TestStl : public TestFixture { " if(b) ++x;\n" " return s[x];\n" "}"); - ASSERT_EQUALS("[test.cpp:5:13]: error: Out of bounds access in 's[x]', if 's' size is 6 and 'x' is 6 [containerOutOfBounds]\n", errout_str()); + ASSERT_EQUALS( + "[test.cpp:5:13]: error: Out of bounds access in 's[x]', if 's' size is 6 and 'x' is 7 [containerOutOfBounds]\n", + errout_str()); checkNormal("void f() {\n" " static const int N = 4;\n" diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index e0c5512f1fd..6a43493cd0d 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4265,7 +4265,7 @@ class TestUninitVar : public TestFixture { " else {}\n" " return y;\n" "}"); - TODO_ASSERT_EQUALS("", "[test.cpp:5:9] -> [test.cpp:7:12]: (warning) Uninitialized variable: y [uninitvar]\n", errout_str()); + ASSERT_EQUALS("", errout_str()); // #4560: escaping else keeps x known, so x is true and y is initialized valueFlowUninit("int f(int a) {\n" // #6583 " int x;\n" @@ -4284,7 +4284,8 @@ class TestUninitVar : public TestFixture { " else y = 123;\n" // <- y is always initialized " return y;\n" "}"); - TODO_ASSERT_EQUALS("", "[test.cpp:5:9] -> [test.cpp:7:12]: (warning) Uninitialized variable: y [uninitvar]\n", errout_str()); + ASSERT_EQUALS("", + errout_str()); // #4560: fork-based condition analysis tracks x==0 -> else branch -> y initialized valueFlowUninit("void f(int x) {\n" // #3948 " int value;\n" @@ -5614,6 +5615,47 @@ class TestUninitVar : public TestFixture { "}"); ASSERT_EQUALS("[test.cpp:18:9] -> [test.cpp:12:13] -> [test.cpp:8:15]: (warning) Uninitialized variable: s->flag [uninitvar]\n", errout_str()); + // A value narrowed by a nested condition (dimensions < 1 here) must survive the enclosing + // 'if' so a later correlated condition can be resolved: dimensions == 1 is then false, the + // function does not return, and the uninitialized read is reached. + valueFlowUninit("unsigned int g();\n" + "void f(bool a) {\n" + " unsigned int dimensions = 0;\n" + " bool mightBeLarger;\n" + " if (a) {\n" + " dimensions = g();\n" + " if (dimensions >= 1)\n" + " mightBeLarger = false;\n" + " } else {\n" + " mightBeLarger = false;\n" + " }\n" + " if (dimensions == 1)\n" + " return;\n" + " if (!mightBeLarger) {}\n" + "}"); + ASSERT_EQUALS( + "[test.cpp:5:9] -> [test.cpp:7:24] -> [test.cpp:14:10]: (warning) Uninitialized variable: mightBeLarger [uninitvar]\n", + errout_str()); + + // Same shape, but the later condition (dimensions == 0) is implied by the narrowing, so the + // early return fires on the uninitialized path and there must be no warning. + valueFlowUninit("unsigned int g();\n" + "void f(bool a) {\n" + " unsigned int dimensions = 0;\n" + " bool mightBeLarger;\n" + " if (a) {\n" + " dimensions = g();\n" + " if (dimensions >= 1)\n" + " mightBeLarger = false;\n" + " } else {\n" + " mightBeLarger = false;\n" + " }\n" + " if (dimensions == 0)\n" + " return;\n" + " if (!mightBeLarger) {}\n" + "}"); + ASSERT_EQUALS("", errout_str()); + // Ticket #2207 - False negative valueFlowUninit("void foo() {\n" " int a;\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 8305a63bc56..dd179a51c80 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -6097,9 +6097,9 @@ class TestValueFlow : public TestFixture { " c++;\n" "}\n"; values = tokenValues(code, "c ++ ; }"); - TODO_ASSERT_EQUALS(true, false, values.size() == 2); - // ASSERT_EQUALS(true, values.front().isUninitValue() || values.back().isUninitValue()); - // ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible()); + ASSERT_EQUALS(true, values.size() == 2); + ASSERT_EQUALS(true, values.front().isUninitValue() || values.back().isUninitValue()); + ASSERT_EQUALS(true, values.front().isPossible() || values.back().isPossible()); // ASSERT_EQUALS(true, values.front().intvalue == 0 || values.back().intvalue == 0); code = "void b(bool d, bool e) {\n"