[clang][lex] Fix non-portability diagnostics with absolute path #74782
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
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
|
@llvm/pr-subscribers-clang Author: Jan Svoboda (jansvoboda11) ChangesThe existing code incorrectly assumes that rdar://91172342 Full diff: https://github.com/llvm/llvm-project/pull/74782.diff 2 Files Affected:
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"
|
jansvoboda11
force-pushed
the
users/jansvoboda11/absolute-case-mismatch
branch
from
6dc0ca6 to
6ab18ed
Compare
cachemeifyoucan
approved these changes
Dec 7, 2023
|
My first version of the test put backslashes into the source file (i.e. Interestingly, that prevented Clang to diagnose the case mismatch. That's because in Note that the diagnostic won't suggest fixing up the separators themselves, since those get inherited from the in-source spelling, not from the real path. |
|
Ping. |
cachemeifyoucan
approved these changes
Dec 18, 2023
jansvoboda11
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Dec 18, 2023
…#74782) 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. This patch also fixes a bug on Windows that would prevent the diagnostic being triggered due to separator mismatch. rdar://91172342 (cherry picked from commit f0691bc)
jansvoboda11
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Dec 19, 2023
…mismatch-fix [clang][lex] Fix non-portability diagnostics with absolute path (llvm#74782)
|
492>Failed Tests (1): I built with "clang", "X86" and "AArch64" under Windows 11 and VS2022 community edition |
|
Thanks for reporting @jrmwng, I'll look into it. |
I wasn't able to reproduce locally on Windows 10. Can you please share detailed reproduction steps and any other config that may be relevant? It seems your realpath to the LLVM repo is |
|
@jansvoboda11 One thing is special. The path |
|
@jansvoboda11 it should be the following steps. (I try to repeat it now)
|
|
@jansvoboda11 the same error using the above steps |
|
Thank you. I think the symlink is most likely the culprit. I'll be able to confirm on my Windows machine tomorrow. For now, I have a speculative fix: #76985 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The existing code incorrectly assumes that
Pathcan 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