Merge pull request #2534 from digit-google/fix-build-log-leak
Fix build log leak
diff --git a/src/build_log.cc b/src/build_log.cc
index 073d2fe..726949a 100644
--- a/src/build_log.cc
+++ b/src/build_log.cc
@@ -41,8 +41,6 @@
#define strtoll _strtoi64
#endif
-using namespace std;
-
// Implementation details:
// Each run's log appends to the log file.
// To load, we run through all log entries in series, throwing away
@@ -63,24 +61,21 @@
return rapidhash(command.str_, command.len_);
}
-BuildLog::LogEntry::LogEntry(const string& output)
- : output(output) {}
+BuildLog::LogEntry::LogEntry(std::string output) : output(std::move(output)) {}
-BuildLog::LogEntry::LogEntry(const string& output, uint64_t command_hash,
- int start_time, int end_time, TimeStamp mtime)
- : output(output), command_hash(command_hash),
- start_time(start_time), end_time(end_time), mtime(mtime)
-{}
+BuildLog::LogEntry::LogEntry(const std::string& output, uint64_t command_hash,
+ int start_time, int end_time, TimeStamp mtime)
+ : output(output), command_hash(command_hash), start_time(start_time),
+ end_time(end_time), mtime(mtime) {}
-BuildLog::BuildLog()
- : log_file_(NULL), needs_recompaction_(false) {}
+BuildLog::BuildLog() = default;
BuildLog::~BuildLog() {
Close();
}
-bool BuildLog::OpenForWrite(const string& path, const BuildLogUser& user,
- string* err) {
+bool BuildLog::OpenForWrite(const std::string& path, const BuildLogUser& user,
+ std::string* err) {
if (needs_recompaction_) {
if (!Recompact(path, user, err))
return false;
@@ -94,18 +89,19 @@
bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time,
TimeStamp mtime) {
- string command = edge->EvaluateCommand(true);
+ std::string command = edge->EvaluateCommand(true);
uint64_t command_hash = LogEntry::HashCommand(command);
- for (vector<Node*>::iterator out = edge->outputs_.begin();
+ for (std::vector<Node*>::iterator out = edge->outputs_.begin();
out != edge->outputs_.end(); ++out) {
- const string& path = (*out)->path();
+ const std::string& path = (*out)->path();
Entries::iterator i = entries_.find(path);
LogEntry* log_entry;
if (i != entries_.end()) {
- log_entry = i->second;
+ log_entry = i->second.get();
} else {
log_entry = new LogEntry(path);
- entries_.insert(Entries::value_type(log_entry->output, log_entry));
+ // Passes ownership of |log_entry| to the map, but keeps the pointer valid.
+ entries_.emplace(log_entry->output, std::unique_ptr<LogEntry>(log_entry));
}
log_entry->command_hash = command_hash;
log_entry->start_time = start_time;
@@ -209,7 +205,7 @@
char* line_end_;
};
-LoadStatus BuildLog::Load(const string& path, string* err) {
+LoadStatus BuildLog::Load(const std::string& path, std::string* err) {
METRIC_RECORD(".ninja_log load");
FILE* file = fopen(path.c_str(), "r");
if (!file) {
@@ -283,7 +279,7 @@
end = static_cast<char*>(memchr(start, kFieldSeparator, line_end - start));
if (!end)
continue;
- string output = string(start, end - start);
+ std::string output(start, end - start);
start = end + 1;
end = line_end;
@@ -291,10 +287,11 @@
LogEntry* entry;
Entries::iterator i = entries_.find(output);
if (i != entries_.end()) {
- entry = i->second;
+ entry = i->second.get();
} else {
- entry = new LogEntry(output);
- entries_.insert(Entries::value_type(entry->output, entry));
+ entry = new LogEntry(std::move(output));
+ // Passes ownership of |entry| to the map, but keeps the pointer valid.
+ entries_.emplace(entry->output, std::unique_ptr<LogEntry>(entry));
++unique_entry_count;
}
++total_entry_count;
@@ -327,10 +324,10 @@
return LOAD_SUCCESS;
}
-BuildLog::LogEntry* BuildLog::LookupByOutput(const string& path) {
+BuildLog::LogEntry* BuildLog::LookupByOutput(const std::string& path) {
Entries::iterator i = entries_.find(path);
if (i != entries_.end())
- return i->second;
+ return i->second.get();
return NULL;
}
@@ -340,12 +337,12 @@
entry.output.c_str(), entry.command_hash) > 0;
}
-bool BuildLog::Recompact(const string& path, const BuildLogUser& user,
- string* err) {
+bool BuildLog::Recompact(const std::string& path, const BuildLogUser& user,
+ std::string* err) {
METRIC_RECORD(".ninja_log recompact");
Close();
- string temp_path = path + ".recompact";
+ std::string temp_path = path + ".recompact";
FILE* f = fopen(temp_path.c_str(), "wb");
if (!f) {
*err = strerror(errno);
@@ -358,22 +355,22 @@
return false;
}
- vector<StringPiece> dead_outputs;
- for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
- if (user.IsPathDead(i->first)) {
- dead_outputs.push_back(i->first);
+ std::vector<StringPiece> dead_outputs;
+ for (const auto& pair : entries_) {
+ if (user.IsPathDead(pair.first)) {
+ dead_outputs.push_back(pair.first);
continue;
}
- if (!WriteEntry(f, *i->second)) {
+ if (!WriteEntry(f, *pair.second)) {
*err = strerror(errno);
fclose(f);
return false;
}
}
- for (size_t i = 0; i < dead_outputs.size(); ++i)
- entries_.erase(dead_outputs[i]);
+ for (StringPiece output : dead_outputs)
+ entries_.erase(output);
fclose(f);
if (unlink(path.c_str()) < 0) {
@@ -408,24 +405,24 @@
fclose(f);
return false;
}
- for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) {
+ for (auto& pair : entries_) {
bool skip = output_count > 0;
for (int j = 0; j < output_count; ++j) {
- if (i->second->output == outputs[j]) {
+ if (pair.second->output == outputs[j]) {
skip = false;
break;
}
}
if (!skip) {
- const TimeStamp mtime = disk_interface.Stat(i->second->output, err);
+ const TimeStamp mtime = disk_interface.Stat(pair.second->output, err);
if (mtime == -1) {
fclose(f);
return false;
}
- i->second->mtime = mtime;
+ pair.second->mtime = mtime;
}
- if (!WriteEntry(f, *i->second)) {
+ if (!WriteEntry(f, *pair.second)) {
*err = strerror(errno);
fclose(f);
return false;
diff --git a/src/build_log.h b/src/build_log.h
index 1223c2a..61bd8ff 100644
--- a/src/build_log.h
+++ b/src/build_log.h
@@ -15,9 +15,11 @@
#ifndef NINJA_BUILD_LOG_H_
#define NINJA_BUILD_LOG_H_
-#include <string>
#include <stdio.h>
+#include <memory>
+#include <string>
+
#include "hash_map.h"
#include "load_status.h"
#include "timestamp.h"
@@ -57,10 +59,10 @@
struct LogEntry {
std::string output;
- uint64_t command_hash;
- int start_time;
- int end_time;
- TimeStamp mtime;
+ uint64_t command_hash = 0;
+ int start_time = 0;
+ int end_time = 0;
+ TimeStamp mtime = 0;
static uint64_t HashCommand(StringPiece command);
@@ -71,7 +73,7 @@
mtime == o.mtime;
}
- explicit LogEntry(const std::string& output);
+ explicit LogEntry(std::string output);
LogEntry(const std::string& output, uint64_t command_hash,
int start_time, int end_time, TimeStamp mtime);
};
@@ -90,7 +92,7 @@
bool Restat(StringPiece path, const DiskInterface& disk_interface,
int output_count, char** outputs, std::string* err);
- typedef ExternalStringHashMap<LogEntry*>::Type Entries;
+ typedef ExternalStringHashMap<std::unique_ptr<LogEntry>>::Type Entries;
const Entries& entries() const { return entries_; }
private:
@@ -99,9 +101,9 @@
bool OpenForWriteIfNeeded();
Entries entries_;
- FILE* log_file_;
+ FILE* log_file_ = nullptr;
std::string log_file_path_;
- bool needs_recompaction_;
+ bool needs_recompaction_ = false;
};
#endif // NINJA_BUILD_LOG_H_
diff --git a/src/build_log_test.cc b/src/build_log_test.cc
index 630b1f1..2b0d6cd 100644
--- a/src/build_log_test.cc
+++ b/src/build_log_test.cc
@@ -27,8 +27,6 @@
#endif
#include <cassert>
-using namespace std;
-
namespace {
const char kTestFilename[] = "BuildLogTest-tempfile";
@@ -50,7 +48,7 @@
"build mid: cat in\n");
BuildLog log1;
- string err;
+ std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log1.RecordCommand(state_.edges_[0], 15, 18);
@@ -77,7 +75,7 @@
const size_t kVersionPos = strlen(kExpectedVersion) - 2; // Points at 'X'.
BuildLog log;
- string contents, err;
+ std::string contents, err;
EXPECT_TRUE(log.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
@@ -111,7 +109,7 @@
BuildLog::LogEntry::HashCommand("command def"));
fclose(f);
- string err;
+ std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
@@ -128,7 +126,7 @@
{
BuildLog log1;
- string err;
+ std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log1.RecordCommand(state_.edges_[0], 15, 18);
@@ -148,7 +146,7 @@
// crash when parsing.
for (off_t size = statbuf.st_size; size > 0; --size) {
BuildLog log2;
- string err;
+ std::string err;
EXPECT_TRUE(log2.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
log2.RecordCommand(state_.edges_[0], 15, 18);
@@ -169,10 +167,10 @@
fprintf(f, "123 456 0 out command\n");
fclose(f);
- string err;
+ std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
- ASSERT_NE(err.find("version"), string::npos);
+ ASSERT_NE(err.find("version"), std::string::npos);
}
TEST_F(BuildLogTest, SpacesInOutput) {
@@ -182,7 +180,7 @@
BuildLog::LogEntry::HashCommand("command"));
fclose(f);
- string err;
+ std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
@@ -208,7 +206,7 @@
BuildLog::LogEntry::HashCommand("command2"));
fclose(f);
- string err;
+ std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
@@ -229,22 +227,23 @@
}
struct TestDiskInterface : public DiskInterface {
- virtual TimeStamp Stat(const string& path, string* err) const {
+ virtual TimeStamp Stat(const std::string& path, std::string* err) const {
return 4;
}
- virtual bool WriteFile(const string& path, const string& contents) {
+ virtual bool WriteFile(const std::string& path, const std::string& contents) {
assert(false);
return true;
}
- virtual bool MakeDir(const string& path) {
+ virtual bool MakeDir(const std::string& path) {
assert(false);
return false;
}
- virtual Status ReadFile(const string& path, string* contents, string* err) {
+ virtual Status ReadFile(const std::string& path, std::string* contents,
+ std::string* err) {
assert(false);
return NotFound;
}
- virtual int RemoveFile(const string& path) {
+ virtual int RemoveFile(const std::string& path) {
assert(false);
return 0;
}
@@ -289,7 +288,7 @@
BuildLog::LogEntry::HashCommand("command2"));
fclose(f);
- string err;
+ std::string err;
BuildLog log;
EXPECT_TRUE(log.Load(kTestFilename, &err));
ASSERT_EQ("", err);
@@ -335,7 +334,7 @@
"build out2: cat in\n");
BuildLog log1;
- string err;
+ std::string err;
EXPECT_TRUE(log1.OpenForWrite(kTestFilename, *this, &err));
ASSERT_EQ("", err);
// Record the same edge several times, to trigger recompaction