Skip to content

Commit 53e55be

Browse files
firewavepfultz2
firewave
and
pfultz2
authored
ProgramMemory: only copy mValues on write (#6646)
Co-authored-by: Paul Fultz II <paul.fultz@amd.com>
1 parent 02e2230 commit 53e55be

5 files changed

Lines changed: 145 additions & 36 deletions

File tree

‎Makefile‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@ TESTOBJ = test/fixture.o \
335335
test/testpostfixoperator.o \
336336
test/testpreprocessor.o \
337337
test/testprocessexecutor.o \
338+
test/testprogrammemory.o \
338339
test/testsettings.o \
339340
test/testsimplifytemplate.o \
340341
test/testsimplifytokens.o \
@@ -902,6 +903,9 @@ test/testpreprocessor.o: test/testpreprocessor.cpp externals/simplecpp/simplecpp
902903
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
903904
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testprocessexecutor.cpp
904905

906+
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
907+
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testprogrammemory.cpp
908+
905909
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
906910
$(CXX) ${INCLUDE_FOR_TEST} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ test/testsettings.cpp
907911

‎lib/programmemory.cpp‎

Lines changed: 42 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ std::size_t ExprIdToken::Hash::operator()(ExprIdToken etok) const
5757
}
5858

5959
void ProgramMemory::setValue(const Token* expr, const ValueFlow::Value& value) {
60-
mValues[expr] = value;
60+
copyOnWrite();
61+
62+
(*mValues)[expr] = value;
6163
ValueFlow::Value subvalue = value;
6264
const Token* subexpr = solveExprValue(
6365
expr,
@@ -71,12 +73,12 @@ void ProgramMemory::setValue(const Token* expr, const ValueFlow::Value& value) {
7173
},
7274
subvalue);
7375
if (subexpr)
74-
mValues[subexpr] = std::move(subvalue);
76+
(*mValues)[subexpr] = std::move(subvalue);
7577
}
7678
const ValueFlow::Value* ProgramMemory::getValue(nonneg int exprid, bool impossible) const
7779
{
78-
const ProgramMemory::Map::const_iterator it = mValues.find(exprid);
79-
const bool found = it != mValues.cend() && (impossible || !it->second.isImpossible());
80+
const ProgramMemory::Map::const_iterator it = mValues->find(exprid);
81+
const bool found = it != mValues->cend() && (impossible || !it->second.isImpossible());
8082
if (found)
8183
return &it->second;
8284
return nullptr;
@@ -147,26 +149,36 @@ void ProgramMemory::setContainerSizeValue(const Token* expr, MathLib::bigint val
147149
}
148150

149151
void ProgramMemory::setUnknown(const Token* expr) {
150-
mValues[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
152+
copyOnWrite();
153+
154+
(*mValues)[expr].valueType = ValueFlow::Value::ValueType::UNINIT;
151155
}
152156

153157
bool ProgramMemory::hasValue(nonneg int exprid)
154158
{
155-
return mValues.find(exprid) != mValues.end();
159+
return mValues->find(exprid) != mValues->end();
156160
}
157161

158162
const ValueFlow::Value& ProgramMemory::at(nonneg int exprid) const {
159-
return mValues.at(exprid);
163+
return mValues->at(exprid);
160164
}
161165
ValueFlow::Value& ProgramMemory::at(nonneg int exprid) {
162-
return mValues.at(exprid);
166+
copyOnWrite();
167+
168+
return mValues->at(exprid);
163169
}
164170

165171
void ProgramMemory::erase_if(const std::function<bool(const ExprIdToken&)>& pred)
166172
{
167-
for (auto it = mValues.begin(); it != mValues.end();) {
173+
if (mValues->empty())
174+
return;
175+
176+
// TODO: how to delay until we actuallly modify?
177+
copyOnWrite();
178+
179+
for (auto it = mValues->begin(); it != mValues->end();) {
168180
if (pred(it->first))
169-
it = mValues.erase(it);
181+
it = mValues->erase(it);
170182
else
171183
++it;
172184
}
@@ -179,25 +191,38 @@ void ProgramMemory::swap(ProgramMemory &pm)
179191

180192
void ProgramMemory::clear()
181193
{
182-
mValues.clear();
194+
if (mValues->empty())
195+
return;
196+
197+
copyOnWrite();
198+
199+
mValues->clear();
183200
}
184201

185202
bool ProgramMemory::empty() const
186203
{
187-
return mValues.empty();
204+
return mValues->empty();
188205
}
189206

207+
// NOLINTNEXTLINE(performance-unnecessary-value-param) - technically correct but we are moving the given values
190208
void ProgramMemory::replace(ProgramMemory pm)
191209
{
192-
for (auto&& p : pm.mValues) {
193-
mValues[p.first] = std::move(p.second);
210+
if (pm.empty())
211+
return;
212+
213+
copyOnWrite();
214+
215+
for (auto&& p : (*pm.mValues)) {
216+
(*mValues)[p.first] = std::move(p.second);
194217
}
195218
}
196219

197-
void ProgramMemory::insert(const ProgramMemory &pm)
220+
void ProgramMemory::copyOnWrite()
198221
{
199-
for (auto&& p : pm)
200-
mValues.insert(p);
222+
if (mValues.use_count() == 1)
223+
return;
224+
225+
mValues = std::make_shared<Map>(*mValues);
201226
}
202227

203228
static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm, const Settings& settings);
@@ -448,14 +473,6 @@ ProgramMemoryState::ProgramMemoryState(const Settings* s) : settings(s)
448473
assert(settings != nullptr);
449474
}
450475

451-
void ProgramMemoryState::insert(const ProgramMemory &pm, const Token* origin)
452-
{
453-
if (origin)
454-
for (auto&& p : pm)
455-
origins.insert(std::make_pair(p.first.getExpressionId(), origin));
456-
state.insert(pm);
457-
}
458-
459476
void ProgramMemoryState::replace(ProgramMemory pm, const Token* origin)
460477
{
461478
if (origin)

‎lib/programmemory.h‎

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include <cstddef>
2727
#include <functional>
2828
#include <map>
29+
#include <memory>
2930
#include <string>
3031
#include <unordered_map>
3132
#include <utility>
@@ -96,12 +97,12 @@ struct ExprIdToken {
9697
};
9798
};
9899

99-
struct ProgramMemory {
100+
struct CPPCHECKLIB ProgramMemory {
100101
using Map = std::unordered_map<ExprIdToken, ValueFlow::Value, ExprIdToken::Hash>;
101102

102-
ProgramMemory() = default;
103+
ProgramMemory() : mValues(new Map()) {}
103104

104-
explicit ProgramMemory(Map values) : mValues(std::move(values)) {}
105+
explicit ProgramMemory(Map values) : mValues(new Map(std::move(values))) {}
105106

106107
void setValue(const Token* expr, const ValueFlow::Value& value);
107108
const ValueFlow::Value* getValue(nonneg int exprid, bool impossible = false) const;
@@ -131,22 +132,24 @@ struct ProgramMemory {
131132

132133
void replace(ProgramMemory pm);
133134

134-
void insert(const ProgramMemory &pm);
135-
136135
Map::iterator begin() {
137-
return mValues.begin();
136+
copyOnWrite();
137+
138+
return mValues->begin();
138139
}
139140

140141
Map::iterator end() {
141-
return mValues.end();
142+
copyOnWrite();
143+
144+
return mValues->end();
142145
}
143146

144147
Map::const_iterator begin() const {
145-
return mValues.begin();
148+
return mValues->cbegin();
146149
}
147150

148151
Map::const_iterator end() const {
149-
return mValues.end();
152+
return mValues->cend();
150153
}
151154

152155
friend bool operator==(const ProgramMemory& x, const ProgramMemory& y) {
@@ -158,7 +161,9 @@ struct ProgramMemory {
158161
}
159162

160163
private:
161-
Map mValues;
164+
void copyOnWrite();
165+
166+
std::shared_ptr<Map> mValues;
162167
};
163168

164169
struct ProgramMemoryState {
@@ -168,7 +173,6 @@ struct ProgramMemoryState {
168173

169174
explicit ProgramMemoryState(const Settings* s);
170175

171-
void insert(const ProgramMemory &pm, const Token* origin = nullptr);
172176
void replace(ProgramMemory pm, const Token* origin = nullptr);
173177

174178
void addState(const Token* tok, const ProgramMemory::Map& vars);

‎test/testprogrammemory.cpp‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/*
2+
* Cppcheck - A tool for static C/C++ code analysis
3+
* Copyright (C) 2007-2024 Cppcheck team.
4+
*
5+
* This program is free software: you can redistribute it and/or modify
6+
* it under the terms of the GNU General Public License as published by
7+
* the Free Software Foundation, either version 3 of the License, or
8+
* (at your option) any later version.
9+
*
10+
* This program is distributed in the hope that it will be useful,
11+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
12+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
13+
* GNU General Public License for more details.
14+
*
15+
* You should have received a copy of the GNU General Public License
16+
* along with this program. If not, see <http://www.gnu.org/licenses/>.
17+
*/
18+
19+
#include "fixture.h"
20+
#include "helpers.h"
21+
#include "token.h"
22+
#include "programmemory.h"
23+
#include "vfvalue.h"
24+
25+
class TestProgramMemory : public TestFixture {
26+
public:
27+
TestProgramMemory() : TestFixture("TestProgramMemory") {}
28+
29+
private:
30+
void run() override {
31+
TEST_CASE(copyOnWrite);
32+
}
33+
34+
void copyOnWrite() const {
35+
SimpleTokenList tokenlist("1+1;");
36+
Token* tok = tokenlist.front();
37+
const nonneg int id = 123;
38+
tok->exprId(id);
39+
40+
ProgramMemory pm;
41+
const ValueFlow::Value* v = pm.getValue(id);
42+
ASSERT(!v);
43+
pm.setValue(tok, ValueFlow::Value{41});
44+
45+
v = pm.getValue(id);
46+
ASSERT(v);
47+
ASSERT_EQUALS(41, v->intvalue);
48+
49+
// create a copy
50+
ProgramMemory pm2 = pm;
51+
52+
// make sure the value was copied
53+
v = pm2.getValue(id);
54+
ASSERT(v);
55+
ASSERT_EQUALS(41, v->intvalue);
56+
57+
// set a value in the copy to trigger copy-on-write
58+
pm2.setValue(tok, ValueFlow::Value{42});
59+
60+
// make another copy and set another value
61+
ProgramMemory pm3 = pm2;
62+
63+
// set a value in the copy to trigger copy-on-write
64+
pm3.setValue(tok, ValueFlow::Value{43});
65+
66+
// make sure the value was set
67+
v = pm2.getValue(id);
68+
ASSERT(v);
69+
ASSERT_EQUALS(42, v->intvalue);
70+
71+
// make sure the value was set
72+
v = pm3.getValue(id);
73+
ASSERT(v);
74+
ASSERT_EQUALS(43, v->intvalue);
75+
76+
// make sure the original value remains unchanged
77+
v = pm.getValue(id);
78+
ASSERT(v);
79+
ASSERT_EQUALS(41, v->intvalue);
80+
}
81+
};
82+
83+
REGISTER_TEST(TestProgramMemory)

‎test/testrunner.vcxproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@
8383
<ClCompile Include="testpostfixoperator.cpp" />
8484
<ClCompile Include="testpreprocessor.cpp" />
8585
<ClCompile Include="testprocessexecutor.cpp" />
86+
<ClCompile Include="testprogrammemory.cpp" />
8687
<ClCompile Include="testsettings.cpp" />
8788
<ClCompile Include="testsimplifytemplate.cpp" />
8889
<ClCompile Include="testsimplifytokens.cpp" />

0 commit comments

Comments
 (0)