From 26ed052e8dfe52c022b9a8415866559778edd4c8 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Mon, 5 Jun 2023 13:49:19 -0500 Subject: [PATCH] Skip forwarding values for unique expressions (#5103) --- lib/analyzer.h | 3 +++ lib/forwardanalyzer.cpp | 4 ++++ lib/reverseanalyzer.cpp | 2 ++ lib/symboldatabase.cpp | 23 ++++++++++++++++++++++- lib/token.h | 19 +++++++++++++++++++ lib/valueflow.cpp | 33 +++++++++++++++++++++++++++------ test/testsimplifytypedef.cpp | 4 +++- test/testvarid.cpp | 17 +++++++++-------- 8 files changed, 89 insertions(+), 16 deletions(-) diff --git a/lib/analyzer.h b/lib/analyzer.h index add89f89e..51da0281f 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -181,6 +181,9 @@ struct Analyzer { virtual void assume(const Token* tok, bool state, unsigned int flags = 0) = 0; /// Return analyzer for expression at token virtual ValuePtr reanalyze(Token* tok, const std::string& msg = emptyString) const = 0; + virtual bool invalid() const { + return false; + } virtual ~Analyzer() {} Analyzer(const Analyzer&) = default; protected: diff --git a/lib/forwardanalyzer.cpp b/lib/forwardanalyzer.cpp index 73e98f015..224d6e9ee 100644 --- a/lib/forwardanalyzer.cpp +++ b/lib/forwardanalyzer.cpp @@ -894,6 +894,8 @@ struct ForwardTraversal { Analyzer::Result valueFlowGenericForward(Token* start, const Token* end, const ValuePtr& a, const Settings& settings) { + if (a->invalid()) + return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail}; ForwardTraversal ft{a, settings}; ft.updateRange(start, end); return Analyzer::Result{ ft.actions, ft.terminate }; @@ -903,6 +905,8 @@ Analyzer::Result valueFlowGenericForward(Token* start, const ValuePtr& { if (Settings::terminated()) throw TerminateException(); + if (a->invalid()) + return Analyzer::Result{Analyzer::Action::None, Analyzer::Terminate::Bail}; ForwardTraversal ft{a, settings}; ft.updateRecursive(start); return Analyzer::Result{ ft.actions, ft.terminate }; diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index f31584781..3436e3808 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -390,6 +390,8 @@ struct ReverseTraversal { void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr& a, const Settings& settings) { + if (a->invalid()) + return; ReverseTraversal rt{a, settings}; rt.traverse(start, end); } diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 7991a0010..9af7e54f4 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -1625,7 +1625,7 @@ void SymbolDatabase::createSymbolDatabaseExprIds() exprs[tok->str()].push_back(tok); tok->exprId(id++); - if (id == std::numeric_limits::max()) { + if (id == std::numeric_limits::max() / 4) { throw InternalError(nullptr, "Ran out of expression ids.", InternalError::INTERNAL); } } else if (isCPP() && Token::simpleMatch(tok, "this")) { @@ -1653,6 +1653,27 @@ void SymbolDatabase::createSymbolDatabaseExprIds() } } } + // Mark expressions that are unique + std::unordered_map exprMap; + for (Token* tok = const_cast(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) { + if (tok->exprId() == 0) + continue; + auto p = exprMap.emplace(tok->exprId(), tok); + // Already exists so set it to null + if (!p.second) { + p.first->second = nullptr; + } + } + for (const auto& p : exprMap) { + if (!p.second) + continue; + if (p.second->variable()) { + const Variable* var = p.second->variable(); + if (var->nameToken() != p.second) + continue; + } + p.second->setUniqueExprId(); + } } } diff --git a/lib/token.h b/lib/token.h index fdc45a6f7..da519039f 100644 --- a/lib/token.h +++ b/lib/token.h @@ -901,6 +901,20 @@ public: mImpl->mExprId = id; } + void setUniqueExprId() + { + assert(mImpl->mExprId > 0); + mImpl->mExprId |= 1 << efIsUnique; + } + + bool isUniqueExprId() const + { + if (mImpl->mExprId > 0) { + return (mImpl->mExprId & (1 << efIsUnique)) != 0; + } + return false; + } + /** * For debugging purposes, prints token and all tokens * followed by it. @@ -1327,6 +1341,11 @@ private: fIsFinalType = (1ULL << 40), // Is this a type with final specifier }; + enum : uint64_t { + efMaxSize = sizeof(nonneg int) * 8, + efIsUnique = efMaxSize - 2, + }; + Token::Type mTokType; uint64_t mFlags; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 103ddfe19..04bddf20e 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2438,10 +2438,6 @@ struct ValueFlowAnalyzer : Analyzer { return false; } - virtual bool invalid() const { - return false; - } - bool isCPP() const { return tokenlist->isCPP(); } @@ -3134,11 +3130,20 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { bool local; bool unknown; bool dependOnThis; + bool uniqueExprId; - ExpressionAnalyzer() : SingleValueFlowAnalyzer(), expr(nullptr), local(true), unknown(false), dependOnThis(false) {} + ExpressionAnalyzer() + : SingleValueFlowAnalyzer(), expr(nullptr), local(true), unknown(false), dependOnThis(false), uniqueExprId(false) + {} ExpressionAnalyzer(const Token* e, ValueFlow::Value val, const TokenList* t, const Settings* s) - : SingleValueFlowAnalyzer(std::move(val), t, s), expr(e), local(true), unknown(false), dependOnThis(false) { + : SingleValueFlowAnalyzer(std::move(val), t, s), + expr(e), + local(true), + unknown(false), + dependOnThis(false), + uniqueExprId(false) + { assert(e && e->exprId() != 0 && "Not a valid expression"); dependOnThis = exprDependsOnThis(expr); @@ -3147,6 +3152,8 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { dependOnThis |= exprDependsOnThis(value.tokvalue); setupExprVarIds(value.tokvalue); } + uniqueExprId = + expr->isUniqueExprId() && (Token::Match(expr, "%cop%") || !isVariableChanged(expr, 0, s, t->isCPP())); } static bool nonLocal(const Variable* var, bool deref) { @@ -3196,7 +3203,13 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer { }); } + virtual bool skipUniqueExprIds() const { + return true; + } + bool invalid() const override { + if (skipUniqueExprIds() && uniqueExprId) + return true; return unknown; } @@ -3231,6 +3244,10 @@ struct SameExpressionAnalyzer : ExpressionAnalyzer { : ExpressionAnalyzer(e, std::move(val), t, s) {} + bool skipUniqueExprIds() const override { + return false; + } + bool match(const Token* tok) const override { return isSameExpression(isCPP(), true, expr, tok, getSettings()->library, true, true); @@ -3246,6 +3263,10 @@ struct OppositeExpressionAnalyzer : ExpressionAnalyzer { : ExpressionAnalyzer(e, std::move(val), t, s), isNot(pIsNot) {} + bool skipUniqueExprIds() const override { + return false; + } + bool match(const Token* tok) const override { return isOppositeCond(isNot, isCPP(), expr, tok, getSettings()->library, true, true); } diff --git a/test/testsimplifytypedef.cpp b/test/testsimplifytypedef.cpp index f85955016..336048cef 100644 --- a/test/testsimplifytypedef.cpp +++ b/test/testsimplifytypedef.cpp @@ -1602,7 +1602,9 @@ private: "}"; checkSimplifyTypedef(code); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS( + "[test.cpp:3]: (debug) valueflow.cpp:6541:(valueFlow) bailout: valueFlowAfterCondition: bailing in conditional block\n", + errout.str()); } void simplifyTypedef46() { diff --git a/test/testvarid.cpp b/test/testvarid.cpp index 586386df3..0fae1bbf7 100644 --- a/test/testvarid.cpp +++ b/test/testvarid.cpp @@ -3749,14 +3749,15 @@ private: " return x + y + a.y + b.y;\n" "}\n"); - const char expected[] = "1: struct A {\n" - "2: int x ; int y ;\n" - "3: } ;\n" - "4: int f ( A a , A b ) {\n" - "5: int x@5 ; x@5 =@9 a@3 .@10 x@6 +@11 b@4 .@12 x@7 ;\n" - "6: int y@8 ; y@8 =@13 b@4 .@12 x@7 +@11 a@3 .@10 x@6 ;\n" - "7: return x@5 +@17 y@8 +@18 a@3 .@19 y@9 +@20 b@4 .@21 y@10 ;\n" - "8: }\n"; + const char expected[] = + "1: struct A {\n" + "2: int x ; int y ;\n" + "3: } ;\n" + "4: int f ( A a , A b ) {\n" + "5: int x@5 ; x@5 =@9 a@3 .@10 x@6 +@11 b@4 .@12 x@7 ;\n" + "6: int y@8 ; y@8 =@1073741837 b@4 .@12 x@7 +@11 a@3 .@10 x@6 ;\n" + "7: return x@5 +@1073741841 y@8 +@1073741842 a@3 .@1073741843 y@9 +@1073741844 b@4 .@1073741845 y@10 ;\n" + "8: }\n"; ASSERT_EQUALS(expected, actual); }