ProgramMemory: copy-on-write mValues [skip ci]

Co-authored-by: Paul Fultz II <[email protected]>
danmar · Jul 29, 2024 · 0fc028e · 0fc028e
1 parent 3edeccb
commit 0fc028e
Show file tree

Hide file tree

Showing 6 changed files with 145 additions and 36 deletions.
diff --git a/Makefile b/Makefile
@@ -335,6 +335,7 @@ TESTOBJ =     test/fixture.o \
               test/testpostfixoperator.o \
               test/testpreprocessor.o \
               test/testprocessexecutor.o \
+              test/testprogrammemory.o \
               test/testsettings.o \
               test/testsimplifytemplate.o \
               test/testsimplifytokens.o \
@@ -902,6 +903,9 @@ test/testpreprocessor.o: test/testpreprocessor.cpp externals/simplecpp/simplecpp
 test/testprocessexecutor.o: test/testprocessexecutor.cpp cli/executor.h cli/processexecutor.h externals/simplecpp/simplecpp.h lib/addoninfo.h lib/analyzerinfo.h lib/check.h lib/color.h lib/config.h lib/cppcheck.h lib/errorlogger.h lib/errortypes.h lib/filesettings.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h test/redirect.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testprocessexecutor.cpp
 
+test/testprogrammemory.o: test/testprogrammemory.cpp externals/simplecpp/simplecpp.h lib/addoninfo.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/preprocessor.h lib/programmemory.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h test/fixture.h test/helpers.h
+	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testprogrammemory.cpp
+
 test/testsettings.o: test/testsettings.cpp externals/simplecpp/simplecpp.h lib/addoninfo.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/tokenize.h lib/tokenlist.h lib/utils.h test/fixture.h test/helpers.h
 	$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsettings.cpp
 

diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp
@@ -57,7 +57,9 @@ std::size_t ExprIdToken::Hash::operator()(ExprIdToken etok) const
 }
 
 void ProgramMemory::setValue(const Token* expr, const ValueFlow::Value& value) {
-    mValues[expr] = value;
+    copyOnWrite();
+
+    (*mValues)[expr] = value;
     ValueFlow::Value subvalue = value;
     const Token* subexpr = solveExprValue(
         expr,
@@ -71,12 +73,12 @@ void ProgramMemory::setValue(const Token* expr, const ValueFlow::Value& value) {
     },
         subvalue);
     if (subexpr)
-        mValues[subexpr] = std::move(subvalue);
+        (*mValues)[subexpr] = std::move(subvalue);
 }
 const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible) const
 {
-    const ProgramMemory::Map::const_iterator it = mValues.find(exprid);
-    const bool found = it != mValues.cend() && (impossible || !it->second.isImpossible());
+    const ProgramMemory::Map::const_iterator it = mValues->find(exprid);
+    const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());
     if (found)
         return &it->second;
     return nullptr;
