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

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

In D81041#2072396 , @ctetreau wrote:

> After some further investigation, I have come to believe that the root cause 
> of the issue I am seeing is on line 783 of clang/lib/Lex/HeaderSearch.cpp. A 
> path is constructed using string concatenation (dir + '/' + file), which is 
> obviously not robust to the various issues in path construction. A fix had 
> been committed and reverted back in 2015.


When I was fixing portability problems with VFS paths, I started out by trying 
to make paths canonical, and that always led to roadblocks.  A clang developer 
told me that clang philosophically does not try to do any regularization of 
paths.  It turns out that's actually key to making VFS paths viable.  Since 
they can truly consist of a mix of styles, there is no "correct" canonical 
form.  Once I took that approach, most of the VFS portability problems were 
simple to fix without inflicting collateral damage.  So I'm not surprised that 
the 2015 "fix" causes problems.

I'm happy to look at future proposals, and I'll CC myself on that bug report.  
But since you've said you have other priorities now, I'll treat this patch as 
dormant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau abandoned this revision.
ctetreau added a comment.

After some further investigation, I have come to believe that the root cause of 
the issue I am seeing is on line 783 of clang/lib/Lex/HeaderSearch.cpp. A path 
is constructed using string concatenation (dir + '/' + file), which is 
obviously not robust to the various issues in path construction. A fix had been 
committed and reverted back in 2015. Upon restoring the fix, I see that it 
causes several other test failures. Unfortunately, I do not have the bandwidth 
to fully resolve this issue myself, so I have opened a bug for it: 
https://bugs.llvm.org/show_bug.cgi?id=46187

While I still believe the issues I mentioned upthread should probably be 
addressed, they are much less pressing having discovered the root cause of my 
particular issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added a comment.

Please make sure all of the clang VFS tests still pass before submitted this.  
VFS paths are inherently problematic with `llvm::sys::path` because they can 
legitimately be in a hybrid of Windows and Posix styles.  Guessing the style 
from the first separator you see can be misleading, but I don't know whether 
that's ever wrong in this path.

Exactly which assertion is firing?  Is it possible it's the assertion that's 
wrong?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Christopher Tetreault via Phabricator via cfe-commits]

ctetreau created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
ctetreau added reviewers: efriedma, apazos, zzheng, dexonsmith, rnk.

Modify clang::FileManager::FixupRelativePath to use the existing path
separator style, if one exists. Prior to this change, if a path is set
for the FileSystemOpts workingDir that does not match the native path
separator style, this can result in a path with mixed separators. This
was causing an assertion failure in
clang::test/ClangScanDeps/vfsoverlay.cpp on my machine.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81041

Files:
  clang/lib/Basic/FileManager.cpp


Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -423,6 +423,8 @@
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl ) const {
+  using namespace llvm::sys::path;
+
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty()
@@ -430,7 +432,19 @@
 return false;
 
   SmallString<128> NewPath(FileSystemOpts.WorkingDir);
-  llvm::sys::path::append(NewPath, pathRef);
+  Style sepStyle = Style::native;
+
+  for (char c : NewPath) {
+if (is_separator(c, Style::windows)) {
+  sepStyle = Style::windows;
+  break;
+} else if (is_separator(c, Style::posix)) {
+  sepStyle = Style::posix;
+  break;
+}
+  }
+
+  append(NewPath, sepStyle, pathRef);
   path = NewPath;
   return true;
 }


Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -423,6 +423,8 @@
 }
 
 bool FileManager::FixupRelativePath(SmallVectorImpl ) const {
+  using namespace llvm::sys::path;
+
   StringRef pathRef(path.data(), path.size());
 
   if (FileSystemOpts.WorkingDir.empty()
@@ -430,7 +432,19 @@
 return false;
 
   SmallString<128> NewPath(FileSystemOpts.WorkingDir);
-  llvm::sys::path::append(NewPath, pathRef);
+  Style sepStyle = Style::native;
+
+  for (char c : NewPath) {
+if (is_separator(c, Style::windows)) {
+  sepStyle = Style::windows;
+  break;
+} else if (is_separator(c, Style::posix)) {
+  sepStyle = Style::posix;
+  break;
+}
+  }
+
+  append(NewPath, sepStyle, pathRef);
   path = NewPath;
   return true;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits