Fix pr31836 on Windows too, and correctly handle repeated separators.

[thakis]

Summary

The approach in D30000 assumes that the '/' returned by path::begin()
is the first element for absolute paths, but that's not true on
Windows.

Also, on Windows backslashes in include lines often end up escaped
so that there are two of them. Having backslashes in include lines
is undefined behavior in most cases and implementation-defined
behavior in C++20, but since clang treats it as normal repeated
path separators, the diagnostic should too.

Unbreaks -Wnonportable-include-path for absolute paths on Windows,
and unbreaks it on non-Windows in the case of absolute paths with
repeated directory separators.

This affects e.g. the #include __FILE__ technique if the file
passed to clang has the wrong case for the drive letter. Before:

C:\src\llvm-project>bin\clang-cl.exe c:\src\llvm-project\test.cc
c:\\src\\llvm-project\\test.cc(4,10): warning: non-portable path to file
    '"c\\srccllvm-projectctest.cc.'; specified path differs in case from
    file name on disk [-Wnonportable-include-path]
     ^

Now:

C:\src\llvm-project> out\gn\bin\clang-cl c:\src\llvm-project\test.cc
c:\\src\\llvm-project\\test.cc(4,10): warning: non-portable path to file
    '"C:\\src\\llvm-project\\test.cc"'; specified path differs in case from
    file name on disk [-Wnonportable-include-path]
     ^

[amccarth]

OK as is, but please consider re-using llvm::sys::path to distinguish separators.

Please also check that there are no regressions in the clang VFS tests.

clang/lib/Lex/PPDirectives.cpp

2118

Can you re-use llvm::sys::path::is_separator instead of inventing a new thing? You can explicitly pass llvm::path::Style::windows if the Microsoft extensions are enabled. Otherwise let it default to llvm::path::Style::native.

[thakis]

Thanks!

clang/lib/Lex/PPDirectives.cpp

Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
   SmallString<1024> SearchPath;
   SmallString<1024> RelativePath;
   // We get the raw path only if we have 'Callbacks' to which we later pass
   // the path.
   ModuleMap::KnownHeader SuggestedModule;
   SourceLocation FilenameLoc = FilenameTok.getLocation();
   StringRef LookupFilename = Filename;
 
-#ifndef _WIN32
+#ifdef _WIN32
+  bool BackslashIsSeparator = true;
+#else
   // Normalize slashes when compiling with -fms-extensions on non-Windows. This
   // is unnecessary on Windows since the filesystem there handles backslashes.
   SmallString<128> NormalizedPath;
+  bool BackslashIsSeparator = false;
   if (LangOpts.MicrosoftExt) {
     NormalizedPath = Filename.str();
     llvm::sys::path::native(NormalizedPath);
     LookupFilename = NormalizedPath;
+    BackslashIsSeparator = true;
   }
 #endif
 
   Optional<FileEntryRef> File = LookupHeaderIncludeOrImport(
       CurDir, Filename, FilenameLoc, FilenameRange, FilenameTok,
       IsFrameworkFound, IsImportDecl, IsMapped, LookupFrom, LookupFromFile,
       LookupFilename, RelativePath, SearchPath, SuggestedModule, isAngled);
 
