Remove phony edges for nodes created by a dependency loader.

This patch simplifies Ninja internals without modifying
its behavior. It removes the creation (and removal) of phony
edges as producers for nodes loaded by dependency loaders, i.e.
coming from depfiles, dyndep files or the deps log.

These edges were only used to ensure the build did not
abort when these files are missing, unlike regular source
inputs. This can be easily checked by adding a new flag to
the Node class instead. This makes it easier to reason about
how Ninja works internally.

More specifically:

- Move the generated_by_dep_loader_ flag from the Edge class
  to the Node class. The flag is true by default to minimize
  changes to the source code, since node instances can be
  first created by reading the deps or build logs before
  the manifest itself.

- Modify Plan::AddSubTarget() to avoid aborting the build
  when a generated-by-deploader node is missing. Instead
  the function exits immediately, which corresponds to
  what happened before.

- State::AddOut(), State::AddIn(), State::AddValidation():
  Ensure that nodes added by these methods, which are
  only called from the manifest parser and unit-tests
  set the |generated_by_dep_loader_| flag to false, to
  indicate that these are regular input / output nodes.

- ManifestParser::ParseEdge(): Add an assertion verifying
  that the dyndep file is marked as a regular input.

- DyndepLoader::UpdateEdge(): Remove code path that
  looked for phony in-edges and ignored them.

- DepLoader::CreatePhonyInEdge() is removed as no
  longer necessary.

+ Update a few places in unit-tests that were checking
  for the creation of the phony edges.

Fuchsia-Topic: persistent-mode
Change-Id: I98998238002351ef9c7a103040eb8a26d4183969

8 files changed, 66 insertions, 60 deletions

