[Core] Fix a crash in cycle detection when scanning from the top item.

 - This also fixes up a few other issues:

	1. We know take the queue locks during cycle discovery, so that we have
       a consistent view of the world.

    2. We now return an empty result value when the build fails due to a cycle
       being discovered, and document this fact.

 - This also extend the BuildEngine unittest to be able to dynamically change
   the inputs during repeated builds, and uses that to test the cycle detection.

include/llbuild/Core/BuildEngine.h

@@ -244,6 +244,10 @@ class BuildEngine {
   /// @{
 
   /// Build the result for a particular key.
+  ///
+  /// \returns The result of computing the key, or the empty value if the key
+  /// could not be computed; the latter case only happens if a cycle was
+  /// discovered currently.
   const ValueType& build(const KeyType& key);
 
   /// Attach a database for persisting build state.

lib/Core/BuildEngine.cpp

@@ -587,7 +587,10 @@ class BuildEngineImpl {
   }
 
   /// Execute all of the work pending in the engine queues until they are empty.
-  void executeTasks() {
+  ///
+  /// \returns True on success, false if the build could not be completed; the
+  /// latter only occurs when the build contains a cycle currently.
+  bool executeTasks() {
     std::vector<TaskInputRequest> finishedInputRequests;
 
     // Process requests as long as we have work to do.
@@ -853,10 +856,17 @@ class BuildEngineImpl {
     // we have found a cycle and are deadlocked.
     if (!taskInfos.empty()) {
       reportCycle();
+      return false;
     }
+
+    return true;
   }
 
   void reportCycle() {
+    // Take all available locks, to ensure we dump a consistent state.
+    std::lock_guard<std::mutex> guard1(taskInfosMutex);
+    std::lock_guard<std::mutex> guard2(finishedTaskInfosMutex);
+
     // Gather all of the successor relationships.
     std::unordered_map<Rule*, std::vector<Rule*>> graph;
     std::vector<RuleScanRecord *> activeRuleScanRecords;
@@ -892,8 +902,10 @@ class BuildEngineImpl {
 
       // For each paused request, add the dependency.
       for (const auto& request: record->pausedInputRequests) {
-        graph[&request.inputRuleInfo->rule].push_back(
-            &request.taskInfo->forRuleInfo->rule);
+        if (request.taskInfo) {
+          graph[&request.inputRuleInfo->rule].push_back(
+              &request.taskInfo->forRuleInfo->rule);
+        }
       }
 
       // Process the deferred scan requests.
@@ -907,6 +919,7 @@ class BuildEngineImpl {
             request.ruleInfo->getPendingScanRecord());
       }
     }
+
     // Find the cycle, which should be reachable from any remaining node.
     //
     // FIXME: Need a setvector.
@@ -1057,7 +1070,7 @@ class BuildEngineImpl {
     inputRequests.push_back({ nullptr, &ruleInfo });
 
     // Run the build engine, to process any necessary tasks.
-    executeTasks();
+    bool success = executeTasks();
 
     // Update the build database, if attached.
     //
@@ -1079,6 +1092,12 @@ class BuildEngineImpl {
     freeRuleScanRecords.clear();
     ruleScanRecordBlocks.clear();
 
+    // If the build failed, return the empty result.
+    if (!success) {
+      static ValueType emptyValue{};
+      return emptyValue;
+    }
+
     // The task queue should be empty and the rule complete.
     assert(taskInfos.empty() && ruleInfo.isComplete(this));
     return ruleInfo.result.value;

unittests/Core/BuildEngineTest.cpp

@@ -14,6 +14,8 @@
 
 #include "llbuild/Core/BuildDB.h"
 
+#include "llvm/Support/ErrorHandling.h"
+
 #include "gtest/gtest.h"
 
 #include <unordered_map>
@@ -25,6 +27,11 @@ using namespace llbuild::core;
 namespace {
 
 class SimpleBuildEngineDelegate : public core::BuildEngineDelegate {
+  /// The cycle, if one was detected during building.
+public:
+  std::vector<std::string> cycle;
+
+private:
   virtual core::Rule lookupRule(const core::KeyType& key) override {
     // We never expect dynamic rule lookup.
     fprintf(stderr, "error: unexpected rule lookup for \"%s\"\n",
@@ -34,9 +41,9 @@ class SimpleBuildEngineDelegate : public core::BuildEngineDelegate {
   }
 
   virtual void cycleDetected(const std::vector<core::Rule*>& items) override {
-    // We never expect a cycle.
-    fprintf(stderr, "error: cycle\n");
-    abort();
+    cycle.clear();
+    std::transform(items.begin(), items.end(), std::back_inserter(cycle),
+                   [](auto rule) { return std::string(rule->key); });
   }
 };
 
@@ -60,22 +67,26 @@ static core::ValueType intToValue(int32_t value) {
 // them all, and then provides the output.
 class SimpleTask : public Task {
 public:
+  typedef std::function<std::vector<KeyType>()> InputListingFnType;
   typedef std::function<int(const std::vector<int>&)> ComputeFnType;
 
 private:
-  std::vector<KeyType> inputs;
+  InputListingFnType listInputs;
   std::vector<int> inputValues;
   ComputeFnType compute;
 
 public:
-  SimpleTask(const std::vector<KeyType>& inputs, ComputeFnType compute)
-      : inputs(inputs), compute(compute)
+  SimpleTask(InputListingFnType listInputs, ComputeFnType compute)
+      : listInputs(listInputs), compute(compute)
   {
-    inputValues.resize(inputs.size());
   }
 
   virtual void start(BuildEngine& engine) override {
+    // Compute the list of inputs.
+    auto inputs = listInputs();
+
     // Request all of the inputs.
+    inputValues.resize(inputs.size());
     for (int i = 0, e = inputs.size(); i != e; ++i) {
       engine.taskNeedsInput(this, inputs[i], i);
     }
@@ -99,7 +110,8 @@ typedef std::function<Task*(BuildEngine&)> ActionFn;
 static ActionFn simpleAction(const std::vector<KeyType>& inputs,
                              SimpleTask::ComputeFnType compute) {
   return [=] (BuildEngine& engine) {
-    return engine.registerTask(new SimpleTask(inputs, compute)); };
+    return engine.registerTask(new SimpleTask([inputs]{ return inputs; },
+                                              compute)); };
 }
 
 TEST(BuildEngineTest, basic) {
@@ -667,4 +679,113 @@ TEST(BuildEngineTest, StatusCallbacks) {
   EXPECT_EQ(2U, numComplete);
 }
 
+/// Check basic cycle detection.
+TEST(BuildEngineTest, SimpleCycle) {
+  SimpleBuildEngineDelegate delegate;
+  core::BuildEngine engine(delegate);
+  engine.addRule({
+      "A",
+      simpleAction({"B"}, [&](const std::vector<int>& inputs) {
+          return 2; }) });
+  engine.addRule({
+      "B",
+      simpleAction({"A"}, [&](const std::vector<int>& inputs) {
+          return 2; }) });
+
+  // Build the result.
+  auto result = engine.build("A");
+  EXPECT_EQ(ValueType{}, result);
+  EXPECT_EQ(std::vector<std::string>({ "A", "B", "A" }), delegate.cycle);
+}
+
+/// Check detection of a cycle discovered during scanning from input.
+TEST(BuildEngineTest, CycleDuringScanningFromTop) {
+  SimpleBuildEngineDelegate delegate;
+  core::BuildEngine engine(delegate);
+  unsigned iteration = 0;
+  engine.addRule({
+      "A",
+      [&](BuildEngine& engine) {
+        return engine.registerTask(
+            new SimpleTask(
+                [&]() -> std::vector<std::string> {
+                  switch (iteration) {
+                  case 0:
+                    return { "B", "C" };
+                  case 1:
+                    return { "B" };
+                  default:
+                    llvm::report_fatal_error("unexpected iterator");
+                  }
+                },
+                [&](const std::vector<int>& inputs) {
+                  return 2; }));
+        },
+      [&](BuildEngine&, const Rule& rule, const ValueType& value) {
+        // Always rebuild
+        return true;
+      }
+    });
+  engine.addRule({
+      "B",
+      [&](BuildEngine& engine) {
+        return engine.registerTask(
+            new SimpleTask(
+                [&]() -> std::vector<std::string> {
+                  switch (iteration) {
+                  case 0:
+                    return { "C" };
+                  case 1:
+                    return { "C" };
+                  default:
+                    llvm::report_fatal_error("unexpected iterator");
+                  }
+                },
+                [&](const std::vector<int>& inputs) {
+                  return 2; }));
+        },
+      [&](BuildEngine&, const Rule& rule, const ValueType& value) {
+        // Always rebuild
+        return true;
+      }
+    });
+  engine.addRule({
+      "C",
+      [&](BuildEngine& engine) {
+        return engine.registerTask(
+            new SimpleTask(
+                [&]() -> std::vector<std::string> {
+                  switch (iteration) {
+                  case 0:
+                    return { };
+                  case 1:
+                    return { "B" };
+                  default:
+                    llvm::report_fatal_error("unexpected iterator");
+                  }
+                },
+                [&](const std::vector<int>& inputs) {
+                  return 2; }));
+        },
+      [&](BuildEngine&, const Rule& rule, const ValueType& value) {
+        // Always rebuild
+        return false;
+      }
+    });
+
+  // Build the result.
+  {
+    EXPECT_EQ(2, intFromValue(engine.build("A")));
+    EXPECT_EQ(std::vector<std::string>({}), delegate.cycle);
+  }
+
+  // Introduce a cycle, and rebuild.
+  {
+    iteration = 1;
+    auto result = engine.build("A");
+    EXPECT_EQ(ValueType{}, result);
+    EXPECT_EQ(std::vector<std::string>({ "B", "C", "B" }), delegate.cycle);
+  }
+}
+
 }