if (CheckIncludePathPortability) {
     StringRef RealPathName = File->getFileEntry().tryGetRealPathName();
     SmallVector<StringRef, 16> Components(llvm::sys::path::begin(Name),
                                           llvm::sys::path::end(Name));
 
     if (trySimplifyPath(Components, RealPathName)) {
       SmallString<128> Path;
       Path.reserve(Name.size()+2);
       Path.push_back(isAngled ? '<' : '"');
-      bool isLeadingSeparator = llvm::sys::path::is_absolute(Name);
+
+      const auto IsSep = [BackslashIsSeparator](char c) {
+        return c == '/' || (BackslashIsSeparator && c == '\\');
+      };
+
       for (auto Component : Components) {
-        if (isLeadingSeparator)
-          isLeadingSeparator = false;
-        else
+        // On POSIX, Components will contain a single '/' as first element
+        // exactly if Name is an absolute path.
+        // On Windows, it will contain "C:" followed by '\' for absolute paths.
+        // The drive letter is optional for absolute paths on Windows, but
+        // clang currently cannot process absolute paths in #include lines that
+        // don't have a drive.
+        // If the first entry in Components is a directory separator,
+        // then the code at the bottom of this loop that keeps the original
+        // directory separator style copies it. If the second entry is
+        // a directory separator (the C:\ case), then that separator already
+        // got copied when the C: was processed and we want to skip that entry.
+        if (!(Component.size() == 1 && IsSep(Component[0])))
           Path.append(Component);
-        // Append the separator the user used, or the close quote
-        Path.push_back(
-          Path.size() <= Filename.size() ? Filename[Path.size()-1] :
-            (isAngled ? '>' : '"'));
-      }
-      // For user files and known standard headers, by default we issue a diagnostic.
-      // For other system headers, we don't. They can be controlled separately.
-      auto DiagId = (FileCharacter == SrcMgr::C_User || warnByDefaultOnWrongCase(Name)) ?
-          diag::pp_nonportable_path : diag::pp_nonportable_system_path;
+        else if (!Path.empty())
+          continue;
+
+        // Append the separator(s) the user used, or the close quote
+        if (Path.size() > Filename.size()) {
+          Path.push_back(isAngled ? '>' : '"');
+          continue;
+        }
+        assert(IsSep(Filename[Path.size()-1]));
+        do
+          Path.push_back(Filename[Path.size()-1]);
+        while (Path.size() <= Filename.size() && IsSep(Filename[Path.size()-1]));
+      }
+      // For user files and known standard headers, issue a diagnostic.
+      // For other system headers, don't. They can be controlled separately.
+      auto DiagId =
+          (FileCharacter == SrcMgr::C_User || warnByDefaultOnWrongCase(Name))
+              ? diag::pp_nonportable_path
+              : diag::pp_nonportable_system_path;
       Diag(FilenameTok, DiagId) << Path <<
         FixItHint::CreateReplacement(FilenameRange, Path);
     }
   }
 
   switch (Action) {
   case Skip:
     // If we don't need to enter the file, stop now.

clang/test/Lexer/case-insensitive-include-ms.c

 // REQUIRES: case-insensitive-filesystem
 
 // RUN: mkdir -p %t/Output/apath
 // RUN: cp %S/Inputs/case-insensitive-include.h %t/Output
 // RUN: cd %t/Output
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %t/Output -verify
 // RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %t/Output 2>&1 | FileCheck %s
 
+// FIXME: Add a test with repeated backslashes once clang can handle that
+// in ms-compat mode on non-Windows hosts.
 #include "..\Output\.\case-insensitive-include.h"
 #include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
 #include "..\output\.\case-insensitive-include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
 
 #include "apath\..\.\case-insensitive-include.h"
 #include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
 #include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(

clang/test/Lexer/case-insensitive-include-pr31836.sh

 // REQUIRES: case-insensitive-filesystem
-// UNSUPPORTED: system-windows
 
 // RUN: mkdir -p %t
 // RUN: touch %t/case-insensitive-include-pr31836.h
 // RUN: echo "#include \"%t/Case-Insensitive-Include-Pr31836.h\"" | %clang_cc1 -E - 2>&1 | FileCheck %s
 
 // CHECK: warning: non-portable path to file
 // CHECK-SAME: /case-insensitive-include-pr31836.h

clang/test/Lexer/case-insensitive-include.c

 // Known standard header, so warn:
 #include <StdDef.h> // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:"<stddef.h>"
 
 #include "case-insensitive-include.h"
 #include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
 
-#include "../Output/./case-insensitive-include.h"
-#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
-#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
-// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../Output/.//case-insensitive-include.h"
+#include "../Output/.//Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:51}:"\"../Output/.//case-insensitive-include.h\""
+#include "../output/.//case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:51}:"\"../Output/.//case-insensitive-include.h\""
 
 #include "apath/.././case-insensitive-include.h"
 #include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
 #include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
 
 #include "../Output/./apath/.././case-insensitive-include.h"
 #include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(
 #include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{non-portable path}}
 // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
 
 #include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.

llvm/include/llvm/Support/Path.h

 ///
 /// Iteration examples. Each component is separated by ',':
 /// @code
 ///   /          => /
 ///   /foo       => /,foo
 ///   foo/       => foo,.
 ///   /foo/bar   => /,foo,bar
 ///   ../        => ..,.
-///   C:\foo\bar => C:,/,foo,bar
+///   C:\foo\bar => C:,\,foo,bar
 /// @endcode
 class const_iterator
     : public iterator_facade_base<const_iterator, std::input_iterator_tag,
                                   const StringRef> {
   StringRef Path;          ///< The entire path.
   StringRef Component;     ///< The current component. Not necessarily in Path.
   size_t    Position = 0;  ///< The iterators current position within Path.
   Style S = Style::native; ///< The path style to use.