@@ -147,26 +149,38 @@ void ProgramMemory::setContainerSizeValue(const Token* expr, MathLib::bigint val
 }
 
 void ProgramMemory::setUnknown(const Token* expr) {
-    mValues[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
+    copyOnWrite();
+
+    (*mValues)[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
 }
 
 bool ProgramMemory::hasValue(nonneg int exprid)
 {
-    return mValues.find(exprid) != mValues.end();
+    return mValues->find(exprid) != mValues->end();
 }
 
 const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
-    return mValues.at(exprid);
+    return mValues->at(exprid);
 }
 ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
-    return mValues.at(exprid);
+    copyOnWrite();
+
+    return mValues->at(exprid);
 }
 
 void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
 {
-    for (auto it = mValues.begin(); it != mValues.end();) {
+    if (mValues->begin() == mValues->end())
+        return;
+
+    // TODO: how to delay until we actuallly modify?
+    copyOnWrite();
+
+    for (auto it = mValues->begin(); it != mValues->end();) {
         if (pred(it->first))
-            it = mValues.erase(it);
+        {
+            it = mValues->erase(it);
+        }
         else
             ++it;
     }
@@ -179,25 +193,39 @@ void ProgramMemory::swap(ProgramMemory &pm)
 
 void ProgramMemory::clear()
 {
-    mValues.clear();
+    copyOnWrite();
+
+    mValues->clear();
 }
 
 bool ProgramMemory::empty() const
 {
-    return mValues.empty();
+    return mValues->empty();
 }
 
 void ProgramMemory::replace(ProgramMemory pm)
 {
-    for (auto&& p : pm.mValues) {
-        mValues[p.first] = std::move(p.second);
+    copyOnWrite();
+
+    for (auto&& p : (*pm.mValues)) {
+        (*mValues)[p.first] = std::move(p.second);
     }
 }
 
 void ProgramMemory::insert(const ProgramMemory &pm)
 {
+    copyOnWrite();
+
     for (auto&& p : pm)
-        mValues.insert(p);
+        mValues->insert(p);
+}
+
+void ProgramMemory::copyOnWrite()
+{
+    if (mValues.use_count() == 1)
+        return;
+
+    mValues = std::shared_ptr<Map>(new Map(*mValues));
 }
 
 static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings);

diff --git a/lib/programmemory.h b/lib/programmemory.h
@@ -96,12 +96,12 @@ struct ExprIdToken {
     };
 };
 
-struct ProgramMemory {
+struct CPPCHECKLIB ProgramMemory {
     using Map = std::unordered_map<ExprIdToken, ValueFlow::Value, ExprIdToken::Hash>;
 
-    ProgramMemory() = default;
+    ProgramMemory() : mValues(new Map()) {}
 
-    explicit ProgramMemory(Map values) : mValues(std::move(values)) {}
+    explicit ProgramMemory(std::shared_ptr<Map> values) : mValues(std::move(values)) {}
 
     void setValue(const Token* expr, const ValueFlow::Value& value);
     const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const;
@@ -134,19 +134,19 @@ struct ProgramMemory {
     void insert(const ProgramMemory &pm);
 
     Map::iterator begin() {
-        return mValues.begin();
+        return mValues->begin();
     }
 
     Map::iterator end() {
-        return mValues.end();
+        return mValues->end();
     }
 
     Map::const_iterator begin() const {
-        return mValues.begin();
+        return mValues->begin();
     }
 
     Map::const_iterator end() const {
-        return mValues.end();
+        return mValues->end();
     }
 
     friend bool operator==(const ProgramMemory& x, const ProgramMemory& y) {
@@ -158,7 +158,9 @@ struct ProgramMemory {
     }
 
 private:
-    Map mValues;
+    void copyOnWrite();
+
+    std::shared_ptr<Map> mValues;
 };
 
 struct ProgramMemoryState {

diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp
@@ -896,7 +896,7 @@ struct ValueFlowAnalyzer : Analyzer {
 
     using ProgramState = ProgramMemory::Map;
 
-    virtual ProgramState getProgramState() const = 0;
+    virtual std::shared_ptr<ProgramState> getProgramState() const = 0;
 
     virtual int getIndirect(const Token* tok) const {
         const ValueFlow::Value* value = getValue(tok);
@@ -1421,7 +1421,7 @@ struct ValueFlowAnalyzer : Analyzer {
     {
         if (e == Evaluate::Integral) {
             return evaluateInt(tok, [&] {
-                return pms.get(tok, ctx, getProgramState());
+                return pms.get(tok, ctx, *getProgramState());
             });
         }
         if (e == Evaluate::ContainerEmpty) {
@@ -1430,7 +1430,7 @@ struct ValueFlowAnalyzer : Analyzer {
             });
             if (value)
                 return {value->intvalue == 0};
-            ProgramMemory pm = pms.get(tok, ctx, getProgramState());
+            ProgramMemory pm = pms.get(tok, ctx, *getProgramState());
             MathLib::bigint out = 0;
             if (pm.getContainerEmptyValue(tok->exprId(), out))
                 return {static_cast<int>(out)};
@@ -1442,7 +1442,7 @@ struct ValueFlowAnalyzer : Analyzer {
     void assume(const Token* tok, bool state, unsigned int flags) override {
         // Update program state
         pms.removeModifiedVars(tok);
-        pms.addState(tok, getProgramState());
+        pms.addState(tok, *getProgramState());
         pms.assume(tok, state, flags & Assume::ContainerEmpty);
 
         bool isCondBlock = false;
@@ -1458,10 +1458,10 @@ struct ValueFlowAnalyzer : Analyzer {
             const Token* endBlock = startBlock->link();
             if (state) {
                 pms.removeModifiedVars(endBlock);
-                pms.addState(endBlock->previous(), getProgramState());
+                pms.addState(endBlock->previous(), *getProgramState());
             } else {
                 if (Token::simpleMatch(endBlock, "} else {"))
-                    pms.addState(endBlock->linkAt(2)->previous(), getProgramState());
+                    pms.addState(endBlock->linkAt(2)->previous(), *getProgramState());
             }
         }
 
@@ -1482,7 +1482,7 @@ struct ValueFlowAnalyzer : Analyzer {
     {
         // Update program state
         pms.removeModifiedVars(tok);
-        pms.addState(tok, getProgramState());
+        pms.addState(tok, *getProgramState());
     }
 
     virtual void internalUpdate(Token* /*tok*/, const ValueFlow::Value& /*v*/, Direction /*d*/)
@@ -1732,9 +1732,9 @@ struct ExpressionAnalyzer : SingleValueFlowAnalyzer {
         return unknown;
     }
 
-    ProgramState getProgramState() const override {
-        ProgramState ps;
-        ps[expr] = value;
+     std::shared_ptr<ProgramState> getProgramState() const override {
+        auto ps = std::make_shared<ProgramState>();
+        (*ps)[expr] = value;
         return ps;
     }
 
@@ -5887,13 +5887,13 @@ struct MultiValueFlowAnalyzer : ValueFlowAnalyzer {
         return values.count(tok->varId()) > 0;
     }
 
-    ProgramState getProgramState() const override {
-        ProgramState ps;
+    std::shared_ptr<ProgramState> getProgramState() const override {
+        auto ps = std::make_shared<ProgramState>();
         for (const auto& p : values) {
             const Variable* var = vars.at(p.first);
             if (!var)
                 continue;
-            ps[var->nameToken()] = p.second;
+            (*ps)[var->nameToken()] = p.second;
         }
         return ps;
     }

diff --git a/test/testprogrammemory.cpp b/test/testprogrammemory.cpp
@@ -0,0 +1,74 @@
+/*
+* Cppcheck - A tool for static C/C++ code analysis
+ * Copyright (C) 2007-2024 Cppcheck team.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "fixture.h"
+#include "helpers.h"
+#include "programmemory.h"
+#include "vfvalue.h"
+
+class TestProgramMemory : public TestFixture {
+public:
+    TestProgramMemory() : TestFixture("TestProgramMemory") {}
+
+private:
+    void run() override {
+        TEST_CASE(copyOnWrite);
+    }
+
+    void copyOnWrite() {
+        SimpleTokenList tokenlist("1+1;");
+        Token* tok = tokenlist.front();
+        const nonneg int id = 123;
+        tok->exprId(id);
+
+        ProgramMemory pm;
+        const ValueFlow::Value* v = pm.getValue(id);
+        ASSERT(!v);
+        pm.setValue(tok, ValueFlow::Value{42});
+
+        v = pm.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(42, v->intvalue);
+
+        // create a copy
+        ProgramMemory pm2 = pm;
+
+        // make sure the value was copied
+        v = pm2.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(42, v->intvalue);
+
+        // set a value in the copy to trigger copy-on-write
+        pm2.setValue(tok, ValueFlow::Value{43});
+
+        // make sure the value was set
+        v = pm2.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(43, v->intvalue);
+
+        // make sure the original value remains unchanged
+        v = pm.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(42, v->intvalue);
+    }
+
+    // TODO: test swap()
+    // TODO: test more levels of inheritence
+};
+
+REGISTER_TEST(TestProgramMemory)
diff --git a/test/testrunner.vcxproj b/test/testrunner.vcxproj
@@ -83,6 +83,7 @@
     <ClCompile Include="testpostfixoperator.cpp" />
     <ClCompile Include="testpreprocessor.cpp" />
     <ClCompile Include="testprocessexecutor.cpp" />
+    <ClCompile Include="testprogrammemory.cpp" />
     <ClCompile Include="testsettings.cpp" />
     <ClCompile Include="testsimplifytemplate.cpp" />
     <ClCompile Include="testsimplifytokens.cpp" />