[Core] Fix handling of spaces in makefile deps.
- This handles ' ' and '#' in file names. I investigated but ended up not
making an attempt to handle other characters as Clang itself does not make an
attempt to manage the escapes for them, and the GNU Make semantics appear to
be quite tricky.
- Fixes: https://bugs.swift.org/browse/SR-1468
- Based on a patch by Ankit Agarwal!
diff --git a/include/llbuild/Core/MakefileDepsParser.h b/include/llbuild/Core/MakefileDepsParser.h
index 4cf9625..ee2d3ff 100644
--- a/include/llbuild/Core/MakefileDepsParser.h
+++ b/include/llbuild/Core/MakefileDepsParser.h
@@ -13,6 +13,10 @@
#ifndef LLBUILD_CORE_MAKEFILEDEPSPARSER_H
#define LLBUILD_CORE_MAKEFILEDEPSPARSER_H
+#include "llbuild/Basic/LLVM.h"
+
+#include "llvm/ADT/StringRef.h"
+
#include <cstdint>
namespace llbuild {
@@ -36,13 +40,30 @@
virtual void error(const char* message, uint64_t position) = 0;
/// Called when a new rule is encountered.
- virtual void actOnRuleStart(const char* name, uint64_t length) = 0;
+ ///
+ /// \param name - A pointer to the rule name string start.
+ ///
+ /// \param length - The raw length of the rule name string, including escape
+ /// sequences.
+ ///
+ /// \param unescapedWord - An unescaped version of the name.
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) = 0;
+
/// Called when a new dependency is found for the current rule.
///
/// This is only called between paired calls to \see actOnRuleStart() and
/// \see actOnRuleEnd().
- virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) = 0;
+ ///
+ /// \param dependency - A pointer to the dependency string start.
+ ///
+ /// \param length - The raw length of the dependency string, including
+ /// escape sequences.
+ ///
+ /// \param unescapedWord - An unescaped version of the dependency.
+ virtual void actOnRuleDependency(const char* dependency, uint64_t length,
+ const StringRef unescapedWord) = 0;
+
/// Called when a rule is complete.
virtual void actOnRuleEnd() = 0;
};
diff --git a/lib/BuildSystem/BuildSystem.cpp b/lib/BuildSystem/BuildSystem.cpp
index 34a6f80..e913420 100644
--- a/lib/BuildSystem/BuildSystem.cpp
+++ b/lib/BuildSystem/BuildSystem.cpp
@@ -1000,12 +1000,14 @@
}
virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) override {
- bsci.taskDiscoveredDependency(
- task, BuildKey::makeNode(StringRef(dependency, length)));
+ uint64_t length,
+ const StringRef unescapedWord) override {
+ bsci.taskDiscoveredDependency(task, BuildKey::makeNode(unescapedWord));
}
- virtual void actOnRuleStart(const char* name, uint64_t length) override {}
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) override {}
+
virtual void actOnRuleEnd() override {}
};
@@ -1247,12 +1249,14 @@
}
virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) override {
- bsci.taskDiscoveredDependency(
- task, BuildKey::makeNode(StringRef(dependency, length)));
+ uint64_t length,
+ const StringRef unescapedWord) override {
+ bsci.taskDiscoveredDependency(task, BuildKey::makeNode(unescapedWord));
}
- virtual void actOnRuleStart(const char* name, uint64_t length) override {}
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) override {}
+
virtual void actOnRuleEnd() override {}
};
diff --git a/lib/BuildSystem/SwiftTools.cpp b/lib/BuildSystem/SwiftTools.cpp
index a76b8f5..788efc0 100644
--- a/lib/BuildSystem/SwiftTools.cpp
+++ b/lib/BuildSystem/SwiftTools.cpp
@@ -468,16 +468,19 @@
}
virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) override {
+ uint64_t length,
+ const StringRef unescapedWord) override {
// Only process dependencies for the first rule (the output file), the
// rest are identical.
if (ruleNumber == 0) {
bsci.taskDiscoveredDependency(
- task, BuildKey::makeNode(StringRef(dependency, length)));
+ task, BuildKey::makeNode(unescapedWord));
}
}
- virtual void actOnRuleStart(const char* name, uint64_t length) override {}
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) override {}
+
virtual void actOnRuleEnd() override {
++ruleNumber;
}
diff --git a/lib/Commands/NinjaBuildCommand.cpp b/lib/Commands/NinjaBuildCommand.cpp
index 1394ccf..5b8ac09 100644
--- a/lib/Commands/NinjaBuildCommand.cpp
+++ b/lib/Commands/NinjaBuildCommand.cpp
@@ -1376,13 +1376,14 @@
}
virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) override {
- context.engine.taskDiscoveredDependency(
- task, std::string(dependency, dependency+length));
+ uint64_t length,
+ const StringRef
+ unescapedWord) override {
+ context.engine.taskDiscoveredDependency(task, unescapedWord);
}
- virtual void actOnRuleStart(const char* name,
- uint64_t length) override {}
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) override {}
virtual void actOnRuleEnd() override {}
};
diff --git a/lib/Core/MakefileDepsParser.cpp b/lib/Core/MakefileDepsParser.cpp
index 79fe58a..ba263be 100644
--- a/lib/Core/MakefileDepsParser.cpp
+++ b/lib/Core/MakefileDepsParser.cpp
@@ -12,6 +12,9 @@
#include "llbuild/Core/MakefileDepsParser.h"
+#include "llvm/ADT/SmallString.h"
+
+using namespace llvm;
using namespace llbuild;
using namespace llbuild::core;
@@ -86,7 +89,9 @@
}
}
-static void lexWord(const char*& cur, const char* end) {
+static void lexWord(const char*& cur, const char* end,
+ SmallVectorImpl<char> &unescapedWord) {
+ unescapedWord.clear();
for (; cur != end; ++cur) {
int c = *cur;
@@ -98,18 +103,44 @@
// Otherwise, skip the escaped character.
++cur;
+ int c = *cur;
+
+ // Honor the escaping rules as generated by Clang and GCC, which are *not
+ // necessarily* the actual escaping rules of BSD Make or GNU Make. Due to
+ // incompatibilities in the escape syntax between those two makes, and the
+ // GNU make behavior of retrying an escaped string with the original
+ // input, GCC/Clang adopt a strategy around escaping ' ' and '#', but not
+ // r"\\" itself, or any other characters. However, there are some
+ // situations where Clang *will* generate an escaped '\\' using the
+ // r"\\\\" sequence, so we also honor that.
+ //
+ // FIXME: Make this more complete, or move to a better dependency format.
+ if (c == ' ' || c == '#' || c == '\\') {
+ unescapedWord.push_back(c);
+ } else {
+ unescapedWord.push_back('\\');
+ unescapedWord.push_back(c);
+ }
+ continue;
+ } else if (c == '$' && cur + 1 != end && cur[1] == '$') {
+ // "$$" is an escaped '$'.
+ unescapedWord.push_back(c);
+ ++cur;
continue;
}
// Otherwise, if this is not a valid word character then skip it.
- if (!isWordChar(*cur))
+ if (!isWordChar(c))
break;
+ unescapedWord.push_back(c);
}
}
void MakefileDepsParser::parse() {
const char* cur = data;
const char* end = data + length;
+ // Storage for currently begin lexed unescaped word.
+ SmallString<256> unescapedWord;
// While we have input data...
while (cur != end) {
@@ -122,13 +153,13 @@
// The next token should be a word.
const char* wordStart = cur;
- lexWord(cur, end);
+ lexWord(cur, end, unescapedWord);
if (cur == wordStart) {
actions.error("unexpected character in file", cur - data);
skipToEndOfLine(cur, end);
continue;
}
- actions.actOnRuleStart(wordStart, cur - wordStart);
+ actions.actOnRuleStart(wordStart, cur - wordStart, unescapedWord.str());
// The next token should be a colon.
skipNonNewlineWhitespace(cur, end);
@@ -151,13 +182,14 @@
// Otherwise, we should have a word.
const char* wordStart = cur;
- lexWord(cur, end);
+ lexWord(cur, end, unescapedWord);
if (cur == wordStart) {
actions.error("unexpected character in prerequisites", cur - data);
skipToEndOfLine(cur, end);
continue;
}
- actions.actOnRuleDependency(wordStart, cur - wordStart);
+ actions.actOnRuleDependency(wordStart, cur - wordStart,
+ unescapedWord.str());
}
actions.actOnRuleEnd();
}
diff --git a/tests/BuildSystem/Build/discovered-makefile-deps.llbuild b/tests/BuildSystem/Build/discovered-makefile-deps.llbuild
index 247e2f7..d7a2565 100644
--- a/tests/BuildSystem/Build/discovered-makefile-deps.llbuild
+++ b/tests/BuildSystem/Build/discovered-makefile-deps.llbuild
@@ -5,7 +5,7 @@
# RUN: rm -rf %t.build
# RUN: mkdir -p %t.build
-# RUN: touch %t.build/header-1 %t.build/input-1
+# RUN: touch %t.build/header\ with\ space-1 %t.build/input-1
# RUN: cp %s %t.build/build.llbuild
@@ -20,7 +20,7 @@
# Check a build that modifies the header.
#
-# RUN: echo "mod" >> %t.build/header-1
+# RUN: echo "mod" >> %t.build/header\ with\ space-1
# RUN: %{llbuild} buildsystem build --serial --chdir %t.build &> %t2.out
# RUN: %{FileCheck} --check-prefix=CHECK-AFTER-MOD --input-file=%t2.out %s
#
@@ -38,7 +38,7 @@
tool: shell
inputs: ["input-1"]
outputs: ["output-1"]
- args: "echo \"output-1: input-1 header-1\" > output-1.d && cat input-1 header-1 > output-1"
+ args: "echo \"output-1: input-1 header\\ with\\ space-1\" > output-1.d && cat input-1 header\\ with\\ space-1 > output-1"
description: CC output-1
deps: output-1.d
deps-style: makefile
diff --git a/unittests/Core/MakefileDepsParserTest.cpp b/unittests/Core/MakefileDepsParserTest.cpp
index b380612..806bb97 100644
--- a/unittests/Core/MakefileDepsParserTest.cpp
+++ b/unittests/Core/MakefileDepsParserTest.cpp
@@ -33,15 +33,15 @@
errors.push_back({ message, length });
}
- virtual void actOnRuleStart(const char* name, uint64_t length) override {
- records.push_back({ std::string(name, name + length), {} });
+ virtual void actOnRuleStart(const char* name, uint64_t length,
+ const StringRef unescapedWord) override {
+ records.push_back({ unescapedWord.str(), {} });
}
- virtual void actOnRuleDependency(const char* dependency,
- uint64_t length) override {
+ virtual void actOnRuleDependency(const char* dependency, uint64_t length,
+ const StringRef unescapedWord) override {
assert(!records.empty());
- records.back().second.push_back(std::string(dependency,
- dependency+length));
+ records.back().second.push_back(unescapedWord.str());
}
virtual void actOnRuleEnd() override {}
};
@@ -67,6 +67,17 @@
EXPECT_EQ(RuleRecord("a", { "b", "c", "d" }),
actions.records[0]);
+ // Check a valid input with various escaped spaces.
+ actions.errors.clear();
+ actions.records.clear();
+ input = "a\\ b: a\\ b a$$b a\\b a\\#b a\\\\\\ b";
+ MakefileDepsParser(input.data(), input.size(), actions).parse();
+ EXPECT_EQ(0U, actions.errors.size());
+ EXPECT_EQ(1U, actions.records.size());
+ EXPECT_EQ(RuleRecord("a b", {
+ "a b", "a$b", "a\\b", "a#b", "a\\ b" }),
+ actions.records[0]);
+
// Check a basic valid input with two rules.
actions.errors.clear();
actions.records.clear();
