Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix build log leak #2534

Merged
merged 5 commits into from
Dec 10, 2024
Merged

Fix build log leak #2534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ed980b
build_log.*: Use member default initialization.
digit-google Nov 25, 2024
d7ceecd
build_log.cc: Remove `using namespace std` usage.
digit-google Nov 25, 2024
c1f74c6
build_log_test.cc: Remove `using namespace std` usage.
digit-google Nov 25, 2024
0837388
build_log.cc: Remove one string copy when loading entries from the log.
digit-google Nov 25, 2024
3ecea33
build_log.cc: Remove memory leak in Recompact() method.
digit-google Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 37 additions & 40 deletions src/build_log.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -63,24 +61,21 @@ uint64_t BuildLog::LogEntry::HashCommand(StringPiece command) {
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;
Expand All @@ -94,18 +89,19 @@ 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<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;
Expand Down Expand Up @@ -209,7 +205,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) {
Expand Down Expand Up @@ -283,18 +279,19 @@ LoadStatus BuildLog::Load(const string& path, string* err) {
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;

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;
Expand Down Expand Up @@ -327,10 +324,10 @@ 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;
return i->second.get();
return NULL;
}

Expand All @@ -340,12 +337,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);
Expand All @@ -358,22 +355,22 @@ bool BuildLog::Recompact(const string& path, const BuildLogUser& user,
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) {
Expand Down Expand Up @@ -408,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;
Expand Down
20 changes: 11 additions & 9 deletions src/build_log.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -57,10 +59,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);

Expand All @@ -71,7 +73,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);
};
Expand All @@ -90,7 +92,7 @@ struct BuildLog {
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:
Expand All @@ -99,9 +101,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_
35 changes: 17 additions & 18 deletions src/build_log_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@
#endif
#include <cassert>

using namespace std;

namespace {

const char kTestFilename[] = "BuildLogTest-tempfile";
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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) {
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
Loading