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

[Tristan Labelle via Phabricator via cfe-commits]

MrTrillian added inline comments.


================
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.
----------------
tahonermann wrote:
> 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).
@tahonermann . `trySimplifyPath()` does not replace substitute drives. It's a best-effort attempt to see if the included path mismatches the real file path only by case. It explicitly bails out without diagnostics if it finds that the included path has a different shape from the real file path, which will happen if the included path is on a substitute drive. It has to compare with the real file path because this is the only reasonable way to get the original path casing.

It was already the case that this diagnostic bailed out in the presence of symbolic links, so there are no behavioral differences. I needed to update the test because previously `lit.py` would enforce that `%t` was a real path, and now it doesn't, which means that we would hit the "bail out" code path and not produce the diagnostic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154130