Remove BindingEnv memory leaks.
Apart from the top-level one from the State instance,
all BindingEnv instances created by sub-parsers (i.e. when
processing included Ninja build plans) were leaked.
This patch ensures that all BindingEnv objects are owned
by the State instance, and destroyed with it.
Fuchsia-Topic: persistent-mode
Original-Change-Id: I00752fe23bdc7f537e01fee17a43fc0f3674b6ac
Change-Id: I9102fcdda54e9037def31b0c8ad0415952f8432e
Reviewed-on: https://fuchsia-review.googlesource.com/c/third_party/github.com/ninja-build/ninja/+/977218
Reviewed-by: David Fang <fangism@google.com>
Commit-Queue: David Turner <digit@google.com>
diff --git a/src/build.cc b/src/build.cc
index 4a280ae..b1012a7 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -525,7 +525,7 @@
scan_(state, build_log, deps_log, disk_interface,
&config_.depfile_parser_options) {
lock_file_path_ = ".ninja_lock";
- string build_dir = state_->bindings_.LookupVariable("builddir");
+ string build_dir = state_->bindings().LookupVariable("builddir");
if (!build_dir.empty())
lock_file_path_ = build_dir + "/" + lock_file_path_;
}
diff --git a/src/clean.cc b/src/clean.cc
index 2374ad0..e48eb8e 100644
--- a/src/clean.cc
+++ b/src/clean.cc
@@ -249,7 +249,7 @@
assert(rule);
Reset();
- const Rule* r = state_->bindings_.LookupRule(rule);
+ const Rule* r = state_->bindings().LookupRule(rule);
if (r) {
CleanRule(r);
} else {
@@ -267,7 +267,7 @@
LoadDyndeps();
for (int i = 0; i < rule_count; ++i) {
const char* rule_name = rules[i];
- const Rule* rule = state_->bindings_.LookupRule(rule_name);
+ const Rule* rule = state_->bindings().LookupRule(rule_name);
if (rule) {
if (IsVerbose())
printf("Rule %s\n", rule_name);
diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index c52925c..67a9dee 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -31,7 +31,7 @@
ManifestParserOptions options)
: Parser(state, file_reader),
options_(options), quiet_(false) {
- env_ = &state->bindings_;
+ env_ = &state->bindings();
}
bool ManifestParser::Parse(const string& filename, const string& input,
@@ -307,7 +307,7 @@
// Bindings on edges are rare, so allocate per-edge envs only when needed.
bool has_indent_token = lexer_.PeekToken(Lexer::INDENT);
- BindingEnv* env = has_indent_token ? new BindingEnv(env_) : env_;
+ BindingEnv* env = has_indent_token ? state_->CreateNewEnv(env_) : env_;
while (has_indent_token) {
string key;
EvalString val;
@@ -421,7 +421,7 @@
ManifestParser subparser(state_, file_reader_, options_);
if (new_scope) {
- subparser.env_ = new BindingEnv(env_);
+ subparser.env_ = state_->CreateNewEnv(env_);
} else {
subparser.env_ = env_;
}
diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc
index e41d8ae..4de646d 100644
--- a/src/manifest_parser_test.cc
+++ b/src/manifest_parser_test.cc
@@ -50,8 +50,8 @@
"\n"
"build result: cat in_1.cc in-2.O\n"));
- ASSERT_EQ(3u, state.bindings_.GetRules().size());
- const Rule* rule = state.bindings_.GetRules().begin()->second.get();
+ ASSERT_EQ(3u, state.bindings().GetRules().size());
+ const Rule* rule = state.bindings().GetRules().begin()->second.get();
EXPECT_EQ("cat", rule->name());
EXPECT_EQ("[cat ][$in][ > ][$out]",
rule->GetBinding("command")->Serialize());
@@ -83,8 +83,8 @@
"build result: cat in_1.cc in-2.O\n"
" #comment\n"));
- ASSERT_EQ(2u, state.bindings_.GetRules().size());
- const Rule* rule = state.bindings_.GetRules().begin()->second.get();
+ ASSERT_EQ(2u, state.bindings().GetRules().size());
+ const Rule* rule = state.bindings().GetRules().begin()->second.get();
EXPECT_EQ("cat", rule->name());
Edge* edge = state.GetNode("result", 0)->in_edge();
EXPECT_TRUE(edge->GetBindingBool("restat"));
@@ -103,7 +103,7 @@
"variable=1\n"));
// the variable must be in the top level environment
- EXPECT_EQ("1", state.bindings_.LookupVariable("variable"));
+ EXPECT_EQ("1", state.bindings().LookupVariable("variable"));
}
TEST_F(ParserTest, ResponseFiles) {
@@ -116,8 +116,8 @@
"build out: cat_rsp in\n"
" rspfile=out.rsp\n"));
- ASSERT_EQ(2u, state.bindings_.GetRules().size());
- const Rule* rule = state.bindings_.GetRules().begin()->second.get();
+ ASSERT_EQ(2u, state.bindings().GetRules().size());
+ const Rule* rule = state.bindings().GetRules().begin()->second.get();
EXPECT_EQ("cat_rsp", rule->name());
EXPECT_EQ("[cat ][$rspfile][ > ][$out]",
rule->GetBinding("command")->Serialize());
@@ -133,8 +133,8 @@
"build out: cat_rsp in in2\n"
" rspfile=out.rsp\n"));
- ASSERT_EQ(2u, state.bindings_.GetRules().size());
- const Rule* rule = state.bindings_.GetRules().begin()->second.get();
+ ASSERT_EQ(2u, state.bindings().GetRules().size());
+ const Rule* rule = state.bindings().GetRules().begin()->second.get();
EXPECT_EQ("cat_rsp", rule->name());
EXPECT_EQ("[cat ][$in_newline][ > ][$out]",
rule->GetBinding("command")->Serialize());
@@ -161,7 +161,7 @@
Edge* edge = state.edges_[0];
EXPECT_EQ("ld one-letter-test -pthread -under -o a b c",
edge->EvaluateCommand());
- EXPECT_EQ("1/2", state.bindings_.LookupVariable("nested2"));
+ EXPECT_EQ("1/2", state.bindings().LookupVariable("nested2"));
edge = state.edges_[1];
EXPECT_EQ("ld one-letter-test 1/2/3 -under -o supernested x",
@@ -194,8 +194,8 @@
"build a: link c $\n"
" d e f\n"));
- ASSERT_EQ(2u, state.bindings_.GetRules().size());
- const Rule* rule = state.bindings_.GetRules().begin()->second.get();
+ ASSERT_EQ(2u, state.bindings().GetRules().size());
+ const Rule* rule = state.bindings().GetRules().begin()->second.get();
EXPECT_EQ("link", rule->name());
EXPECT_EQ("[foo bar baz]", rule->GetBinding("command")->Serialize());
}
@@ -205,15 +205,15 @@
"foo = bar\\baz\n"
"foo2 = bar\\ baz\n"
));
- EXPECT_EQ("bar\\baz", state.bindings_.LookupVariable("foo"));
- EXPECT_EQ("bar\\ baz", state.bindings_.LookupVariable("foo2"));
+ EXPECT_EQ("bar\\baz", state.bindings().LookupVariable("foo"));
+ EXPECT_EQ("bar\\ baz", state.bindings().LookupVariable("foo2"));
}
TEST_F(ParserTest, Comment) {
ASSERT_NO_FATAL_FAILURE(AssertParse(
"# this is a comment\n"
"foo = not # a comment\n"));
- EXPECT_EQ("not # a comment", state.bindings_.LookupVariable("foo"));
+ EXPECT_EQ("not # a comment", state.bindings().LookupVariable("foo"));
}
TEST_F(ParserTest, Dollars) {
@@ -224,7 +224,7 @@
"x = $$dollar\n"
"build $x: foo y\n"
));
- EXPECT_EQ("$dollar", state.bindings_.LookupVariable("x"));
+ EXPECT_EQ("$dollar", state.bindings().LookupVariable("x"));
#ifdef _WIN32
EXPECT_EQ("$dollarbar$baz$blah", state.edges_[0]->EvaluateCommand());
#else
@@ -905,7 +905,7 @@
ASSERT_EQ(1u, fs_.files_read_.size());
EXPECT_EQ("include.ninja", fs_.files_read_[0]);
- EXPECT_EQ("inner", state.bindings_.LookupVariable("var"));
+ EXPECT_EQ("inner", state.bindings().LookupVariable("var"));
}
TEST_F(ParserTest, BrokenInclude) {
diff --git a/src/ninja.cc b/src/ninja.cc
index c08b20e..4739510 100644
--- a/src/ninja.cc
+++ b/src/ninja.cc
@@ -658,7 +658,7 @@
// Print rules
- for (const auto& pair : state_.bindings_.GetRules()) {
+ for (const auto& pair : state_.bindings().GetRules()) {
printf("%s", pair.first.c_str());
if (print_description) {
const Rule* rule = pair.second.get();
@@ -1321,7 +1321,7 @@
}
bool NinjaMain::EnsureBuildDirExists() {
- build_dir_ = state_.bindings_.LookupVariable("builddir");
+ build_dir_ = state_.bindings().LookupVariable("builddir");
if (!build_dir_.empty() && !config_.dry_run) {
if (!disk_interface_.MakeDirs(build_dir_ + "/.") && errno != EEXIST) {
Error("creating build directory %s: %s",
diff --git a/src/state.cc b/src/state.cc
index cc78d03..84d08a4 100644
--- a/src/state.cc
+++ b/src/state.cc
@@ -68,8 +68,14 @@
}
State::State() {
+ // Create top-level scope.
+ bindings_.emplace_back(new BindingEnv());
+
+ // Add the phony rule to it.
phony_rule_ = new Rule("phony");
- bindings_.AddRule(phony_rule_);
+ bindings_[0]->AddRule(phony_rule_);
+
+ // Create default and console pools.
default_pool_ = new Pool("", 0);
console_pool_ = new Pool("console", 1);
AddPool(default_pool_);
@@ -102,7 +108,7 @@
Edge* edge = new Edge();
edge->rule_ = rule;
edge->pool_ = default_pool_;
- edge->env_ = &bindings_;
+ edge->env_ = bindings_[0].get();
edge->id_ = edges_.size();
edges_.push_back(edge);
return edge;
@@ -204,6 +210,11 @@
return defaults_.empty() ? RootNodes(err) : defaults_;
}
+BindingEnv* State::CreateNewEnv(BindingEnv* parent) {
+ bindings_.emplace_back(new BindingEnv(parent));
+ return bindings_.back().get();
+}
+
void State::Reset() {
METRIC_RECORD("state reset");
for (auto& pair : pools_)
diff --git a/src/state.h b/src/state.h
index 958463b..7000637 100644
--- a/src/state.h
+++ b/src/state.h
@@ -154,6 +154,12 @@
std::vector<Node*> RootNodes(std::string* error) const;
std::vector<Node*> DefaultNodes(std::string* error) const;
+ /// Create new BindingEnv instance.
+ BindingEnv* CreateNewEnv(BindingEnv* parent);
+
+ /// Return top-level BindingEnv instance.
+ BindingEnv& bindings() const { return *bindings_[0]; }
+
/// Mapping of path -> Node.
/// NOTE: This owns all Node instances, but a DepsLog instance contains
/// pointers to them as well.
@@ -172,7 +178,12 @@
std::vector<Edge*> edges_;
Rule* phony_rule_ = nullptr;
- BindingEnv bindings_;
+
+ /// Note: BindingEnv need pointer stability (as they all include a parent
+ /// pointer)
+ std::vector<std::unique_ptr<BindingEnv>> bindings_;
+
+ /// Points to elements of paths_.
std::vector<Node*> defaults_;
};
diff --git a/src/state_test.cc b/src/state_test.cc
index e0e3060..7b6dd8f 100644
--- a/src/state_test.cc
+++ b/src/state_test.cc
@@ -31,7 +31,7 @@
Rule* rule = new Rule("cat");
rule->AddBinding("command", command);
- state.bindings_.AddRule(rule);
+ state.bindings().AddRule(rule);
Edge* edge = state.AddEdge(rule);
state.AddIn(edge, "in1", 0);
