Page MenuHomePhabricator

[VFS] Emit an error when a file isn't located in any directory.
ClosedPublic

Authored by vsapsai on Jul 18 2018, 4:26 PM.

Details

Summary

Orphaned files prevent us from building a file system tree and cause the
assertion

Assertion failed: (NewParentE && "Parent entry must exist"), function uniqueOverlayTree, file clang/lib/Basic/VirtualFileSystem.cpp, line 1303.

rdar://problem/28990865

Diff Detail

Repository
rC Clang

Event Timeline

vsapsai created this revision.Jul 18 2018, 4:26 PM
vsapsai added inline comments.Jul 18 2018, 4:27 PM
clang/lib/Basic/VirtualFileSystem.cpp
1416 ↗(On Diff #156176)

Not happy with the error message but didn't come up with anything better. Suggestions are welcome.

Need to double check what tests we have when using relative path names at the root level. I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

bruno added a comment.Jul 19 2018, 6:50 AM

Hi Volodymyr, thanks for improving this.

Need to double check what tests we have when using relative path names at the root level.

Another interesting place to look at is unittests/Basic/VirtualFileSystemTest.cpp

I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

Makes sense. My general impression is that relative paths don't make sense here either, but you can address that in a follow up patch (giving time for any potential counterexample on this).

clang/lib/Basic/VirtualFileSystem.cpp
1416 ↗(On Diff #156176)

What about the message you used in the comment: file entry is at the root level, outside of any directory ?

Hi Volodymyr, thanks for improving this.

Need to double check what tests we have when using relative path names at the root level.

Another interesting place to look at is unittests/Basic/VirtualFileSystemTest.cpp

Thanks for the pointer. Probably I'll move the added test there as it doesn't need full file system interaction and doesn't need to execute entire clang_cc1.

I'd like to make the behavior consistent because a file name is a specific case of relative paths. So far there are no assertions and no errors but file lookup doesn't seem to be working.

Makes sense. My general impression is that relative paths don't make sense here either, but you can address that in a follow up patch (giving time for any potential counterexample on this).

Based on the code, relative paths won't work for file lookup. That is so because in RedirectingFileSystem::lookupPath we convert paths to absolute

if (std::error_code EC = makeAbsolute(Path))
  return EC;
// ...
sys::path::const_iterator Start = sys::path::begin(Path);
sys::path::const_iterator End = sys::path::end(Path);

and textually compare them with VFS entry name.

Having an error for relative paths makes the change cleaner, so I'll include it in this patch.

vsapsai updated this revision to Diff 157311.Jul 25 2018, 11:04 AM
  • Make diagnostics more general, use unit tests.
bruno accepted this revision.Aug 7 2018, 11:11 AM

LGTM

clang/lib/Basic/VirtualFileSystem.cpp
1391 ↗(On Diff #157311)

Reads better :)

This revision is now accepted and ready to land.Aug 7 2018, 11:11 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review, Bruno.