From c47506449cd4f6e44a7d43d9b7ecb33f3b448c16 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Mon, 10 Nov 2014 12:00:16 -0800 Subject: make all GetNode explicit, add DepsLog canonicalize test --- src/build.cc | 6 ++- src/build_test.cc | 68 +++++++++++++++++++++++++++++++- src/deps_log.cc | 10 ++++- src/deps_log_test.cc | 94 ++++++++++++++++++++++----------------------- src/graph.cc | 2 +- src/includes_normalize.h | 3 +- src/manifest_parser_test.cc | 2 +- src/state.cc | 7 ---- src/state.h | 1 - src/state_test.cc | 6 +-- src/test.cc | 3 +- 11 files changed, 135 insertions(+), 67 deletions(-) diff --git a/src/build.cc b/src/build.cc index a346028..3e74131 100644 --- a/src/build.cc +++ b/src/build.cc @@ -825,7 +825,11 @@ bool Builder::ExtractDeps(CommandRunner::Result* result, result->output = parser.Parse(result->output, deps_prefix); for (set::iterator i = parser.includes_.begin(); i != parser.includes_.end(); ++i) { - deps_nodes->push_back(state_->GetNode(*i)); + // ~0 is assuming that with MSVC-parsed headers, it's ok to always make + // all backslashes (as some of the slashes will certainly be backslashes + // anyway). This could be fixed if necessary with some additional + // complexity in IncludesNormalize::Relativize. + deps_nodes->push_back(state_->GetNode(*i, ~0u)); } } else #endif diff --git a/src/build_test.cc b/src/build_test.cc index 912e3ef..bd1cd30 100644 --- a/src/build_test.cc +++ b/src/build_test.cc @@ -1883,7 +1883,7 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) { Edge* edge = state.edges_.back(); - state.GetNode("bar.h")->MarkDirty(); // Mark bar.h as missing. + state.GetNode("bar.h", 0)->MarkDirty(); // Mark bar.h as missing. EXPECT_TRUE(builder.AddTarget("fo o.o", &err)); ASSERT_EQ("", err); @@ -1901,6 +1901,72 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) { } } +#ifdef _WIN32 +TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) { + string err; + const char* manifest = + "rule cc\n command = cc $in\n depfile = $out.d\n deps = gcc\n" + "build a/b\\c\\d/e/fo$ o.o: cc x\\y/z\\foo.c\n"; + + fs_.Create("x/y/z/foo.c", ""); + + { + State state; + ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest)); + + // Run the build once, everything should be ok. + DepsLog deps_log; + ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err)); + ASSERT_EQ("", err); + + Builder builder(&state, config_, NULL, &deps_log, &fs_); + builder.command_runner_.reset(&command_runner_); + EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err)); + ASSERT_EQ("", err); + // Note, different slashes from manifest. + fs_.Create("a/b\\c\\d/e/fo o.o.d", + "a\\b\\c\\d\\e\\fo\\ o.o: blah.h bar.h\n"); + EXPECT_TRUE(builder.Build(&err)); + EXPECT_EQ("", err); + + deps_log.Close(); + builder.command_runner_.release(); + } + + { + State state; + ASSERT_NO_FATAL_FAILURE(AssertParse(&state, manifest)); + + DepsLog deps_log; + ASSERT_TRUE(deps_log.Load("ninja_deps", &state, &err)); + ASSERT_TRUE(deps_log.OpenForWrite("ninja_deps", &err)); + ASSERT_EQ("", err); + + Builder builder(&state, config_, NULL, &deps_log, &fs_); + builder.command_runner_.reset(&command_runner_); + + Edge* edge = state.edges_.back(); + + state.GetNode("bar.h", 0)->MarkDirty(); // Mark bar.h as missing. + EXPECT_TRUE(builder.AddTarget("a/b/c/d/e/fo o.o", &err)); + ASSERT_EQ("", err); + + // Expect three new edges: one generating fo o.o, and two more from + // loading the depfile. + ASSERT_EQ(3u, state.edges_.size()); + // Expect our edge to now have three inputs: foo.c and two headers. + ASSERT_EQ(3u, edge->inputs_.size()); + + // Expect the command line we generate to only use the original input. + // Note, slashes from manifest, not .d. + ASSERT_EQ("cc x\\y/z\\foo.c", edge->EvaluateCommand()); + + deps_log.Close(); + builder.command_runner_.release(); + } +} +#endif + /// Check that a restat rule doesn't clear an edge if the depfile is missing. /// Follows from: https://github.com/martine/ninja/issues/603 TEST_F(BuildTest, RestatMissingDepfile) { diff --git a/src/deps_log.cc b/src/deps_log.cc index 39de180..d889dfd 100644 --- a/src/deps_log.cc +++ b/src/deps_log.cc @@ -233,14 +233,20 @@ bool DepsLog::Load(const string& path, State* state, string* err) { if (!UpdateDeps(out_id, deps)) ++unique_dep_record_count; } else { - int path_size = size - 4; + size_t path_size = size - 4; assert(path_size > 0); // CanonicalizePath() rejects empty paths. // There can be up to 3 bytes of padding. if (buf[path_size - 1] == '\0') --path_size; if (buf[path_size - 1] == '\0') --path_size; if (buf[path_size - 1] == '\0') --path_size; + unsigned int slash_bits; + string err; + if (!CanonicalizePath(&buf[0], &path_size, &slash_bits, &err)) { + read_failed = true; + break; + } StringPiece path(buf, path_size); - Node* node = state->GetNode(path); + Node* node = state->GetNode(path, slash_bits); // Check that the expected index matches the actual index. This can only // happen if two ninja processes write to the same deps log concurrently. diff --git a/src/deps_log_test.cc b/src/deps_log_test.cc index 60ff48f..ac2b315 100644 --- a/src/deps_log_test.cc +++ b/src/deps_log_test.cc @@ -46,16 +46,16 @@ TEST_F(DepsLogTest, WriteRead) { { vector deps; - deps.push_back(state1.GetNode("foo.h")); - deps.push_back(state1.GetNode("bar.h")); - log1.RecordDeps(state1.GetNode("out.o"), 1, deps); + deps.push_back(state1.GetNode("foo.h", 0)); + deps.push_back(state1.GetNode("bar.h", 0)); + log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps); deps.clear(); - deps.push_back(state1.GetNode("foo.h")); - deps.push_back(state1.GetNode("bar2.h")); - log1.RecordDeps(state1.GetNode("out2.o"), 2, deps); + deps.push_back(state1.GetNode("foo.h", 0)); + deps.push_back(state1.GetNode("bar2.h", 0)); + log1.RecordDeps(state1.GetNode("out2.o", 0), 2, deps); - DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o")); + DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0)); ASSERT_TRUE(log_deps); ASSERT_EQ(1, log_deps->mtime); ASSERT_EQ(2, log_deps->node_count); @@ -79,7 +79,7 @@ TEST_F(DepsLogTest, WriteRead) { } // Spot-check the entries in log2. - DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o")); + DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out2.o", 0)); ASSERT_TRUE(log_deps); ASSERT_EQ(2, log_deps->mtime); ASSERT_EQ(2, log_deps->node_count); @@ -101,11 +101,11 @@ TEST_F(DepsLogTest, LotsOfDeps) { for (int i = 0; i < kNumDeps; ++i) { char buf[32]; sprintf(buf, "file%d.h", i); - deps.push_back(state1.GetNode(buf)); + deps.push_back(state1.GetNode(buf, 0)); } - log1.RecordDeps(state1.GetNode("out.o"), 1, deps); + log1.RecordDeps(state1.GetNode("out.o", 0), 1, deps); - DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o")); + DepsLog::Deps* log_deps = log1.GetDeps(state1.GetNode("out.o", 0)); ASSERT_EQ(kNumDeps, log_deps->node_count); } @@ -116,7 +116,7 @@ TEST_F(DepsLogTest, LotsOfDeps) { EXPECT_TRUE(log2.Load(kTestFilename, &state2, &err)); ASSERT_EQ("", err); - DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o")); + DepsLog::Deps* log_deps = log2.GetDeps(state2.GetNode("out.o", 0)); ASSERT_EQ(kNumDeps, log_deps->node_count); } @@ -132,9 +132,9 @@ TEST_F(DepsLogTest, DoubleEntry) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); log.Close(); struct stat st; @@ -154,9 +154,9 @@ TEST_F(DepsLogTest, DoubleEntry) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); log.Close(); struct stat st; @@ -186,14 +186,14 @@ TEST_F(DepsLogTest, Recompact) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); deps.clear(); - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("baz.h")); - log.RecordDeps(state.GetNode("other_out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("baz.h", 0)); + log.RecordDeps(state.GetNode("other_out.o", 0), 1, deps); log.Close(); @@ -216,8 +216,8 @@ TEST_F(DepsLogTest, Recompact) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); log.Close(); struct stat st; @@ -237,14 +237,14 @@ TEST_F(DepsLogTest, Recompact) { string err; ASSERT_TRUE(log.Load(kTestFilename, &state, &err)); - Node* out = state.GetNode("out.o"); + Node* out = state.GetNode("out.o", 0); DepsLog::Deps* deps = log.GetDeps(out); ASSERT_TRUE(deps); ASSERT_EQ(1, deps->mtime); ASSERT_EQ(1, deps->node_count); ASSERT_EQ("foo.h", deps->nodes[0]->path()); - Node* other_out = state.GetNode("other_out.o"); + Node* other_out = state.GetNode("other_out.o", 0); deps = log.GetDeps(other_out); ASSERT_TRUE(deps); ASSERT_EQ(1, deps->mtime); @@ -287,14 +287,14 @@ TEST_F(DepsLogTest, Recompact) { string err; ASSERT_TRUE(log.Load(kTestFilename, &state, &err)); - Node* out = state.GetNode("out.o"); + Node* out = state.GetNode("out.o", 0); DepsLog::Deps* deps = log.GetDeps(out); ASSERT_TRUE(deps); ASSERT_EQ(1, deps->mtime); ASSERT_EQ(1, deps->node_count); ASSERT_EQ("foo.h", deps->nodes[0]->path()); - Node* other_out = state.GetNode("other_out.o"); + Node* other_out = state.GetNode("other_out.o", 0); deps = log.GetDeps(other_out); ASSERT_TRUE(deps); ASSERT_EQ(1, deps->mtime); @@ -361,14 +361,14 @@ TEST_F(DepsLogTest, Truncated) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); deps.clear(); - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar2.h")); - log.RecordDeps(state.GetNode("out2.o"), 2, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar2.h", 0)); + log.RecordDeps(state.GetNode("out2.o", 0), 2, deps); log.Close(); } @@ -420,14 +420,14 @@ TEST_F(DepsLogTest, TruncatedRecovery) { ASSERT_EQ("", err); vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar.h")); - log.RecordDeps(state.GetNode("out.o"), 1, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar.h", 0)); + log.RecordDeps(state.GetNode("out.o", 0), 1, deps); deps.clear(); - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar2.h")); - log.RecordDeps(state.GetNode("out2.o"), 2, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar2.h", 0)); + log.RecordDeps(state.GetNode("out2.o", 0), 2, deps); log.Close(); } @@ -448,16 +448,16 @@ TEST_F(DepsLogTest, TruncatedRecovery) { err.clear(); // The truncated entry should've been discarded. - EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o"))); + EXPECT_EQ(NULL, log.GetDeps(state.GetNode("out2.o", 0))); EXPECT_TRUE(log.OpenForWrite(kTestFilename, &err)); ASSERT_EQ("", err); // Add a new entry. vector deps; - deps.push_back(state.GetNode("foo.h")); - deps.push_back(state.GetNode("bar2.h")); - log.RecordDeps(state.GetNode("out2.o"), 3, deps); + deps.push_back(state.GetNode("foo.h", 0)); + deps.push_back(state.GetNode("bar2.h", 0)); + log.RecordDeps(state.GetNode("out2.o", 0), 3, deps); log.Close(); } @@ -471,7 +471,7 @@ TEST_F(DepsLogTest, TruncatedRecovery) { EXPECT_TRUE(log.Load(kTestFilename, &state, &err)); // The truncated entry should exist. - DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o")); + DepsLog::Deps* deps = log.GetDeps(state.GetNode("out2.o", 0)); ASSERT_TRUE(deps); } } diff --git a/src/graph.cc b/src/graph.cc index f85a1f6..930c626 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -331,7 +331,7 @@ bool Edge::use_console() const { string Node::PathDecanonicalized() const { string result = path_; unsigned int mask = 1; - for (char* c = &result[0]; (c = strpbrk(c, "/\\")) != NULL;) { + for (char* c = &result[0]; (c = strchr(c, '/')) != NULL;) { if (slash_bits_ & mask) *c = '\\'; c++; diff --git a/src/includes_normalize.h b/src/includes_normalize.h index 43527af..634fef3 100644 --- a/src/includes_normalize.h +++ b/src/includes_normalize.h @@ -29,7 +29,6 @@ struct IncludesNormalize { static string Relativize(StringPiece path, const string& start); /// Normalize by fixing slashes style, fixing redundant .. and . and makes the - /// path relative to |relative_to|. Case is normalized to lowercase on - /// Windows too. + /// path relative to |relative_to|. static string Normalize(const string& input, const char* relative_to); }; diff --git a/src/manifest_parser_test.cc b/src/manifest_parser_test.cc index 0668668..6909ea9 100644 --- a/src/manifest_parser_test.cc +++ b/src/manifest_parser_test.cc @@ -96,7 +96,7 @@ TEST_F(ParserTest, IgnoreIndentedComments) { ASSERT_EQ(2u, state.rules_.size()); const Rule* rule = state.rules_.begin()->second; EXPECT_EQ("cat", rule->name()); - Edge* edge = state.GetNode("result")->in_edge(); + Edge* edge = state.GetNode("result", 0)->in_edge(); EXPECT_TRUE(edge->GetBindingBool("restat")); EXPECT_FALSE(edge->GetBindingBool("generator")); } diff --git a/src/state.cc b/src/state.cc index 7d1d79d..6e3e10d 100644 --- a/src/state.cc +++ b/src/state.cc @@ -111,13 +111,6 @@ Edge* State::AddEdge(const Rule* rule) { return edge; } -Node* State::GetNode(StringPiece path) { -#if defined(_WIN32) && !defined(NDEBUG) - assert(strpbrk(path.AsString().c_str(), "/\\") == NULL); -#endif - return GetNode(path, 0); -} - Node* State::GetNode(StringPiece path, unsigned int slash_bits) { Node* node = LookupNode(path); if (node) diff --git a/src/state.h b/src/state.h index 9da6d1b..8804532 100644 --- a/src/state.h +++ b/src/state.h @@ -95,7 +95,6 @@ struct State { Edge* AddEdge(const Rule* rule); - Node* GetNode(StringPiece path); Node* GetNode(StringPiece path, unsigned int slash_bits); Node* LookupNode(StringPiece path) const; Node* SpellcheckNode(const string& path); diff --git a/src/state_test.cc b/src/state_test.cc index 85c6513..bd133b6 100644 --- a/src/state_test.cc +++ b/src/state_test.cc @@ -38,9 +38,9 @@ TEST(State, Basic) { EXPECT_EQ("cat in1 in2 > out", edge->EvaluateCommand()); - EXPECT_FALSE(state.GetNode("in1")->dirty()); - EXPECT_FALSE(state.GetNode("in2")->dirty()); - EXPECT_FALSE(state.GetNode("out")->dirty()); + EXPECT_FALSE(state.GetNode("in1", 0)->dirty()); + EXPECT_FALSE(state.GetNode("in2", 0)->dirty()); + EXPECT_FALSE(state.GetNode("out", 0)->dirty()); } } // namespace diff --git a/src/test.cc b/src/test.cc index 560ef3a..f667fef 100644 --- a/src/test.cc +++ b/src/test.cc @@ -89,7 +89,8 @@ void StateTestWithBuiltinRules::AddCatRule(State* state) { } Node* StateTestWithBuiltinRules::GetNode(const string& path) { - return state_.GetNode(path); + EXPECT_FALSE(strpbrk(path.c_str(), "/\\")); + return state_.GetNode(path, 0); } void AssertParse(State* state, const char* input) { -- cgit v0.12