[PATCH] D81041: Use existing path sep style in clang::FileManager::FixupRelativePath

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau added a comment.

@amccarth

The assert that I am getting is at line 1701 of VirtualFileSystem.cpp:

  assert(!isTraversalComponent(*Start) &&
           !isTraversalComponent(From->getName()) &&
           "Paths should not contain traversal components");

path is `C:/[path]/[to]/[build]/tools/clang/test/ClangScanDeps/Output/vfsoverlay.cpp.tmp.dir\\.` (notice the trailing windows path separator)

The path is canonicalized on line 1688 of VirtualFileSystem.cpp in lookupPath. The canonicalization fails to remove the trailing '.' because it detects (using the same method I am using in my patch) that the file path has posix separators and it sees "vfsoverlay.cpp.tmp.dir\\." as one path component. Perhaps the real fix is to make canonicalize handle mixed separators? I'm guessing that canonicalize is implemented as it is for performance reasons? This comment in the body of canonicalize "Explicitly specifying the path style prevents the direction of the slashes from changing" leads me to believe that it is desirable that the path separators not be changed, so a new implementation would need to walk the entire string and fail if mixed separators are encountered, or llvm::sys::path::remove_dots needs to also be changed to have a mixed-separators version.

The test clang::test/ClangScanDeps/vfsoverlay.cpp causes this on my machine. It is unfortunate that for reasons beyond my comprehensions, I often get test failures on my machine that are not caught by the builders, so I have no idea why the builders don't see this. Perhaps they aren't running this test?

Regardless, I think that using even a flawed method to detect what the default path separator should be might be better than just assuming native. Either it will be correct, or the path already had mixed separators, and it doesn't actually matter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81041