[PATCH] D154130: [lit][clang] Avoid realpath on Windows due to MAX_PATH limitations

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

> @tahonermann are all your comments addressed at this point?

Apologies for the late response; I was on vacation last week.

No. I'm still skeptical that there is ever a desire to expand substitute drives.



================
Comment at: clang/test/Lexer/case-insensitive-include-win.c:5-9
+// Note: We must use the real path here, because the logic to detect case
+// mismatch relies on resolving the real path and checking that casing differs.
+// If we use %t and we are on a substitute drive S: mapping to C:\subst,
+// then we will compare "S:\test.dir\FOO.h" to "C:\subst\test.dir\foo.h"
+// and avoid emitting the diagnostic because the structure is different.
----------------
MrTrillian wrote:
> tahonermann wrote:
> > MrTrillian wrote:
> > > tahonermann wrote:
> > > > Hmm, is it really desirable or necessary that the case mismatch logic resolve substituted drives? I wouldn't think so. This seems like another case where our common canonicalization would be desired.
> > > I think it is necessary as I don't see how else we can retrieve the original casing of the file path. Merely making the path absolute would not do that. Searching for solutions on the internet finds ideas that are worse like converting to a short path then back to a long path or rebuilding the path one component at a time with a series directory listing requests.
> > The warning this test checks for is diagnosed in `Preprocessor::HandleHeaderIncludeOrImport()`; search for `pp_nonportable_path` and/or `pp_nonportable_system_path`. I believe this warning will be issued if any component of the path does not match the case of the include directive. Since the file name differs in case, unless there is a bug in handling of the file name, this test isn't sensitive to case mismatches in `%t`.
> > 
> > From a user point of view, resolving a substitute drive doesn't seem desirableto me with respect to determining whether a non-portable path is being used; I don't think the behavior should be dependent on whether a substitute drive is being used or what its target is.
> @tahonermann I think the code is working by design and it would be an unrelated change to modify its logic. See `trySimplifyPath` in `PPDirectives.cpp`:
> 
> ```
>   // Below is a best-effort to handle ".." in paths. It is admittedly
>   // not 100% correct in the presence of symlinks.
> 
>         // 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.
> ```
> 
> The test was previously implicitly requiring getting the realpath, and it worked because `lit.py`'s logic of doing that. Now that requirement is explicit in the test.
I'm still not following here. Are you saying that `trySimplifyPath()` will replace substitute drives? If so, that doesn't sound desirable and I would expect it to be problematic for your use case. I think we should follow this up ... somewhere (now that this review is closed).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154130/new/

https://reviews.llvm.org/D154130