[llvm] b9d50bd - Fix pr31836 on Windows too, and correctly handle repeated separators.
Nico Weber via llvm-commits
llvm-commits at lists.llvm.org
Fri May 1 11:17:26 PDT 2020
Author: Nico Weber
Date: 2020-05-01T14:17:01-04:00
New Revision: b9d50bdff211eb806dce5bc42167a6b9c0cac6e6
URL: https://github.com/llvm/llvm-project/commit/b9d50bdff211eb806dce5bc42167a6b9c0cac6e6
DIFF: https://github.com/llvm/llvm-project/commit/b9d50bdff211eb806dce5bc42167a6b9c0cac6e6.diff
LOG: Fix pr31836 on Windows too, and correctly handle repeated separators.
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]
^
Differential Revision: https://reviews.llvm.org/D79223
Added:
Modified:
clang/lib/Lex/PPDirectives.cpp
clang/test/Lexer/case-insensitive-include-ms.c
clang/test/Lexer/case-insensitive-include-pr31836.sh
clang/test/Lexer/case-insensitive-include.c
llvm/include/llvm/Support/Path.h
llvm/unittests/Support/Path.cpp
Removed:
################################################################################
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 03bd36bfe267..8b1d1fb320e7 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -1915,14 +1915,18 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
SourceLocation FilenameLoc = FilenameTok.getLocation();
StringRef LookupFilename = Filename;
-#ifndef _WIN32
+#ifdef _WIN32
+ llvm::sys::path::Style BackslashStyle = llvm::sys::path::Style::windows;
+#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;
+ llvm::sys::path::Style BackslashStyle = llvm::sys::path::Style::posix;
if (LangOpts.MicrosoftExt) {
NormalizedPath = Filename.str();
llvm::sys::path::native(NormalizedPath);
LookupFilename = NormalizedPath;
+ BackslashStyle = llvm::sys::path::Style::windows;
}
#endif
@@ -2105,21 +2109,44 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
SmallString<128> Path;
Path.reserve(Name.size()+2);
Path.push_back(isAngled ? '<' : '"');
- bool isLeadingSeparator = llvm::sys::path::is_absolute(Name);
+
+ const auto IsSep = [BackslashStyle](char c) {
+ return llvm::sys::path::is_separator(c, BackslashStyle);
+ };
+
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 ? '>' : '"'));
+ 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, 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;
+ // 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);
}
diff --git a/clang/test/Lexer/case-insensitive-include-ms.c b/clang/test/Lexer/case-insensitive-include-ms.c
index a7101544fe2f..cf14d2530d01 100644
--- a/clang/test/Lexer/case-insensitive-include-ms.c
+++ b/clang/test/Lexer/case-insensitive-include-ms.c
@@ -6,6 +6,8 @@
// 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\""
diff --git a/clang/test/Lexer/case-insensitive-include-pr31836.sh b/clang/test/Lexer/case-insensitive-include-pr31836.sh
index e842badc7f28..b60e6ca6ff2b 100644
--- a/clang/test/Lexer/case-insensitive-include-pr31836.sh
+++ b/clang/test/Lexer/case-insensitive-include-pr31836.sh
@@ -1,5 +1,4 @@
// REQUIRES: case-insensitive-filesystem
-// UNSUPPORTED: system-windows
// RUN: mkdir -p %t
// RUN: touch %t/case-insensitive-include-pr31836.h
diff --git a/clang/test/Lexer/case-insensitive-include.c b/clang/test/Lexer/case-insensitive-include.c
index 099555de5f5c..f469270e0f66 100644
--- a/clang/test/Lexer/case-insensitive-include.c
+++ b/clang/test/Lexer/case-insensitive-include.c
@@ -16,11 +16,11 @@
#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}}
diff --git a/llvm/include/llvm/Support/Path.h b/llvm/include/llvm/Support/Path.h
index 9edd9c4183ee..3b712c00dc70 100644
--- a/llvm/include/llvm/Support/Path.h
+++ b/llvm/include/llvm/Support/Path.h
@@ -47,7 +47,7 @@ enum class Style { windows, posix, native };
/// 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,
diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp
index c3b7230c2366..3030fb2ebb2e 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -1176,6 +1176,7 @@ TEST_F(FileSystemTest, FileMapping) {
}
TEST(Support, NormalizePath) {
+ // Input, Expected Win, Expected Posix
using TestTuple = std::tuple<const char *, const char *, const char *>;
std::vector<TestTuple> Tests;
Tests.emplace_back("a", "a", "a");
More information about the llvm-commits
mailing list