[clang] [clang][lex] Fix non-portability diagnostics with absolute path (PR #74782)
Jan Svoboda via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 11 11:40:02 PST 2023
https://github.com/jansvoboda11 updated https://github.com/llvm/llvm-project/pull/74782
>From 6ab18edae7b86ca216848b7fcaff5e58fb3e186c Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 7 Dec 2023 15:15:16 -0800
Subject: [PATCH 1/5] [clang][lex] Fix non-portability diagnostics with
absolute path
The existing code incorrectly assumes that `Path` can be empty. It can't, it always contains at least `<` or `"`. On Unix, this patch fixes an incorrect diagnostics that instead of `"/Users/blah"` suggested `"Userss/blah"`. In assert builds, this would outright crash.
rdar://91172342
---
clang/lib/Lex/PPDirectives.cpp | 22 ++++++++++++-------
.../Lexer/case-insensitive-include-absolute.c | 13 +++++++++++
2 files changed, 27 insertions(+), 8 deletions(-)
create mode 100644 clang/test/Lexer/case-insensitive-include-absolute.c
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 956e2276f25b71..576b3c59253539 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -2466,15 +2466,21 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
// 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])))
+ if (Component.size() == 1 && IsSep(Component[0])) {
+ // Note: Path always contains at least '<' or '"'.
+ if (Path.size() == 1) {
+ // 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.
+ } else {
+ // 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.
+ continue;
+ }
+ } else {
Path.append(Component);
- else if (!Path.empty())
- continue;
+ }
// Append the separator(s) the user used, or the close quote
if (Path.size() > NameWithoriginalSlashes.size()) {
diff --git a/clang/test/Lexer/case-insensitive-include-absolute.c b/clang/test/Lexer/case-insensitive-include-absolute.c
new file mode 100644
index 00000000000000..48f3d59421bd23
--- /dev/null
+++ b/clang/test/Lexer/case-insensitive-include-absolute.c
@@ -0,0 +1,13 @@
+// REQUIRES: case-insensitive-filesystem
+
+// RUN: rm -rf %t && split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/tu.c.in > %t/tu.c
+// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s --DPREFIX=%t
+
+//--- header.h
+//--- tu.c.in
+#import "DIR/Header.h"
+// CHECK: tu.c:1:9: warning: non-portable path to file '"[[PREFIX]]/header.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
+// CHECK-NEXT: 1 | #import "[[PREFIX]]/Header.h"
+// CHECK-NEXT: | ^~~~~~~~~~~~~~~~~~~~~
+// CHECK-NEXT: | "[[PREFIX]]/header.h"
>From 7437975205d4b6642d0439f6d5fa35204a88f67c Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 7 Dec 2023 21:43:55 -0800
Subject: [PATCH 2/5] Fix test on Windows
---
clang/test/Lexer/case-insensitive-include-absolute.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/clang/test/Lexer/case-insensitive-include-absolute.c b/clang/test/Lexer/case-insensitive-include-absolute.c
index 48f3d59421bd23..da6de6acfb07c7 100644
--- a/clang/test/Lexer/case-insensitive-include-absolute.c
+++ b/clang/test/Lexer/case-insensitive-include-absolute.c
@@ -1,13 +1,13 @@
// REQUIRES: case-insensitive-filesystem
// RUN: rm -rf %t && split-file %s %t
-// RUN: sed "s|DIR|%/t|g" %t/tu.c.in > %t/tu.c
-// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s --DPREFIX=%t
+// RUN: sed "s|DIR|%t|g" %t/tu.c.in > %t/tu.c
+// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s --DDIR=%t
//--- header.h
//--- tu.c.in
#import "DIR/Header.h"
-// CHECK: tu.c:1:9: warning: non-portable path to file '"[[PREFIX]]/header.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
-// CHECK-NEXT: 1 | #import "[[PREFIX]]/Header.h"
-// CHECK-NEXT: | ^~~~~~~~~~~~~~~~~~~~~
-// CHECK-NEXT: | "[[PREFIX]]/header.h"
+// CHECK: tu.c:1:9: warning: non-portable path to file '"[[DIR]]/header.h"'; specified path differs in case from file name on disk [-Wnonportable-include-path]
+// CHECK-NEXT: 1 | #import "[[DIR]]/Header.h"
+// CHECK-NEXT: | ^~~~~~~~~~~~~~~~~~
+// CHECK-NEXT: | "[[DIR]]/header.h"
>From 32c0161ab41dfed85765f5889cf79b470a8c2cb2 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Thu, 7 Dec 2023 22:23:51 -0800
Subject: [PATCH 3/5] Fix test on Windows
---
clang/test/Lexer/case-insensitive-include-absolute.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang/test/Lexer/case-insensitive-include-absolute.c b/clang/test/Lexer/case-insensitive-include-absolute.c
index da6de6acfb07c7..6247e4808c7fa2 100644
--- a/clang/test/Lexer/case-insensitive-include-absolute.c
+++ b/clang/test/Lexer/case-insensitive-include-absolute.c
@@ -1,8 +1,8 @@
// REQUIRES: case-insensitive-filesystem
// RUN: rm -rf %t && split-file %s %t
-// RUN: sed "s|DIR|%t|g" %t/tu.c.in > %t/tu.c
-// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s --DDIR=%t
+// RUN: sed "s|DIR|%/t|g" %t/tu.c.in > %t/tu.c
+// RUN: %clang_cc1 -fsyntax-only %t/tu.c 2>&1 | FileCheck %s --DDIR=%/t
//--- header.h
//--- tu.c.in
>From 62e4c5c943bba3a7210bf0c3052e77d40a9757a7 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Fri, 8 Dec 2023 13:27:43 -0800
Subject: [PATCH 4/5] [clang][lex] Don't let mismatching path separators
prevent diagnostic emission
---
clang/lib/Lex/PPDirectives.cpp | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 576b3c59253539..1bf3cbd0ca8880 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -1858,11 +1858,18 @@ static void diagnoseAutoModuleImport(
// path to the file, build a properly-cased replacement in the vector,
// and return true if the replacement should be suggested.
static bool trySimplifyPath(SmallVectorImpl<StringRef> &Components,
- StringRef RealPathName) {
+ StringRef RealPathName,
+ llvm::sys::path::Style Separator) {
auto RealPathComponentIter = llvm::sys::path::rbegin(RealPathName);
auto RealPathComponentEnd = llvm::sys::path::rend(RealPathName);
int Cnt = 0;
bool SuggestReplacement = false;
+
+ auto IsSep = [Separator](StringRef Component) {
+ return Component.size() == 1 &&
+ llvm::sys::path::is_separator(Component[0], Separator);
+ };
+
// Below is a best-effort to handle ".." in paths. It is admittedly
// not 100% correct in the presence of symlinks.
for (auto &Component : llvm::reverse(Components)) {
@@ -1872,10 +1879,11 @@ static bool trySimplifyPath(SmallVectorImpl<StringRef> &Components,
} else if (Cnt) {
--Cnt;
} else if (RealPathComponentIter != RealPathComponentEnd) {
- if (Component != *RealPathComponentIter) {
- // If these path components differ by more than just case, then we
- // may be looking at symlinked paths. Bail on this diagnostic to avoid
- // noisy false positives.
+ if (!IsSep(Component) && !IsSep(*RealPathComponentIter) &&
+ Component != *RealPathComponentIter) {
+ // If these non-separator path components differ by more than just case,
+ // then we may be looking at symlinked paths. Bail on this diagnostic to
+ // avoid noisy false positives.
SuggestReplacement =
RealPathComponentIter->equals_insensitive(Component);
if (!SuggestReplacement)
@@ -2450,7 +2458,7 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
}
#endif
- if (trySimplifyPath(Components, RealPathName)) {
+ if (trySimplifyPath(Components, RealPathName, BackslashStyle)) {
SmallString<128> Path;
Path.reserve(Name.size()+2);
Path.push_back(isAngled ? '<' : '"');
>From 093f6f1ca4ddf0ba91c328617bc8cc85c883d698 Mon Sep 17 00:00:00 2001
From: Jan Svoboda <jan_svoboda at apple.com>
Date: Mon, 11 Dec 2023 11:39:49 -0800
Subject: [PATCH 5/5] Upgrade comment to an assert
---
clang/lib/Lex/PPDirectives.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp
index 1bf3cbd0ca8880..f8f20372647f60 100644
--- a/clang/lib/Lex/PPDirectives.cpp
+++ b/clang/lib/Lex/PPDirectives.cpp
@@ -2475,7 +2475,7 @@ Preprocessor::ImportAction Preprocessor::HandleHeaderIncludeOrImport(
// clang currently cannot process absolute paths in #include lines that
// don't have a drive.
if (Component.size() == 1 && IsSep(Component[0])) {
- // Note: Path always contains at least '<' or '"'.
+ assert(!Path.empty() && "Path always contains at least '<' or quote");
if (Path.size() == 1) {
// If the first entry in Components is a directory separator,
// then the code at the bottom of this loop that keeps the original
More information about the cfe-commits
mailing list