[clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows

[saudi]

Summary

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].

[saudi]

Update patch : clang-format fixes.

There remains some clang-tidy warnings in Path.cpp and Path.h from llvm/Support.
I would like extra opinions on this, because fixing them would break the homogeneity with the rest of the file, which exposes snake_case - like function names.
More code from llvm/Support have that clang-tidy mismatch, but adding Support to the clang-tidy exclusion seems a bit extreme...

[saudi]

Ping!

[dblaikie]

Bit out of my wheelhouse hopefully @rsmith or @rnk could chime in at some point.

[rnk]

Seems reasonable

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

534

I'm trying to imagine a use case where the user might be validating diagnostics generated with a Windows style virtual filesystem, but I can't think of how that could happen.

In any case, I think we should drop the ifdef, and just do the special case on all platforms, especially if you tighten it up with the slash check.

534–537

I think looking for a slash after the colon is a bit more robust, and worth doing. [a-zA-Z]:[/\\]

llvm/unittests/Support/Path.cpp

1477

Thank you for adding unit tests!

clang/lib/Frontend/VerifyDiagnosticConsumer.cpp

if (!PH.Next("@")) {
           ExpectedLoc = SM.translateLineCol(SM.getFileID(Pos), Line, 1);
       } else if (PH.NextMarker()) {
         Marker = PH.Match();
       } else if (PP && PH.Search(":")) {
         // Specific source file.
         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("*")) {
             Diags.Report(Pos.getLocWithOffset(PH.C - PH.Begin),
                          diag::err_verify_missing_line)
                 << "'*'";
             continue;
           }

clang/lib/Serialization/ASTWriter.cpp

 
 /// Adjusts the given filename to only write out the portion of the
 /// filename that is not part of the system root directory.
 ///
 /// \param Filename the file name to adjust.
 ///
 /// \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()))
-      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;
-  }
+  StringRef RelativePath = Filename.drop_front(BaseDir.size());
+
+  // Remove leading separators in case there were several in a row
+  const 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 + Pos;
+  return StringRef(I, std::distance(I, Filename.end()));
 }
 
 std::pair<ASTFileSignature, ASTFileSignature>
 ASTWriter::createSignature(StringRef AllBytes, StringRef ASTBlockBytes) {
   llvm::SHA1 Hasher;
   Hasher.update(ASTBlockBytes);
   auto Hash = Hasher.result();
   ASTFileSignature ASTBlockHash = ASTFileSignature::create(Hash);

 
 bool ASTWriter::PreparePathForOutput(SmallVectorImpl<char> &Path) {
   assert(Context && "should have context when outputting path");
 
   bool Changed =
       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;
   }
 
   return Changed;
 }
 
 void ASTWriter::AddPath(StringRef Path, RecordDataImpl &Record) {
   SmallString<128> FilePath(Path);

clang/test/PCH/reloc-windows.c

+// 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}}

clang/test/PCH/reloc.c

-// 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>
 
 int x = 2; // expected-error{{redefinition}}
 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}}

llvm/include/llvm/Support/Path.h

 ///
 /// @param path A path that has its extension replaced with \a extension.
 /// @param extension The extension to be added. It may be empty. It may also
 ///                  optionally start with a '.', if it does not, one will be
 ///                  prepended.
 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
 ///   /foo, /old, /new => /foo
 ///   /old, /old, /new => /new
 ///   /old, /old/, /new => /old
 ///   /old/foo, /old, /new => /new/foo
 ///   /old/foo, /old/, /new => /new/foo

llvm/lib/Support/Path.cpp

void replace_extension(SmallVectorImpl<char> &path, const Twine &extension,
   // Append '.' if needed.
   if (ext.size() > 0 && ext[0] != '.')
     path.push_back('.');
 
   // Append extension.
   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())
       return false;
     for (size_t I = 0, E = Prefix.size(); I != E; ++I) {
       bool SepPath = is_separator(Path[I], style);
       bool SepPrefix = is_separator(Prefix[I], style);
       if (SepPath != SepPrefix)

llvm/unittests/Support/Path.cpp

TEST(Support, RemoveDots) {
   EXPECT_EQ("//foo/bar",
             remove_dots("//foo/bar/", true, path::Style::posix));
 
   SmallString<64> Path2("././c");
   EXPECT_TRUE(path::remove_dots(Path2, true, path::Style::posix));
   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");
   SmallString<64> Path3("/oldnew/foo");
   SmallString<64> Path4("C:\\old/foo\\bar");
   SmallString<64> OldPrefix("/old");
   SmallString<64> OldPrefixSep("/old/");
   SmallString<64> OldPrefixWin("c:/oLD/F");