From 4ed980b0937cf98e05ec645ce43b2a39780a3600 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 25 Nov 2024 12:26:04 +0100 Subject: [PATCH 1/5] build_log.*: Use member default initialization. --- src/build_log.cc | 3 +-- src/build_log.h | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index 073d2fe81e..4d6ee2823a 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -72,8 +72,7 @@ BuildLog::LogEntry::LogEntry(const string& output, uint64_t 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(); diff --git a/src/build_log.h b/src/build_log.h index 1223c2a16a..36f3846d3c 100644 --- a/src/build_log.h +++ b/src/build_log.h @@ -57,10 +57,10 @@ struct BuildLog { 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); @@ -99,9 +99,9 @@ struct BuildLog { 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_ From d7ceecdf5e96e35cac221212edeab3c1dcef7370 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 25 Nov 2024 12:29:59 +0100 Subject: [PATCH 2/5] build_log.cc: Remove `using namespace std` usage. --- src/build_log.cc | 38 +++++++++++++++++--------------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index 4d6ee2823a..befc975cc2 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,14 +61,12 @@ uint64_t BuildLog::LogEntry::HashCommand(StringPiece command) { return rapidhash(command.str_, command.len_); } -BuildLog::LogEntry::LogEntry(const string& output) - : output(output) {} +BuildLog::LogEntry::LogEntry(const std::string& output) : output(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() = default; @@ -78,8 +74,8 @@ 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; @@ -93,11 +89,11 @@ bool BuildLog::OpenForWrite(const string& path, const BuildLogUser& user, 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::iterator out = edge->outputs_.begin(); + for (std::vector::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()) { @@ -208,7 +204,7 @@ struct LineReader { 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) { @@ -282,7 +278,7 @@ LoadStatus BuildLog::Load(const string& path, string* err) { end = static_cast(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; @@ -326,7 +322,7 @@ LoadStatus BuildLog::Load(const string& path, string* err) { 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; @@ -339,12 +335,12 @@ bool BuildLog::WriteEntry(FILE* f, const LogEntry& entry) { 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); @@ -357,7 +353,7 @@ bool BuildLog::Recompact(const string& path, const BuildLogUser& user, return false; } - vector dead_outputs; + std::vector dead_outputs; for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) { if (user.IsPathDead(i->first)) { dead_outputs.push_back(i->first); From c1f74c69f3fd30a126a92d32713be51178da0a73 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 25 Nov 2024 12:45:52 +0100 Subject: [PATCH 3/5] build_log_test.cc: Remove `using namespace std` usage. --- src/build_log_test.cc | 35 +++++++++++++++++------------------ 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/build_log_test.cc b/src/build_log_test.cc index 630b1f1a92..2b0d6cda2f 100644 --- a/src/build_log_test.cc +++ b/src/build_log_test.cc @@ -27,8 +27,6 @@ #endif #include -using namespace std; - namespace { const char kTestFilename[] = "BuildLogTest-tempfile"; @@ -50,7 +48,7 @@ TEST_F(BuildLogTest, WriteRead) { "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 @@ TEST_F(BuildLogTest, FirstWriteAddsSignature) { 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 @@ TEST_F(BuildLogTest, DoubleEntry) { 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 @@ TEST_F(BuildLogTest, Truncate) { { 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 @@ TEST_F(BuildLogTest, Truncate) { // 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 @@ TEST_F(BuildLogTest, ObsoleteOldVersion) { 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 @@ TEST_F(BuildLogTest, SpacesInOutput) { 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 @@ TEST_F(BuildLogTest, DuplicateVersionHeader) { 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 @@ TEST_F(BuildLogTest, DuplicateVersionHeader) { } 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 @@ TEST_F(BuildLogTest, VeryLongInputLine) { 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 @@ TEST_F(BuildLogRecompactTest, Recompact) { "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 From 0837388594d11c45d71d31bc51d82bd88d541ea3 Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 25 Nov 2024 12:32:56 +0100 Subject: [PATCH 4/5] build_log.cc: Remove one string copy when loading entries from the log. --- src/build_log.cc | 4 ++-- src/build_log.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index befc975cc2..71cfd0f998 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -61,7 +61,7 @@ uint64_t BuildLog::LogEntry::HashCommand(StringPiece command) { return rapidhash(command.str_, command.len_); } -BuildLog::LogEntry::LogEntry(const std::string& output) : output(output) {} +BuildLog::LogEntry::LogEntry(std::string output) : output(std::move(output)) {} BuildLog::LogEntry::LogEntry(const std::string& output, uint64_t command_hash, int start_time, int end_time, TimeStamp mtime) @@ -288,7 +288,7 @@ LoadStatus BuildLog::Load(const std::string& path, std::string* err) { if (i != entries_.end()) { entry = i->second; } else { - entry = new LogEntry(output); + entry = new LogEntry(std::move(output)); entries_.insert(Entries::value_type(entry->output, entry)); ++unique_entry_count; } diff --git a/src/build_log.h b/src/build_log.h index 36f3846d3c..2a549e6b74 100644 --- a/src/build_log.h +++ b/src/build_log.h @@ -71,7 +71,7 @@ struct BuildLog { 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); }; From 3ecea33bb9a988022fc602c1e42ff069ab135ecf Mon Sep 17 00:00:00 2001 From: David 'Digit' Turner Date: Mon, 25 Nov 2024 12:23:26 +0100 Subject: [PATCH 5/5] build_log.cc: Remove memory leak in Recompact() method. The erase() call to remove `dead_outputs` was leaking the LogEntry value that the map pointed to. This patch changes the type of `Entries` to store `std::unique_ptr` values, instead of raw pointers to get rid of the problem, and adjust call sites accordingly. Also use Entries::emplace() instead of Entries::insert() to create the new value in-place when inserting new ones into the map. --- src/build_log.cc | 34 ++++++++++++++++++---------------- src/build_log.h | 6 ++++-- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/build_log.cc b/src/build_log.cc index 71cfd0f998..726949ad34 100644 --- a/src/build_log.cc +++ b/src/build_log.cc @@ -97,10 +97,11 @@ bool BuildLog::RecordCommand(Edge* edge, int start_time, int end_time, 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(log_entry)); } log_entry->command_hash = command_hash; log_entry->start_time = start_time; @@ -286,10 +287,11 @@ LoadStatus BuildLog::Load(const std::string& path, std::string* err) { LogEntry* entry; Entries::iterator i = entries_.find(output); if (i != entries_.end()) { - entry = i->second; + entry = i->second.get(); } else { entry = new LogEntry(std::move(output)); - entries_.insert(Entries::value_type(entry->output, entry)); + // Passes ownership of |entry| to the map, but keeps the pointer valid. + entries_.emplace(entry->output, std::unique_ptr(entry)); ++unique_entry_count; } ++total_entry_count; @@ -325,7 +327,7 @@ LoadStatus BuildLog::Load(const std::string& path, std::string* err) { 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; } @@ -354,21 +356,21 @@ bool BuildLog::Recompact(const std::string& path, const BuildLogUser& user, } std::vector dead_outputs; - for (Entries::iterator i = entries_.begin(); i != entries_.end(); ++i) { - if (user.IsPathDead(i->first)) { - dead_outputs.push_back(i->first); + 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) { @@ -403,24 +405,24 @@ bool BuildLog::Restat(const StringPiece path, 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 2a549e6b74..61bd8ffbf2 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 #include +#include +#include + #include "hash_map.h" #include "load_status.h" #include "timestamp.h" @@ -90,7 +92,7 @@ struct BuildLog { bool Restat(StringPiece path, const DiskInterface& disk_interface, int output_count, char** outputs, std::string* err); - typedef ExternalStringHashMap::Type Entries; + typedef ExternalStringHashMap>::Type Entries; const Entries& entries() const { return entries_; } private: