From 1808e050c1a7e9247676816565391c0be8930a4b Mon Sep 17 00:00:00 2001
From: "David M. Bryson" <dmbryson@apple.com>
Date: Wed, 26 Jun 2019 14:29:23 -0700
Subject: [PATCH] Handle SkippedCommand values in DirectoryContents

Under some cancellation conditions, the directory value observed by
DirectoryContentsTask may be a SkippedCommand. Prior to this change, the
provideInput() handler was assuming that such values had output info and
would read a garbage value and crash (in release mode).

rdar://problem/50380532
---
 lib/BuildSystem/BuildSystem.cpp               |  9 +-
 .../BuildSystem/BuildSystemTaskTests.cpp      | 88 +++++++++++++++++++
 2 files changed, 96 insertions(+), 1 deletion(-)

diff --git a/lib/BuildSystem/BuildSystem.cpp b/lib/BuildSystem/BuildSystem.cpp
index 3c0fc05af..79e5997a1 100644
--- a/lib/BuildSystem/BuildSystem.cpp
+++ b/lib/BuildSystem/BuildSystem.cpp
@@ -880,6 +880,13 @@ class DirectoryContentsTask : public Task {
       return;
     }
 
+    // The input directory may be a 'mkdir' command, which can be cancelled or
+    // skipped by the engine or the delegate. rdar://problem/50380532
+    if (directoryValue.isSkippedCommand()) {
+      engine.taskIsComplete(this, BuildValue::makeSkippedCommand().toData());
+      return;
+    }
+
     std::vector<std::string> filenames;
     getContents(path, filenames);
 
@@ -1294,7 +1301,7 @@ class DirectoryTreeStructureSignatureTask : public Task {
 
       // Request the inputs for each subpath.
       auto value = BuildValue::fromData(valueData);
-      if (value.isMissingInput())
+      if (value.isMissingInput() || value.isSkippedCommand())
         return;
 
       assert(value.isDirectoryContents());
diff --git a/unittests/BuildSystem/BuildSystemTaskTests.cpp b/unittests/BuildSystem/BuildSystemTaskTests.cpp
index 8ce319655..0ac0d917f 100644
--- a/unittests/BuildSystem/BuildSystemTaskTests.cpp
+++ b/unittests/BuildSystem/BuildSystemTaskTests.cpp
@@ -1078,3 +1078,91 @@ TEST(BuildSystemTaskTests, producedNodeAfterPreviouslyMissing) {
     ASSERT_TRUE(!afterFileInfo.isMissing());
   }
 }
+
+/// Check that directory contents properly handles when commands have been
+/// skipped. rdar://problem/50380532
+TEST(BuildSystemTaskTests, directoryContentsWithSkippedCommand) {
+  TmpDir tempDir(__func__);
+
+  SmallString<256> manifest{ tempDir.str() };
+  for (auto& c : manifest) {
+    if (c == '\\')
+      c = '/';
+  }
+  sys::path::append(manifest, "manifest.llbuild");
+
+
+  {
+    std::error_code ec;
+    llvm::raw_fd_ostream os(manifest, ec, llvm::sys::fs::F_Text);
+    assert(!ec);
+
+    os <<
+    "client:\n"
+    "  name: mock\n"
+    "\n"
+    "targets:\n"
+    "  \"\": [\"<all>\"]\n"
+    "\n"
+    "commands:\n"
+    "  \"mkdir-inputDir\":\n"
+    "    tool: mkdir\n"
+    "    outputs: [\"inputDir\"]\n"
+    "  \"read-inputDir\":\n"
+    "    tool: shell\n"
+    "    inputs: [\"inputDir/\"]\n"
+    "    outputs: [\"read-inputDir\"]\n"
+    "    description: \"read-inputDir\"\n"
+    "    args:\n"
+    "      touch read-inputDir\n"
+    "  \"<all>\":\n"
+    "    tool: phony\n"
+    "    inputs: [\"read-inputDir\"]\n"
+    "    outputs: [\"<all>\"]";
+  }
+
+  class CancellingDelegate: public MockBuildSystemDelegate {
+  public:
+    BuildSystem* system = nullptr;
+
+    CancellingDelegate(): MockBuildSystemDelegate() { }
+
+    virtual bool shouldCommandStart(Command* command) override {
+      if (command->getName() == "mkdir-inputDir") {
+        return false;
+      }
+      return true;
+    }
+  };
+
+  // Create the build system.
+  CancellingDelegate delegate;
+  BuildSystem system(delegate, createLocalFileSystem());
+  delegate.system = &system;
+  bool loadingResult = system.loadDescription(manifest);
+  ASSERT_TRUE(loadingResult);
+
+  // Build the default target
+  auto result = system.build(BuildKey::makeTarget(""));
+  ASSERT_TRUE(result.hasValue());
+
+  // The contents should propagate the skipped command
+  result = system.build(BuildKey::makeDirectoryContents("inputDir"));
+  ASSERT_TRUE(result.hasValue());
+  ASSERT_TRUE(result.getValue().isSkippedCommand());
+
+  // Signatures don't need the contents and should be fine with encoding the
+  // skipped value in the signature.
+  result = system.build(BuildKey::makeDirectoryTreeSignature("inputDir", {}));
+  ASSERT_TRUE(result.hasValue());
+  ASSERT_FALSE(result.getValue().isSkippedCommand());
+
+  auto filters = StringList({"filter"});
+  result = system.build(BuildKey::makeDirectoryTreeSignature("inputDir", filters));
+  ASSERT_TRUE(result.hasValue());
+  ASSERT_FALSE(result.getValue().isSkippedCommand());
+
+  result = system.build(BuildKey::makeDirectoryTreeStructureSignature("inputDir"));
+  ASSERT_TRUE(result.hasValue());
+  ASSERT_FALSE(result.getValue().isSkippedCommand());
+}