[clang] [clang][lex] Fix non-portability diagnostics with absolute path (PR #74782)

Jan Svoboda via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 8 13:27:57 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/4] [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 956e2276f25b7..576b3c5925353 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 0000000000000..48f3d59421bd2
--- /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/4] 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 48f3d59421bd2..da6de6acfb07c 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/4] 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 da6de6acfb07c..6247e4808c7fa 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/4] [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 576b3c5925353..1bf3cbd0ca888 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 ? '<' : '"');



More information about the cfe-commits mailing list