From 6ab18edae7b86ca216848b7fcaff5e58fb3e186c Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Thu, 7 Dec 2023 15:15:16 -0800 Subject: [PATCH 1/6] [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 Date: Thu, 7 Dec 2023 21:43:55 -0800 Subject: [PATCH 2/6] 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 Date: Thu, 7 Dec 2023 22:23:51 -0800 Subject: [PATCH 3/6] 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 Date: Fri, 8 Dec 2023 13:27:43 -0800 Subject: [PATCH 4/6] [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 &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 &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 Date: Mon, 11 Dec 2023 11:39:49 -0800 Subject: [PATCH 5/6] 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 From d7bf3fbc154627ebbfa9e033b68483f15d21361e Mon Sep 17 00:00:00 2001 From: Jan Svoboda Date: Mon, 11 Dec 2023 14:52:47 -0800 Subject: [PATCH 6/6] Revert some stylistic changes --- clang/lib/Lex/PPDirectives.cpp | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/clang/lib/Lex/PPDirectives.cpp b/clang/lib/Lex/PPDirectives.cpp index f8f20372647f60..c262ea38bc9f58 100644 --- a/clang/lib/Lex/PPDirectives.cpp +++ b/clang/lib/Lex/PPDirectives.cpp @@ -2474,21 +2474,15 @@ 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 (Component.size() == 1 && IsSep(Component[0])) { - 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 - // 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 { + // 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); - } + else if (Path.size() != 1) + continue; // Append the separator(s) the user used, or the close quote if (Path.size() > NameWithoriginalSlashes.size()) {