Page MenuHomePhabricator

[VFS] Add property 'fallthrough' that controls fallback to real file system.
ClosedPublic

Authored by vsapsai on Aug 9 2018, 4:00 PM.

Details

Summary

Default property value 'true' preserves current behavior. Value 'false' can be
used to create VFS "root", file system that gives better control over which
files compiler can use during compilation as there are no unpredictable
accesses to real file system.

Non-fallthrough use case changes how we treat multiple VFS overlay
files. Instead of all of them being at the same level just above a real
file system, now they are nested and subsequent overlays can refer to
files in previous overlays.

rdar://problem/39465552

Diff Detail

Repository
rL LLVM

Event Timeline

vsapsai created this revision.Aug 9 2018, 4:00 PM

Known issues:

  • No support for 'fallthrough' in crash reproducer.
  • Current way of working with modules in VFS "root" is clunky and error-prone.

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Rebased on top of trunk and checked that this is still working. Please take a look.

bruno added a comment.Sep 20 2018, 1:29 PM

Hi Volodymyr,

Thanks for working on this, really nice work with the tests! Comments below.

  • No support for 'fallthrough' in crash reproducer.

That'd be nice to have at some point to make sure we never escape into the real fs.

  • Current way of working with modules in VFS "root" is clunky and error-prone.

Why?

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Maybe add FIXMEs for it?

clang/lib/Basic/VirtualFileSystem.cpp
934 ↗(On Diff #160032)

Can you add comments to these new members? Specifically, what's the purpose of IsExternalFSCurrent and how it connects to ExternalFS

940 ↗(On Diff #160032)

Same for these methods

1738 ↗(On Diff #160032)

Is passing ShouldCheckExternalFS really necessary? Why checking the status of the member isn't enough?

2041 ↗(On Diff #160032)

incrementImpl(true/*IsFirstTime*/)

2045 ↗(On Diff #160032)

incrementImpl(false/*IsFirstTime*/)

vsapsai updated this revision to Diff 169849.Oct 16 2018, 10:20 AM
  • Rebase on top of the latest changes in trunk.
  • Address review comments.

I expect diff between diffs to be noisy due to rebase.

vsapsai marked 5 inline comments as done.Oct 16 2018, 10:39 AM
vsapsai added inline comments.
clang/lib/Basic/VirtualFileSystem.cpp
934 ↗(On Diff #160032)

Added comments.

940 ↗(On Diff #160032)

Added comments. Not entirely happy with them, would like to hear opinion of others.

1738 ↗(On Diff #160032)

Not required anymore, simplified the code.

vsapsai marked 3 inline comments as done.Oct 16 2018, 10:43 AM
  • Current way of working with modules in VFS "root" is clunky and error-prone.

Why?

Mostly because when you approach it in a straightforward way, you cannot read written modules because they aren't in VFS and "rootness" correctly prevents you from reading random files. You can add -fdisable-module-hash as I've done in the test but it has its own downsides when used not in a test but in a real project.

Also I don't like that VFSFromYamlDirIterImpl is similar to overlay iterator but doesn't share code. At the moment I think it's not worth it to rework those abstractions to make more code reusable. If you've noticed problems with those iterators before, maybe it makes sense to try to find a better approach.

Maybe add FIXMEs for it?

Good idea, added.

bruno accepted this revision.Oct 23 2018, 11:28 AM

LGTM with some minor changes.

llvm/lib/Support/VirtualFileSystem.cpp
1008 ↗(On Diff #169849)

Please use /// instead of // to get doxygen's auto brief.

1010 ↗(On Diff #169849)

Same here

1018 ↗(On Diff #169849)

Same here

1020 ↗(On Diff #169849)

Same here

1022 ↗(On Diff #169849)

Same here

This revision is now accepted and ready to land.Oct 23 2018, 11:28 AM

LGTM with some minor changes.

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

IMO we should have the doxygen comments for any developer that's trying to expand/use/understand the impl class. In my experience the main reasons we stuff things in .cpp files are to optimize compile time and to enable better optimizations within a TU.

I was using // instead of /// on purpose. Class VFSFromYamlDirIterImpl resides entirely in .cpp file and isn't available outside of it. Comments are supposed to cover implementation details and intention, not class interface. That's why I think those comments shouldn't be consumed by Doxygen. Does it make sense or do you think it would be better to have those comments in Doxygen?

IMO we should have the doxygen comments for any developer that's trying to expand/use/understand the impl class. In my experience the main reasons we stuff things in .cpp files are to optimize compile time and to enable better optimizations within a TU.

OK, I'll change to Doxygen-style comments.

vsapsai updated this revision to Diff 171210.Oct 25 2018, 3:51 PM
  • Rebase once again.
  • Use Doxygen comments in some places.
This revision was automatically updated to reflect the committed changes.