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.

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 <string>
 #include <vector>
 
+#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<StringPiece> 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