ProgramMemory: only copy mValues on write (#6646)

Co-authored-by: Paul Fultz II <[email protected]>
danmar · Jul 31, 2024 · 53e55be · 53e55be
1 parent 02e2230
commit 53e55be
Show file tree

Hide file tree

Showing 5 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/templatesimplifier.h lib/token.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,36 @@ 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->empty())
+        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 +191,38 @@ void ProgramMemory::swap(ProgramMemory &pm)
 
 void ProgramMemory::clear()
 {
-    mValues.clear();
+    if (mValues->empty())
+        return;
+
+    copyOnWrite();
+
+    mValues->clear();
 }
 
 bool ProgramMemory::empty() const
 {
-    return mValues.empty();
+    return mValues->empty();
 }
 
+// NOLINTNEXTLINE(performance-unnecessary-value-param) - technically correct but we are moving the given values
 void ProgramMemory::replace(ProgramMemory pm)
 {
-    for (auto&& p : pm.mValues) {
-        mValues[p.first] = std::move(p.second);
+    if (pm.empty())
+        return;
+
+    copyOnWrite();
+
+    for (auto&& p : (*pm.mValues)) {
+        (*mValues)[p.first] = std::move(p.second);
     }
 }
 
-void ProgramMemory::insert(const ProgramMemory &pm)
+void ProgramMemory::copyOnWrite()
 {
-    for (auto&& p : pm)
-        mValues.insert(p);
+    if (mValues.use_count() == 1)
+        return;
+
+    mValues = std::make_shared<Map>(*mValues);
 }
 
 static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings);
@@ -448,14 +473,6 @@ ProgramMemoryState::ProgramMemoryState(const Settings* s) : settings(s)
     assert(settings != nullptr);
 }
 
-void ProgramMemoryState::insert(const ProgramMemory &pm, const Token* origin)
-{
-    if (origin)
-        for (auto&& p : pm)
-            origins.insert(std::make_pair(p.first.getExpressionId(), origin));
-    state.insert(pm);
-}
-
 void ProgramMemoryState::replace(ProgramMemory pm, const Token* origin)
 {
     if (origin)

diff --git a/lib/programmemory.h b/lib/programmemory.h
@@ -26,6 +26,7 @@
 #include <cstddef>
 #include <functional>
 #include <map>
+#include <memory>
 #include <string>
 #include <unordered_map>
 #include <utility>
@@ -96,12 +97,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(Map values) : mValues(new Map(std::move(values))) {}
 
     void setValue(const Token* expr, const ValueFlow::Value& value);
     const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const;
@@ -131,22 +132,24 @@ struct ProgramMemory {
 
     void replace(ProgramMemory pm);
 
-    void insert(const ProgramMemory &pm);
-
     Map::iterator begin() {
-        return mValues.begin();
+        copyOnWrite();
+
+        return mValues->begin();
     }
 
     Map::iterator end() {
-        return mValues.end();
+        copyOnWrite();
+
+        return mValues->end();
     }
 
     Map::const_iterator begin() const {
-        return mValues.begin();
+        return mValues->cbegin();
     }
 
     Map::const_iterator end() const {
-        return mValues.end();
+        return mValues->cend();
     }
 
     friend bool operator==(const ProgramMemory& x, const ProgramMemory& y) {
@@ -158,7 +161,9 @@ struct ProgramMemory {
     }
 
 private:
-    Map mValues;
+    void copyOnWrite();
+
+    std::shared_ptr<Map> mValues;
 };
 
 struct ProgramMemoryState {
@@ -168,7 +173,6 @@ struct ProgramMemoryState {
 
     explicit ProgramMemoryState(const Settings* s);
 
-    void insert(const ProgramMemory &pm, const Token* origin = nullptr);
     void replace(ProgramMemory pm, const Token* origin = nullptr);
 
     void addState(const Token* tok, const ProgramMemory::Map& vars);

diff --git a/test/testprogrammemory.cpp b/test/testprogrammemory.cpp
@@ -0,0 +1,83 @@
+/*
+ * 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 "token.h"
+#include "programmemory.h"
+#include "vfvalue.h"
+
+class TestProgramMemory : public TestFixture {
+public:
+    TestProgramMemory() : TestFixture("TestProgramMemory") {}
+
+private:
+    void run() override {
+        TEST_CASE(copyOnWrite);
+    }
+
+    void copyOnWrite() const {
+        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{41});
+
+        v = pm.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(41, v->intvalue);
+
+        // create a copy
+        ProgramMemory pm2 = pm;
+
+        // make sure the value was copied
+        v = pm2.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(41, v->intvalue);
+
+        // set a value in the copy to trigger copy-on-write
+        pm2.setValue(tok, ValueFlow::Value{42});
+
+        // make another copy and set another value
+        ProgramMemory pm3 = pm2;
+
+        // set a value in the copy to trigger copy-on-write
+        pm3.setValue(tok, ValueFlow::Value{43});
+
+        // make sure the value was set
+        v = pm2.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(42, v->intvalue);
+
+        // make sure the value was set
+        v = pm3.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(43, v->intvalue);
+
+        // make sure the original value remains unchanged
+        v = pm.getValue(id);
+        ASSERT(v);
+        ASSERT_EQUALS(41, v->intvalue);
+    }
+};
+
+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" />