saudi created this revision.
saudi added reviewers: rnk, thakis, rsmith, AndyG.
saudi added a project: clang.
Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, ormris,
hiraditya.
Herald added a project: LLVM.
saudi requested review of this revision.
The test now moves the header files between the pch creation and its use.
This change caused failures under Windows:
- The `-isysroot` path matching was failing when given variations of casing or
separators (`/` vs `\`)
- clang `-verify` checks "`// expected-[note/warning/...]@[file]:[line] ...`"
would fail when `file` is an absolute path, since the `:` character was
interpreted as the delimiter between `[file]` and `[line]`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D92136
Files:
clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
clang/lib/Serialization/ASTWriter.cpp
clang/test/PCH/reloc-windows.c
clang/test/PCH/reloc.c
llvm/include/llvm/Support/Path.h
llvm/lib/Support/Path.cpp
llvm/unittests/Support/Path.cpp
Index: llvm/unittests/Support/Path.cpp
===================================================================
--- llvm/unittests/Support/Path.cpp
+++ llvm/unittests/Support/Path.cpp
@@ -1474,6 +1474,21 @@
EXPECT_EQ("c", Path2);
}
+TEST(Support, StartsWith) {
+ EXPECT_FALSE(starts_with("/foo/bar", "/bar", path::Style::posix));
+ EXPECT_FALSE(starts_with("/foo/bar", "/bar", path::Style::windows));
+ EXPECT_TRUE(starts_with("/foo/bar", "/fo", path::Style::posix));
+ EXPECT_TRUE(starts_with("/foo/bar", "/fo", path::Style::windows));
+ EXPECT_FALSE(starts_with("/foo/bar", "/Fo", path::Style::posix));
+ EXPECT_TRUE(starts_with("/foo/bar", "/Fo", path::Style::windows));
+ EXPECT_TRUE(starts_with("/foo/bar", "/foo", path::Style::posix));
+ EXPECT_TRUE(starts_with("/foo/bar", "/foo", path::Style::windows));
+ EXPECT_TRUE(starts_with("/foo/bar", "/foo/", path::Style::posix));
+ EXPECT_TRUE(starts_with("/foo/bar", "/foo/", path::Style::windows));
+ EXPECT_FALSE(starts_with("A:\\Foo\\Bar", "a:\\foo/bar", path::Style::posix));
+ EXPECT_TRUE(starts_with("A:\\Foo\\Bar", "a:\\foo/bar", path::Style::windows));
+}
+
TEST(Support, ReplacePathPrefix) {
SmallString<64> Path1("/foo");
SmallString<64> Path2("/old/foo");
Index: llvm/lib/Support/Path.cpp
===================================================================
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -494,8 +494,7 @@
path.append(ext.begin(), ext.end());
}
-static bool starts_with(StringRef Path, StringRef Prefix,
- Style style = Style::native) {
+bool starts_with(StringRef Path, StringRef Prefix, Style style) {
// Windows prefix matching : case and separator insensitive
if (real_style(style) == Style::windows) {
if (Path.size() < Prefix.size())
Index: llvm/include/llvm/Support/Path.h
===================================================================
--- llvm/include/llvm/Support/Path.h
+++ llvm/include/llvm/Support/Path.h
@@ -148,6 +148,29 @@
void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
Style style = Style::native);
+
+/// Test for matching path with a prefix.
+///
+/// @code
+/// /foo, /old, /new => /foo
+/// /old, /old, /new => /new
+/// /old, /old/, /new => /old
+/// /old/foo, /old, /new => /new/foo
+/// /old/foo, /old/, /new => /new/foo
+/// /old/foo, /old/, /new/ => /new/foo
+/// /oldfoo, /old, /new => /oldfoo
+/// /foo, <empty>, /new => /new/foo
+/// /foo, <empty>, new => new/foo
+/// /old/foo, /old, <empty> => /foo
+/// @endcode
+///
+/// @param Path The path to check for the prefix
+/// @param Prefix The path prefix to match in \a Path.
+/// @param style The style used to match the prefix. Exact match using
+/// Posix style, case/separator insensitive match for Windows style.
+/// @result true if \a Path begins with Prefix, false otherwise
+bool starts_with(StringRef Path, StringRef Prefix, Style style = Style::native);
+
/// Replace matching path prefix with another path.
///
/// @code
Index: clang/test/PCH/reloc.c
===================================================================
--- clang/test/PCH/reloc.c
+++ clang/test/PCH/reloc.c
@@ -1,8 +1,18 @@
-// RUN: %clang -target x86_64-apple-darwin10 --relocatable-pch -o %t \
-// RUN: -isysroot %S/Inputs/libroot %S/Inputs/libroot/usr/include/reloc.h
-// RUN: %clang -target x86_64-apple-darwin10 -fsyntax-only \
-// RUN: -include-pch %t -isysroot %S/Inputs/libroot %s -Xclang -verify
-// RUN: not %clang -target x86_64-apple-darwin10 -include-pch %t %s
+// RUN: rm -rf %t.dir
+// RUN: mkdir -p %t.dir/Inputs
+// RUN: cp -R %S/Inputs/libroot %t.dir/Inputs/libroot
+// RUN: sed -e "s|TMPDIR|%/t.dir|g" %s > %t.dir/reloc.c
+
+// For this test, we rename the -isysroot folder between the pch file creation and its use.
+// We create the pch using a relative path to avoid introducing absolute file paths in it.
+// Renaming the folder leaves the .h files mtime unchanged, so it doesn't break the pch validation upon use.
+
+// RUN: cd %t.dir && %clang -target x86_64-apple-darwin10 --relocatable-pch -o reloc.pch \
+// RUN: -isysroot %t.dir/Inputs/libroot ./Inputs/libroot/usr/include/reloc.h
+// RUN: mv %t.dir/Inputs %t.dir/Inputs_moved
+// RUN: cd %t.dir && %clang -target x86_64-apple-darwin10 -fsyntax-only \
+// RUN: -include-pch %t.dir/reloc.pch -isysroot %t.dir/Inputs_moved/libroot %t.dir/reloc.c -Xclang -verify
+// RUN: not %clang -target x86_64-apple-darwin10 -include-pch %t.dir/reloc.pch %t.dir/reloc.c
// REQUIRES: x86-registered-target
#include <reloc.h>
@@ -11,5 +21,5 @@
int y = 5; // expected-error{{redefinition}}
-// expected-note@Inputs/libroot/usr/include/reloc.h:13{{previous definition}}
-// expected-note@Inputs/libroot/usr/include/reloc2.h:14{{previous definition}}
+// expected-note@TMPDIR/Inputs_moved/libroot/usr/include/reloc.h:13{{previous definition}}
+// expected-note@TMPDIR/Inputs_moved/libroot/usr/include/reloc2.h:14{{previous definition}}
Index: clang/test/PCH/reloc-windows.c
===================================================================
--- /dev/null
+++ clang/test/PCH/reloc-windows.c
@@ -0,0 +1,29 @@
+// RUN: rm -rf %t.dir
+// RUN: mkdir -p %t.dir/Inputs
+// RUN: cp -R %S/Inputs/libroot %t.dir/Inputs/libroot
+// RUN: sed -e "s|TMPDIR|%/t.dir|g" %s > %t.dir/reloc.c
+
+// For this test, we rename the -isysroot folder between the pch file creation and its use.
+// We create the pch using a relative path to avoid introducing absolute file paths in it.
+// Renaming the folder leaves the .h files mtime unchanged, so it doesn't break the pch validation upon use.
+
+// This Windows-specific test reproduces reloc.c.
+// It validates that --relocatable-pch when -isyroot parameter still works with different casing and separators.
+
+// RUN: cd %t.dir && %clang -target x86_64-apple-darwin10 --relocatable-pch -o reloc.pch \
+// RUN: -isysroot %t.Dir\INPUTS\LibRoot ./Inputs/libroot/usr/include/reloc.h
+// RUN: mv %t.dir/Inputs %t.dir/Inputs_moved
+// RUN: cd %t.dir && %clang -target x86_64-apple-darwin10 -fsyntax-only \
+// RUN: -include-pch %t.dir/reloc.pch -isysroot %t.dir/Inputs_moved/libroot %t.dir/reloc.c -Xclang -verify
+// RUN: not %clang -target x86_64-apple-darwin10 -include-pch %t.dir/reloc.pch %t.dir/reloc.c
+// REQUIRES: x86-registered-target
+// REQUIRES: system-windows
+
+#include <reloc.h>
+
+int x = 2; // expected-error{{redefinition}}
+int y = 5; // expected-error{{redefinition}}
+
+
+// expected-note@TMPDIR/Inputs_moved/libroot/usr/include/reloc.h:13{{previous definition}}
+// expected-note@TMPDIR/Inputs_moved/libroot/usr/include/reloc2.h:14{{previous definition}}
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -988,44 +988,31 @@
///
/// \param BaseDir When non-NULL, the PCH file is a relocatable AST file and
/// the returned filename will be adjusted by this root directory.
+/// Under Windows, the matching is case and separator (/ vs \) insensitive.
///
/// \returns either the original filename (if it needs no adjustment) or the
/// adjusted filename (which points into the @p Filename parameter).
-static const char *
-adjustFilenameForRelocatableAST(const char *Filename, StringRef BaseDir) {
- assert(Filename && "No file name to adjust?");
-
+static StringRef adjustFilenameForRelocatableAST(StringRef Filename,
+ StringRef BaseDir) {
+ using namespace llvm::sys;
if (BaseDir.empty())
return Filename;
- // Verify that the filename and the system root have the same prefix.
- unsigned Pos = 0;
- for (; Filename[Pos] && Pos < BaseDir.size(); ++Pos)
- if (Filename[Pos] != BaseDir[Pos])
- return Filename; // Prefixes don't match.
-
- // We hit the end of the filename before we hit the end of the system root.
- if (!Filename[Pos])
+ if (!path::starts_with(Filename, BaseDir))
return Filename;
- // If there's not a path separator at the end of the base directory nor
- // immediately after it, then this isn't within the base directory.
- if (!llvm::sys::path::is_separator(Filename[Pos])) {
- if (!llvm::sys::path::is_separator(BaseDir.back()))
+ StringRef RelativePath = Filename.drop_front(BaseDir.size());
+
+ // Remove leading separators in case there were several in a row
+ auto I = std::find_if(RelativePath.begin(), RelativePath.end(),
+ [](char c) { return !path::is_separator(c); });
+
+ // Forbid partial match of directory name.
+ // For example, "/path" matches "/path/foo" but not "/pathbar/baz"
+ if (I == RelativePath.begin() && !path::is_separator(RelativePath.back()))
return Filename;
- } else {
- // If the file name has a '/' at the current position, skip over the '/'.
- // We distinguish relative paths from absolute paths by the
- // absence of '/' at the beginning of relative paths.
- //
- // FIXME: This is wrong. We distinguish them by asking if the path is
- // absolute, which isn't the same thing. And there might be multiple '/'s
- // in a row. Use a better mechanism to indicate whether we have emitted an
- // absolute or relative path.
- ++Pos;
- }
- return Filename + Pos;
+ return StringRef(I, std::distance(I, Filename.end()));
}
std::pair<ASTFileSignature, ASTFileSignature>
@@ -4279,11 +4266,11 @@
cleanPathForOutput(Context->getSourceManager().getFileManager(), Path);
// Remove a prefix to make the path relative, if relevant.
- const char *PathBegin = Path.data();
- const char *PathPtr =
- adjustFilenameForRelocatableAST(PathBegin, BaseDirectory);
- if (PathPtr != PathBegin) {
- Path.erase(Path.begin(), Path.begin() + (PathPtr - PathBegin));
+ StringRef PathRef(Path.data(), Path.size());
+ StringRef AdjustedPath =
+ adjustFilenameForRelocatableAST(PathRef, BaseDirectory);
+ if (AdjustedPath.begin() != PathRef.begin()) {
+ Path.erase(Path.begin(), AdjustedPath.begin());
Changed = true;
}
Index: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
===================================================================
--- clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
+++ clang/lib/Frontend/VerifyDiagnosticConsumer.cpp
@@ -531,6 +531,15 @@
StringRef Filename(PH.C, PH.P-PH.C);
PH.Advance();
+#ifdef _WIN32
+ // Support special case of Windows absolute file paths
+ if (Filename.size() == 1 && isLetter(Filename.front()) &&
+ PH.Search(":")) {
+ Filename = StringRef(Filename.begin(), PH.P - Filename.begin());
+ PH.Advance();
+ }
+#endif
+
if (Filename == "*") {
MatchAnyFileAndLine = true;
if (!PH.Next("*")) {
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
