Page MenuHomePhabricator

[VFS] Add print/dump to the whole FileSystem hierarchy
ClosedPublic

Authored by bnbarham on Mar 10 2022, 3:31 PM.

Details

Summary

For now most are implemented by printing out the name of the filesystem,
but this can be expanded in the future. Only OverlayFileSystem and
RedirectingFileSystem are properly implemented in this patch.

  • OverlayFileSystem: Prints each filesystem in the order that any operations are actually run on them. Optionally prints recursively.
  • RedirectingFileSystem: Prints out all mappings, as well as the ExternalFS. Most of this was already implemented other than the handling for the DirectoryRemap case and to actually print out the mapping.

Each FS should implement printImpl rather than print, where the
latter just fowards to the former. This is to avoid spreading the
default arguments through to the subclasses (where we may miss updating
in the future).

Diff Detail

Event Timeline

bnbarham created this revision.Mar 10 2022, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 3:31 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
bnbarham requested review of this revision.Mar 10 2022, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 3:31 PM
bnbarham added a comment.EditedMar 10 2022, 3:41 PM

This and https://reviews.llvm.org/D121423 can be committed by themselves, but https://reviews.llvm.org/D121424, https://reviews.llvm.org/D121425, and https://reviews.llvm.org/D121426 need to be pushed together - they're split to make reviewing easier but tests will fail until all are in.

EDIT: And I've failed at my split already. overlays_range is in a later commit :(

bnbarham updated this revision to Diff 414520.Mar 10 2022, 3:50 PM

Added overlays_range to this patch.

This is awesome! Maybe eventually we'll want more details about some of these but this is enough to be useful.

A few comments inline, but besides the rename I suggested from dump to print, up to you whether to address now or later. It'd also be nice to unit test print() for OverlayFileSystem; in which case you might want the extra flags I suggest inline.

llvm/include/llvm/Support/VirtualFileSystem.h
310

Usually, the dump that takes a raw_ostream is called print(). Can we follow that here too?

I also think it'd be useful to have it take a second argument for whether to print recursively. A bit awkward with virtual functions, but you can make it work nicely with indirection:

  void print(raw_ostream &OS, bool PrintContents = true, bool PrintRecursively = false) {
    printImpl(OS, PrintRecursively);
  }
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
  LLVM_DUMP_METHOD void dump() const; // { print(dbgs(), /*PrintContents=*/true, /*PrintRecursively=*/true); }
#endif

protected:
  virtual void printImpl(raw_ostream &OS, bool, bool) {}
947–952

I'd suggest renaming both of these in a prep commit (you probably don't need pre-commit review for that... as long as tests pass):

  • print()
  • printEntry()

and reserve dump() for when there's no argument. I think this better matches usage elsewhere in LLVM.

llvm/lib/Support/VirtualFileSystem.cpp
155

I don't think you need to repeat LLVM_DUMP_METHOD here. Either header or source is sufficient.

482–487

I'd suggest (with the new flags I suggested):

void OverlayFileSystem::printImpl(raw_ostream &OS, bool PrintContents, bool PrintRecursively) const {
  OS << "OverlayFileSystem:";
  if (!PrintContents) {
    OS << "\n";
    return;
  }

  for (auto FS : enumerate(overlays_range())) {
    OS << FS.index() << ") ";
    FS.value()->print(OS, /*PrintContents=*/PrintRecursively, PrintRecursively);
    OS << "\n";
  }
}

... or something... maybe the flags aren't quite right, but the idea is that print(OS, true, false) would print:

OverlayFileSystem:
FS1

without printing the contents of FS1.

Also makes me feel like an Indent would be useful.

bnbarham added a comment.EditedMar 11 2022, 2:12 PM

This is awesome! Maybe eventually we'll want more details about some of these but this is enough to be useful.

I almost implemented a print for InMemoryFileSystem but figured we could do that later if it becomes useful. The things I added were mostly what I found useful while debugging the rest of the changes here.

A few comments inline, but besides the rename I suggested from dump to print, up to you whether to address now or later. It'd also be nice to unit test print() for OverlayFileSystem; in which case you might want the extra flags I suggest inline.

Thanks for the review! All great suggestions that I think would be worth getting in now - they shouldn't take too long.

bnbarham updated this revision to Diff 415214.Mar 14 2022, 1:58 PM
bnbarham retitled this revision from [VFS] Add dump to the whole FileSystem hierarchy to [VFS] Add print/dump to the whole FileSystem hierarchy.
bnbarham edited the summary of this revision. (Show Details)

Address review comments

bnbarham marked 4 inline comments as done.Mar 14 2022, 3:05 PM
bnbarham updated this revision to Diff 415243.Mar 14 2022, 3:08 PM

Forgot to update dump (have a release build locally)

dexonsmith accepted this revision.Mar 15 2022, 11:10 AM

LGTM, with a couple of suggestions inline.

llvm/include/llvm/Support/VirtualFileSystem.h
321

Maybe worth an explanation in the commit message why print()'s virtual behaviour is split out to printImpl(); up to you though.

402

For this const/non-const pair you might skip the blank link between them.

This revision is now accepted and ready to land.Mar 15 2022, 11:10 AM
bnbarham updated this revision to Diff 415606.Mar 15 2022, 3:19 PM
bnbarham edited the summary of this revision. (Show Details)

Final review comments and small addition

Added "UseExternalName(s)" to the RedirectingFileSystem output as I keep putting that in our small
code snippets but realised it wasn't actually in there :).

bnbarham updated this revision to Diff 415969.Mar 16 2022, 2:05 PM
bnbarham edited the summary of this revision. (Show Details)

Slight description update

bnbarham updated this revision to Diff 416053.Mar 16 2022, 8:14 PM

Remove RedirectingFileSystem::dump (got re-added in the rebase)

This revision was landed with ongoing or failed builds.Mar 17 2022, 1:03 PM
This revision was automatically updated to reflect the committed changes.