From 8a9edb110354d5468ab42685cfece6a073146f27 Mon Sep 17 00:00:00 2001 From: Brad King Date: Mon, 8 Oct 2018 09:14:58 -0400 Subject: Restore depfile toleration of multiple output paths on distinct lines Prior to introduction of depfile parser handling of multiple rules, ninja silently accepted a depfile of the form: out: in1 in2 in3 other: otherIn1 otherIn2 otherIn3 and incorrectly treated `other` and `otherIn*` as additional inputs to `out`. Now we prefer to reject this just as we already do for a depfile specifying multiple outputs on one line. However, this can break existing cases where such a depfile was silently tolerated. Add a `-w depfilemulti={err,warn}` option to control this behavior, and make it just a warning by default. --- src/build.cc | 5 +++-- src/build.h | 2 ++ src/depfile_parser.cc | 30 ++++++++++++++++++++++++++++-- src/depfile_parser.h | 17 +++++++++++++++++ src/depfile_parser.in.cc | 30 ++++++++++++++++++++++++++++-- src/depfile_parser_test.cc | 13 ++++++++++--- src/disk_interface_test.cc | 2 +- src/graph.cc | 4 +++- src/graph.h | 13 +++++++++---- src/graph_test.cc | 2 +- src/ninja.cc | 19 ++++++++++++++++++- 11 files changed, 120 insertions(+), 17 deletions(-) diff --git a/src/build.cc b/src/build.cc index ed219fd..b392803 100644 --- a/src/build.cc +++ b/src/build.cc @@ -557,7 +557,8 @@ Builder::Builder(State* state, const BuildConfig& config, BuildLog* build_log, DepsLog* deps_log, DiskInterface* disk_interface) : state_(state), config_(config), disk_interface_(disk_interface), - scan_(state, build_log, deps_log, disk_interface) { + scan_(state, build_log, deps_log, disk_interface, + &config_.depfile_parser_options) { status_ = new BuildStatus(config); } @@ -906,7 +907,7 @@ bool Builder::ExtractDeps(CommandRunner::Result* result, if (content.empty()) return true; - DepfileParser deps; + DepfileParser deps(config_.depfile_parser_options); if (!deps.Parse(&content, err)) return false; diff --git a/src/build.h b/src/build.h index 9b90e8a..a42b8d4 100644 --- a/src/build.h +++ b/src/build.h @@ -23,6 +23,7 @@ #include #include +#include "depfile_parser.h" #include "graph.h" // XXX needed for DependencyScan; should rearrange. #include "exit_status.h" #include "line_printer.h" @@ -151,6 +152,7 @@ struct BuildConfig { /// The maximum load average we must not exceed. A negative value /// means that we do not have any limit. double max_load_average; + DepfileParserOptions depfile_parser_options; }; /// Builder wraps the build process: starting commands, updating status. diff --git a/src/depfile_parser.cc b/src/depfile_parser.cc index 7724eed..405289f 100644 --- a/src/depfile_parser.cc +++ b/src/depfile_parser.cc @@ -14,6 +14,12 @@ // limitations under the License. #include "depfile_parser.h" +#include "util.h" + +DepfileParser::DepfileParser(DepfileParserOptions options) + : options_(options) +{ +} // A note on backslashes in Makefiles, from reading the docs: // Backslash-newline is the line continuation character. @@ -37,6 +43,8 @@ bool DepfileParser::Parse(string* content, string* err) { char* end = in + content->size(); bool have_target = false; bool have_secondary_target_on_this_rule = false; + bool have_newline_since_primary_target = false; + bool warned_distinct_target_lines = false; bool parsing_targets = true; while (in < end) { bool have_newline = false; @@ -219,8 +227,23 @@ yy22: if (len > 0) { if (is_dependency) { if (have_secondary_target_on_this_rule) { - *err = "depfile has multiple output paths"; - return false; + if (!have_newline_since_primary_target) { + *err = "depfile has multiple output paths"; + return false; + } else if (options_.depfile_distinct_target_lines_action_ == + kDepfileDistinctTargetLinesActionError) { + *err = + "depfile has multiple output paths (on separate lines)" + " [-w depfilemulti=err]"; + return false; + } else { + if (!warned_distinct_target_lines) { + warned_distinct_target_lines = true; + Warning("depfile has multiple output paths (on separate lines); " + "continuing anyway [-w depfilemulti=warn]"); + } + continue; + } } ins_.push_back(StringPiece(filename, len)); } else if (!out_.str_) { @@ -234,6 +257,9 @@ yy22: // A newline ends a rule so the next filename will be a new target. parsing_targets = true; have_secondary_target_on_this_rule = false; + if (have_target) { + have_newline_since_primary_target = true; + } } } if (!have_target) { diff --git a/src/depfile_parser.h b/src/depfile_parser.h index 1e6ebb5..be20374 100644 --- a/src/depfile_parser.h +++ b/src/depfile_parser.h @@ -21,8 +21,24 @@ using namespace std; #include "string_piece.h" +enum DepfileDistinctTargetLinesAction { + kDepfileDistinctTargetLinesActionWarn, + kDepfileDistinctTargetLinesActionError, +}; + +struct DepfileParserOptions { + DepfileParserOptions() + : depfile_distinct_target_lines_action_( + kDepfileDistinctTargetLinesActionWarn) {} + DepfileDistinctTargetLinesAction + depfile_distinct_target_lines_action_; +}; + /// Parser for the dependency information emitted by gcc's -M flags. struct DepfileParser { + explicit DepfileParser(DepfileParserOptions options = + DepfileParserOptions()); + /// Parse an input file. Input must be NUL-terminated. /// Warning: may mutate the content in-place and parsed StringPieces are /// pointers within it. @@ -30,6 +46,7 @@ struct DepfileParser { StringPiece out_; vector ins_; + DepfileParserOptions options_; }; #endif // NINJA_DEPFILE_PARSER_H_ diff --git a/src/depfile_parser.in.cc b/src/depfile_parser.in.cc index d299ee2..f8c94b3 100644 --- a/src/depfile_parser.in.cc +++ b/src/depfile_parser.in.cc @@ -13,6 +13,12 @@ // limitations under the License. #include "depfile_parser.h" +#include "util.h" + +DepfileParser::DepfileParser(DepfileParserOptions options) + : options_(options) +{ +} // A note on backslashes in Makefiles, from reading the docs: // Backslash-newline is the line continuation character. @@ -36,6 +42,8 @@ bool DepfileParser::Parse(string* content, string* err) { char* end = in + content->size(); bool have_target = false; bool have_secondary_target_on_this_rule = false; + bool have_newline_since_primary_target = false; + bool warned_distinct_target_lines = false; bool parsing_targets = true; while (in < end) { bool have_newline = false; @@ -119,8 +127,23 @@ bool DepfileParser::Parse(string* content, string* err) { if (len > 0) { if (is_dependency) { if (have_secondary_target_on_this_rule) { - *err = "depfile has multiple output paths"; - return false; + if (!have_newline_since_primary_target) { + *err = "depfile has multiple output paths"; + return false; + } else if (options_.depfile_distinct_target_lines_action_ == + kDepfileDistinctTargetLinesActionError) { + *err = + "depfile has multiple output paths (on separate lines)" + " [-w depfilemulti=err]"; + return false; + } else { + if (!warned_distinct_target_lines) { + warned_distinct_target_lines = true; + Warning("depfile has multiple output paths (on separate lines); " + "continuing anyway [-w depfilemulti=warn]"); + } + continue; + } } ins_.push_back(StringPiece(filename, len)); } else if (!out_.str_) { @@ -134,6 +157,9 @@ bool DepfileParser::Parse(string* content, string* err) { // A newline ends a rule so the next filename will be a new target. parsing_targets = true; have_secondary_target_on_this_rule = false; + if (have_target) { + have_newline_since_primary_target = true; + } } } if (!have_target) { diff --git a/src/depfile_parser_test.cc b/src/depfile_parser_test.cc index 70e4029..52fe7cd 100644 --- a/src/depfile_parser_test.cc +++ b/src/depfile_parser_test.cc @@ -276,8 +276,15 @@ TEST_F(DepfileParserTest, MultipleRulesTolerateMP) { TEST_F(DepfileParserTest, MultipleRulesRejectDifferentOutputs) { // check that multiple different outputs are rejected by the parser // when spread across multiple rules + DepfileParserOptions parser_opts; + parser_opts.depfile_distinct_target_lines_action_ = + kDepfileDistinctTargetLinesActionError; + DepfileParser parser(parser_opts); string err; - EXPECT_FALSE(Parse("foo: x y\n" - "bar: y z\n", &err)); - ASSERT_EQ("depfile has multiple output paths", err); + string input = + "foo: x y\n" + "bar: y z\n"; + EXPECT_FALSE(parser.Parse(&input, &err)); + ASSERT_EQ("depfile has multiple output paths (on separate lines)" + " [-w depfilemulti=err]", err); } diff --git a/src/disk_interface_test.cc b/src/disk_interface_test.cc index 2de4f28..bac515d 100644 --- a/src/disk_interface_test.cc +++ b/src/disk_interface_test.cc @@ -213,7 +213,7 @@ TEST_F(DiskInterfaceTest, RemoveFile) { struct StatTest : public StateTestWithBuiltinRules, public DiskInterface { - StatTest() : scan_(&state_, NULL, NULL, this) {} + StatTest() : scan_(&state_, NULL, NULL, this, NULL) {} // DiskInterface implementation. virtual TimeStamp Stat(const string& path, string* err) const; diff --git a/src/graph.cc b/src/graph.cc index b41c247..9c2f784 100644 --- a/src/graph.cc +++ b/src/graph.cc @@ -491,7 +491,9 @@ bool ImplicitDepLoader::LoadDepFile(Edge* edge, const string& path, return false; } - DepfileParser depfile; + DepfileParser depfile(depfile_parser_options_ + ? *depfile_parser_options_ + : DepfileParserOptions()); string depfile_err; if (!depfile.Parse(&content, &depfile_err)) { *err = path + ": " + depfile_err; diff --git a/src/graph.h b/src/graph.h index a8f0641..d58fecd 100644 --- a/src/graph.h +++ b/src/graph.h @@ -24,6 +24,7 @@ using namespace std; #include "util.h" struct BuildLog; +struct DepfileParserOptions; struct DiskInterface; struct DepsLog; struct Edge; @@ -209,8 +210,10 @@ struct Edge { /// "depfile" attribute in build files. struct ImplicitDepLoader { ImplicitDepLoader(State* state, DepsLog* deps_log, - DiskInterface* disk_interface) - : state_(state), disk_interface_(disk_interface), deps_log_(deps_log) {} + DiskInterface* disk_interface, + DepfileParserOptions const* depfile_parser_options) + : state_(state), disk_interface_(disk_interface), deps_log_(deps_log), + depfile_parser_options_(depfile_parser_options) {} /// Load implicit dependencies for \a edge. /// @return false on error (without filling \a err if info is just missing @@ -242,6 +245,7 @@ struct ImplicitDepLoader { State* state_; DiskInterface* disk_interface_; DepsLog* deps_log_; + DepfileParserOptions const* depfile_parser_options_; }; @@ -249,10 +253,11 @@ struct ImplicitDepLoader { /// and updating the dirty/outputs_ready state of all the nodes and edges. struct DependencyScan { DependencyScan(State* state, BuildLog* build_log, DepsLog* deps_log, - DiskInterface* disk_interface) + DiskInterface* disk_interface, + DepfileParserOptions const* depfile_parser_options) : build_log_(build_log), disk_interface_(disk_interface), - dep_loader_(state, deps_log, disk_interface) {} + dep_loader_(state, deps_log, disk_interface, depfile_parser_options) {} /// Update the |dirty_| state of the given node by inspecting its input edge. /// Examine inputs, outputs, and command lines to judge whether an edge diff --git a/src/graph_test.cc b/src/graph_test.cc index 422bc9a..4a66831 100644 --- a/src/graph_test.cc +++ b/src/graph_test.cc @@ -18,7 +18,7 @@ #include "test.h" struct GraphTest : public StateTestWithBuiltinRules { - GraphTest() : scan_(&state_, NULL, NULL, &fs_) {} + GraphTest() : scan_(&state_, NULL, NULL, &fs_, NULL) {} VirtualFileSystem fs_; DependencyScan scan_; diff --git a/src/ninja.cc b/src/ninja.cc index a3d73ad..b608426 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -73,6 +73,10 @@ struct Options { /// Whether phony cycles should warn or print an error. bool phony_cycle_should_err; + + /// Whether a depfile with multiple targets on separate lines should + /// warn or print an error. + bool depfile_distinct_target_lines_should_err; }; /// The Ninja main() loads up a series of data structures; various tools need @@ -912,7 +916,9 @@ bool WarningEnable(const string& name, Options* options) { if (name == "list") { printf("warning flags:\n" " dupbuild={err,warn} multiple build lines for one target\n" -" phonycycle={err,warn} phony build statement references itself\n"); +" phonycycle={err,warn} phony build statement references itself\n" +" depfilemulti={err,warn} depfile has multiple output paths on separate lines\n" + ); return false; } else if (name == "dupbuild=err") { options->dupe_edges_should_err = true; @@ -926,6 +932,12 @@ bool WarningEnable(const string& name, Options* options) { } else if (name == "phonycycle=warn") { options->phony_cycle_should_err = false; return true; + } else if (name == "depfilemulti=err") { + options->depfile_distinct_target_lines_should_err = true; + return true; + } else if (name == "depfilemulti=warn") { + options->depfile_distinct_target_lines_should_err = false; + return true; } else { const char* suggestion = SpellcheckString(name.c_str(), "dupbuild=err", "dupbuild=warn", @@ -1200,6 +1212,11 @@ NORETURN void real_main(int argc, char** argv) { if (exit_code >= 0) exit(exit_code); + if (options.depfile_distinct_target_lines_should_err) { + config.depfile_parser_options.depfile_distinct_target_lines_action_ = + kDepfileDistinctTargetLinesActionError; + } + if (options.working_dir) { // The formatting of this string, complete with funny quotes, is // so Emacs can properly identify that the cwd has changed for -- cgit v0.12