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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir accepted this revision.
benlangmuir added a comment.
This revision is now accepted and ready to land.

LGTM other than my suggestion to add a comment. @tahonermann are all your comments addressed at this point?



================
Comment at: clang/lib/Basic/FileManager.cpp:663
+    } else {
+      llvm::sys::path::remove_dots(AbsPathBuf, /*remove_dot_dot=*/true);
+      CanonicalName = AbsPathBuf.str().copy(CanonicalNameStorage);
----------------
MrTrillian wrote:
> benlangmuir wrote:
> > MrTrillian wrote:
> > > benlangmuir wrote:
> > > > Removing .. can change where the path points in the presence of symlinks; is this needed?
> > > > Removing .. can change where the path points in the presence of symlinks; is this needed?
> > > 
> > > @benlangmuir That's true and not ideal, but `makeAbsolute` will not resolve `/./` or `/../` in paths, so it's not a canonicalization and some tests were failing because of that. One alternative would be to use `makeAbsolute` + `remove_dots` on Windows (where removing dot dots is semantically correct) and `getRealPath` on Unix, like I do in lit. Suggestions?
> > Wouldn't removing .. have the same issue with symlinks on Windows? I know symlinks are less common there, but it's not clear to me why it would be correct.  I guess you could also check if the paths resolve to the same file after removing ..
> > 
> > 
> @benlangmuir Windows simplifies `\..\` in paths before starting to walk the filesystem. https://superuser.com/questions/1502360/different-behavior-of-double-dots-and-symlinks-in-windows-and-unix
> 
> If I detected that the path didn't resolve to the same file after removing `..`, what would I do?
> 
> I think the current logic will only end up using the `else` branch on Windows. At least I can't think of a scenario where the root would change from using realpath on unix.
Thanks, I wasn't aware of that subtlety! I suggest we add a comment about that here and also mention (or assert) that this is only reachable on Windows.


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

https://reviews.llvm.org/D154130