This is an archive of the discontinued LLVM Phabricator instance.

Some infrastructure work for virtual file system (now on phab)
ClosedPublic

Authored by benlangmuir on Feb 11 2014, 11:20 AM.

Details

Summary

Adding to Phabricator at Manuel's suggestion. Original thread here: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20140210/098787.html.

Summary of changes since the original patch:

  • Since the symlink issue that Dmitri pointed out is going to be a bigger fix, I've pulled all the OverlayFileSystem bits out of this patch. I was hoping to start getting some testing for a half-baked overlay fs, but this issue convinced me it wasn't worth the trouble.
  • Moved static status queries to be methods of Status
  • Some smaller stuff from the reviews (e.g. using LLVM_OVERRIDE)

Diff Detail

Event Timeline

benlangmuir updated this revision to Unknown Object (????).Feb 11 2014, 2:41 PM
  • Don't use name/realname to differentiate virtual and non-virtual files
rafael added inline comments.Feb 12 2014, 2:21 PM
include/clang/Basic/FileManager.h
122 ↗(On Diff #7006)

Why is the reference count necessary? Given its nature I would expect the FS to outlive the file manger, in which case the FileManager could have just a pointer to the FileSystem.

include/clang/Basic/VirtualFileSystem.h
62 ↗(On Diff #7006)

It might be better to use llvm's names: getType for example.

130 ↗(On Diff #7006)

Please upgrade this to return ErrorOr<Status>. We should really upgrade sys::fs, but at least don't propagate it further.

benlangmuir updated this revision to Unknown Object (????).Feb 13 2014, 2:44 PM
  • Use ErrorOr in places where it makes sense to prevent using an invalid object
  • Dodge the question of how to write the predicate functions by removing them all from AbstractFileSystem, since only the ones in Status are being called for now
  • De-virtualize getBufferForFile(Path) by using getBufferForOpenFile(FD). This is what MemoryBuffer was doing internally anway.
  • To support the previous point, I added a close(FD) method. Right now this requires an ugly #include to get ::close, but I will move this into llvm::sys::fs. For now it is still a net improvement, since the FileManager now doesn't call raw ::close.
  • Rename functions in Status to follow naming conventions
klimek added inline comments.Feb 14 2014, 1:14 AM
lib/Basic/VirtualFileSystem.cpp
62–83 ↗(On Diff #7095)

I'd have expected this->*OpenFile(...) calls in those methods.

benlangmuir added inline comments.Feb 14 2014, 6:50 AM
lib/Basic/VirtualFileSystem.cpp
62–83 ↗(On Diff #7095)

These methods take a file descriptor as a parameter, so the file is already open.

benlangmuir added inline comments.Feb 14 2014, 7:15 AM
lib/Basic/VirtualFileSystem.cpp
62–83 ↗(On Diff #7095)

Oh sorry, I think I misunderstood your comment.

The reason we don't call this->*OpenFile in here is that the FileDescriptor object (when we create one) should give us the AbstractFileSystem that we need to call the method on. If we have a virtual file system composed of several AbstractFileSystems overlaid on top of each other, we shouldn't have to search for which file system owns the open file, since by opening it we already figured that out and can save it in the file descriptor.

benlangmuir updated this revision to Unknown Object (????).Feb 14 2014, 4:41 PM
  • Replace file descriptor with vfs::File object, and perform the rename I promised.
gribozavr added inline comments.Feb 14 2014, 5:15 PM
include/clang/Basic/VirtualFileSystem.h
87–90 ↗(On Diff #7140)

I could be misunderstanding something, but why not call the virtual close() from this destructor?

87–90 ↗(On Diff #7140)

Right, we can not do that. Please disregard this comment.

lib/Basic/VirtualFileSystem.cpp
84 ↗(On Diff #7140)

assert(FD >= 0) ?

88 ↗(On Diff #7140)

80 cols?

benlangmuir updated this revision to Unknown Object (????).Feb 14 2014, 5:45 PM
  • Add assert(FD >=0) and fix 80-column violations

Hey all,

Things seem to have quieted down on this patch - does anyone mind if I commit it as is? I'm happy to keep refining it post-commit of course.

doug.gregor accepted this revision.Feb 18 2014, 1:36 PM

I think this is close enough to commit now; we can address any other issues with subsequent commits.

benlangmuir closed this revision.Feb 18 2014, 4:17 PM

Closed by commit rL201618 (authored by @benlangmuir).