diff --git a/src/build.cc b/src/build.cc
index 76ff93a..6903e45 100644
--- a/src/build.cc
+++ b/src/build.cc
@@ -95,15 +95,20 @@ bool Plan::AddTarget(const Node* target, string* err) {
 bool Plan::AddSubTarget(const Node* node, const Node* dependent, string* err,
                         set<Edge*>* dyndep_walk) {
   Edge* edge = node->in_edge();
-  if (!edge) {  // Leaf node.
-    if (node->dirty()) {
-      string referenced;
-      if (dependent)
-        referenced = ", needed by '" + dependent->path() + "',";
-      *err = "'" + node->path() + "'" + referenced + " missing "
-             "and no known rule to make it";
-    }
-    return false;
+  if (!edge) {
+     // Leaf node, this can be either a regular input from the manifest
+     // (e.g. a source file), or an implicit input from a depfile or dyndep
+     // file. In the first case, a dirty flag means the file is missing,
+     // and the build should stop. In the second, do not do anything here
+     // since there is no producing edge to add to the plan.
+     if (node->dirty() && !node->generated_by_dep_loader()) {
+       string referenced;
+       if (dependent)
+         referenced = ", needed by '" + dependent->path() + "',";
+       *err = "'" + node->path() + "'" + referenced +
+              " missing and no known rule to make it";
+     }
+     return false;
   }
 
   if (edge->outputs_ready())
diff --git a/src/build_test.cc b/src/build_test.cc
index d32ad3e..5ed8245 100644
--- a/src/build_test.cc
+++ b/src/build_test.cc
@@ -986,9 +986,19 @@ TEST_F(BuildTest, DepFileOK) {
   ASSERT_EQ(1u, fs_.files_read_.size());
   EXPECT_EQ("foo.o.d", fs_.files_read_[0]);
 
-  // Expect three new edges: one generating foo.o, and two more from
-  // loading the depfile.
-  ASSERT_EQ(orig_edges + 3, (int)state_.edges_.size());
+  // Expect one new edge generating foo.o. Loading the depfile should have
+  // added nodes, but not phony edges to the graph.
+  ASSERT_EQ(orig_edges + 1, (int)state_.edges_.size());
+
+  // Verify that nodes for blah.h and bar.h were added and that they
+  // are marked as generated by a dep loader.
+  ASSERT_FALSE(state_.LookupNode("foo.o")->generated_by_dep_loader());
+  ASSERT_FALSE(state_.LookupNode("foo.c")->generated_by_dep_loader());
+  ASSERT_TRUE(state_.LookupNode("blah.h"));
+  ASSERT_TRUE(state_.LookupNode("blah.h")->generated_by_dep_loader());
+  ASSERT_TRUE(state_.LookupNode("bar.h"));
+  ASSERT_TRUE(state_.LookupNode("bar.h")->generated_by_dep_loader());
+
   // Expect our edge to now have three inputs: foo.c and two headers.
   ASSERT_EQ(3u, edge->inputs_.size());
 
@@ -1154,7 +1164,6 @@ TEST_F(BuildTest, DepFileCanonicalize) {
   ASSERT_NO_FATAL_FAILURE(AssertParse(&state_,
 "rule cc\n  command = cc $in\n  depfile = $out.d\n"
 "build gen/stuff\\things/foo.o: cc x\\y/z\\foo.c\n"));
-  Edge* edge = state_.edges_.back();
 
   fs_.Create("x/y/z/foo.c", "");
   GetNode("bar.h")->MarkDirty();  // Mark bar.h as missing.
@@ -1167,10 +1176,10 @@ TEST_F(BuildTest, DepFileCanonicalize) {
   // The depfile path does not get Canonicalize as it seems unnecessary.
   EXPECT_EQ("gen/stuff\\things/foo.o.d", fs_.files_read_[0]);
 
-  // Expect three new edges: one generating foo.o, and two more from
-  // loading the depfile.
-  ASSERT_EQ(orig_edges + 3, (int)state_.edges_.size());
+  // Expect one new edge enerating foo.o.
+  ASSERT_EQ(orig_edges + 1, (int)state_.edges_.size());
   // Expect our edge to now have three inputs: foo.c and two headers.
+  Edge* edge = state_.edges_.back();
   ASSERT_EQ(3u, edge->inputs_.size());
 
   // Expect the command line we generate to only use the original input, and
@@ -2968,9 +2977,9 @@ TEST_F(BuildWithDepsLogTest, DepFileOKDepsLog) {
     EXPECT_TRUE(builder.AddTarget("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 one new edge generating fo o.o, loading the depfile should
+    // note generate new edges.
+    ASSERT_EQ(1u, state.edges_.size());
     // Expect our edge to now have three inputs: foo.c and two headers.
     ASSERT_EQ(3u, edge->inputs_.size());
 
@@ -3110,16 +3119,14 @@ TEST_F(BuildWithDepsLogTest, DepFileDepsLogCanonicalize) {
     Builder builder(&state, config_, NULL, &deps_log, &fs_, &status_, 0);
     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 one new edge generating fo o.o.
+    ASSERT_EQ(1u, state.edges_.size());
     // Expect our edge to now have three inputs: foo.c and two headers.
+    Edge* edge = state.edges_.back();
     ASSERT_EQ(3u, edge->inputs_.size());
 
     // Expect the command line we generate to only use the original input.
@@ -3675,8 +3682,8 @@ TEST_F(BuildTest, DyndepBuildDiscoverOutputAndDepfileInput) {
   EXPECT_TRUE(builder_.AddTarget("out", &err));
   ASSERT_EQ("", err);
 
-  // Loading the depfile gave tmp.imp a phony input edge.
-  ASSERT_TRUE(GetNode("tmp.imp")->in_edge()->is_phony());
+  // Loading the depfile did not give tmp.imp a phony input edge.
+  ASSERT_FALSE(GetNode("tmp.imp")->in_edge());
 
   EXPECT_TRUE(builder_.Build(&err));
   EXPECT_EQ("", err);
diff --git a/src/dyndep.cc b/src/dyndep.cc
index dd4ed09..a0d699d 100644
--- a/src/dyndep.cc
+++ b/src/dyndep.cc
@@ -97,15 +97,10 @@ bool DyndepLoader::UpdateEdge(Edge* edge, Dyndeps const* dyndeps,
   for (std::vector<Node*>::const_iterator i =
            dyndeps->implicit_outputs_.begin();
        i != dyndeps->implicit_outputs_.end(); ++i) {
-    if (Edge* old_in_edge = (*i)->in_edge()) {
-      // This node already has an edge producing it.  Fail with an error
-      // unless the edge was generated by ImplicitDepLoader, in which
-      // case we can replace it with the now-known real producer.
-      if (!old_in_edge->generated_by_dep_loader_) {
-        *err = "multiple rules generate " + (*i)->path();
-        return false;
-      }
-      old_in_edge->outputs_.clear();
+    if ((*i)->in_edge()) {
+      // This node already has an edge producing it.
+      *err = "multiple rules generate " + (*i)->path();
+      return false;
     }
     (*i)->set_in_edge(edge);
   }
diff --git a/src/graph.cc b/src/graph.cc
index 199294d..62f13ec 100644
--- a/src/graph.cc
+++ b/src/graph.cc
@@ -728,7 +728,6 @@ bool ImplicitDepLoader::ProcessDepfileDeps(
     Node* node = state_->GetNode(*i, slash_bits);
     *implicit_dep = node;
     node->AddOutEdge(edge);
-    CreatePhonyInEdge(node);
   }
 
   return true;
@@ -756,7 +755,6 @@ bool ImplicitDepLoader::LoadDepsFromLog(Edge* edge, string* err) {
     Node* node = deps->nodes[i];
     *implicit_dep = node;
     node->AddOutEdge(edge);
-    CreatePhonyInEdge(node);
   }
   return true;
 }
@@ -768,21 +766,3 @@ vector<Node*>::iterator ImplicitDepLoader::PreallocateSpace(Edge* edge,
   edge->implicit_deps_ += count;
   return edge->inputs_.end() - edge->order_only_deps_ - count;
 }
-
-void ImplicitDepLoader::CreatePhonyInEdge(Node* node) {
-  if (node->in_edge())
-    return;
-
-  Edge* phony_edge = state_->AddEdge(&State::kPhonyRule);
-  phony_edge->generated_by_dep_loader_ = true;
-  node->set_in_edge(phony_edge);
-  phony_edge->outputs_.push_back(node);
-
-  // RecomputeDirty might not be called for phony_edge if a previous call
-  // to RecomputeDirty had caused the file to be stat'ed.  Because previous
-  // invocations of RecomputeDirty would have seen this node without an
-  // input edge (and therefore ready), we have to set outputs_ready_ to true
-  // to avoid a potential stuck build.  If we do call RecomputeDirty for
-  // this node, it will simply set outputs_ready_ to the correct value.
-  phony_edge->outputs_ready_ = true;
-}
diff --git a/src/graph.h b/src/graph.h
index d07a9b7..5c8ca2c 100644
--- a/src/graph.h
+++ b/src/graph.h
@@ -104,6 +104,14 @@ struct Node {
   Edge* in_edge() const { return in_edge_; }
   void set_in_edge(Edge* edge) { in_edge_ = edge; }
 
+  /// Indicates whether this node was generated from a depfile or dyndep file,
+  /// instead of being a regular input or output from the Ninja manifest.
+  bool generated_by_dep_loader() const { return generated_by_dep_loader_; }
+
+  void set_generated_by_dep_loader(bool value) {
+    generated_by_dep_loader_ = value;
+  }
+
   int id() const { return id_; }
   void set_id(int id) { id_ = id; }
 
@@ -146,6 +154,13 @@ private:
   /// has not yet been loaded.
   bool dyndep_pending_;
 
+  /// Set to true when this node comes from a depfile, a dyndep file or the
+  /// deps log. If it does not have a producing edge, the build should not
+  /// abort if it is missing (as for regular source inputs). By default
+  /// all nodes have this flag set to true, since the deps and build logs
+  /// can be loaded before the manifest.
+  bool generated_by_dep_loader_ = true;
+
   /// The Edge that produces this Node, or NULL when there is no
   /// known edge to produce it.
   Edge* in_edge_;
@@ -297,11 +312,6 @@ struct ImplicitDepLoader {
   /// an iterator pointing at the first new space.
   std::vector<Node*>::iterator PreallocateSpace(Edge* edge, int count);
 
-  /// If we don't have a edge that generates this input already,
-  /// create one; this makes us not abort if the input is missing,
-  /// but instead will rebuild in that circumstance.
-  void CreatePhonyInEdge(Node* node);
-
   State* state_;
   DiskInterface* disk_interface_;
   DepsLog* deps_log_;
diff --git a/src/manifest_parser.cc b/src/manifest_parser.cc
index 8db6eb3..103c365 100644
--- a/src/manifest_parser.cc
+++ b/src/manifest_parser.cc
@@ -14,8 +14,10 @@
 
 #include "manifest_parser.h"
 
+#include <assert.h>
 #include <stdio.h>
 #include <stdlib.h>
+
 #include <vector>
 
 #include "graph.h"
@@ -416,6 +418,7 @@ bool ManifestParser::ParseEdge(string* err) {
     if (dgi == edge->inputs_.end()) {
       return lexer_.Error("dyndep '" + dyndep + "' is not an input", err);
     }
+    assert(!edge->dyndep_->generated_by_dep_loader());
   }
 
   return true;
diff --git a/src/state.cc b/src/state.cc
index 556b0d8..0a68f21 100644
--- a/src/state.cc
+++ b/src/state.cc
@@ -128,6 +128,7 @@ Node* State::SpellcheckNode(const string& path) {
 
 void State::AddIn(Edge* edge, StringPiece path, uint64_t slash_bits) {
   Node* node = GetNode(path, slash_bits);
+  node->set_generated_by_dep_loader(false);
   edge->inputs_.push_back(node);
   node->AddOutEdge(edge);
 }
@@ -138,6 +139,7 @@ bool State::AddOut(Edge* edge, StringPiece path, uint64_t slash_bits) {
     return false;
   edge->outputs_.push_back(node);
   node->set_in_edge(edge);
+  node->set_generated_by_dep_loader(false);
   return true;
 }
 
@@ -145,6 +147,7 @@ void State::AddValidation(Edge* edge, StringPiece path, uint64_t slash_bits) {
   Node* node = GetNode(path, slash_bits);
   edge->validations_.push_back(node);
   node->AddValidationOutEdge(edge);
+  node->set_generated_by_dep_loader(false);
 }
 
 bool State::AddDefault(StringPiece path, string* err) {
diff --git a/src/state.h b/src/state.h
index 878ac6d..886b78f 100644
--- a/src/state.h
+++ b/src/state.h
@@ -105,6 +105,9 @@ struct State {
   Node* LookupNode(StringPiece path) const;
   Node* SpellcheckNode(const std::string& path);
 
+  /// Add input / output / validation nodes to a given edge. This also
+  /// ensures that the generated_by_dep_loader() flag for all these nodes
+  /// is set to false, to indicate that they come from the input manifest.
   void AddIn(Edge* edge, StringPiece path, uint64_t slash_bits);
   bool AddOut(Edge* edge, StringPiece path, uint64_t slash_bits);
   void AddValidation(Edge* edge, StringPiece path, uint64_t slash_bits);