Page MenuHomePhabricator

bnbarham (Ben Barham)
User

Projects

User does not belong to any projects.

User Details

User Since
Oct 7 2020, 6:11 PM (95 w, 4 d)

Recent Activity

Fri, Aug 5

bnbarham accepted D131273: [clang] Fix redirection behaviour for cached FileEntryRef.
Fri, Aug 5, 12:06 PM · Restricted Project, Restricted Project

Wed, Aug 3

bnbarham accepted D131076: [clang][modules] Don't depend on sharing FileManager during module build.
Wed, Aug 3, 1:01 PM · Restricted Project, Restricted Project

Tue, Aug 2

bnbarham accepted D131017: Fix use-after-free in clang-apply-replacements.
Tue, Aug 2, 1:33 PM · Restricted Project, Restricted Project
bnbarham accepted D131004: [clang] Add FileEntryRef::getNameAsRequested().

Thanks 🙇

Tue, Aug 2, 12:37 PM · Restricted Project, Restricted Project

Mon, Aug 1

bnbarham accepted D130935: [clang] Only modify FileEntryRef names that are externally remapped.
Mon, Aug 1, 1:57 PM · Restricted Project, Restricted Project
bnbarham accepted D130934: [clang] Update code that assumes FileEntry::getName is absolute NFC.

LGTM, thanks for doing this 🙇

Mon, Aug 1, 1:52 PM · Restricted Project, Restricted Project, Restricted Project

Jun 13 2022

bnbarham added a comment to D127647: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::{load,lookup}ModuleMap().

The failure here is likely due to the hack in FileManager::getFileRef:

// FIXME: This hack ensures that `getDir()` will use the path that was
// used to lookup this file, even if we found a file by different path
// first. This is required in order to find a module's structure when its
// headers/module map are mapped in the VFS.

So the directory returned from the FileEntry is the *lookup* directory, but the FileEntryRef will be giving back the directory from the external path (if it has been remapped).

Jun 13 2022, 12:39 PM · Restricted Project, Restricted Project
bnbarham added a comment to D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile.

Failing test seems to be because . is turned into the full path at some point. It's possible that this is because getFileRef returns the absolute path (see the massive FIXME there). If that's the case we could fix that by fixing clang-apply-replacements and then change the Status.getName() == Filename to only check whether it's an externally mapped path.

Jun 13 2022, 11:22 AM · Restricted Project, Restricted Project
bnbarham accepted D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile.
Jun 13 2022, 11:15 AM · Restricted Project, Restricted Project
bnbarham accepted D127660: [clang][lex] NFCI: Use DirectoryEntryRef in Preprocessor::MainFileDir.
Jun 13 2022, 11:02 AM · Restricted Project, Restricted Project
bnbarham added inline comments to D127654: [clang] NFCI: Use DirectoryEntryRef in Module::Directory.
Jun 13 2022, 10:59 AM · Restricted Project, Restricted Project
bnbarham accepted D127658: [clang][lex] NFCI: Use FileEntryRef in Sema::CodeCompleteIncludedFile.
Jun 13 2022, 10:55 AM · Restricted Project, Restricted Project
bnbarham accepted D127654: [clang] NFCI: Use DirectoryEntryRef in Module::Directory.
Jun 13 2022, 10:54 AM · Restricted Project, Restricted Project
bnbarham accepted D127651: [clang][lex] NFCI: Use DirectoryEntryRef in ModuleMap::parseModuleMapFile().
Jun 13 2022, 10:01 AM · Restricted Project, Restricted Project, Restricted Project
bnbarham accepted D127648: [clang][lex] NFCI: Use DirectoryEntryRef in ModuleMap::inferFrameworkModule().
Jun 13 2022, 9:42 AM · Restricted Project, Restricted Project
bnbarham accepted D127647: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::{load,lookup}ModuleMap().

Few small comments, LGTM otherwise.

Jun 13 2022, 9:38 AM · Restricted Project, Restricted Project

May 9 2022

bnbarham added inline comments to D125152: [compiler-rt] Add missing arguments to dynamic cflags.
May 9 2022, 10:51 AM · Restricted Project, Restricted Project

May 6 2022

bnbarham requested review of D125152: [compiler-rt] Add missing arguments to dynamic cflags.
May 6 2022, 5:49 PM · Restricted Project, Restricted Project

Apr 22 2022

bnbarham committed rG089b6efefc3d: [Index] Remove reference to `UnresolvedUsingIfExists` (authored by bnbarham).
[Index] Remove reference to `UnresolvedUsingIfExists`
Apr 22 2022, 5:20 PM · Restricted Project, Restricted Project
bnbarham closed D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.
Apr 22 2022, 5:19 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.

Remove line for <unknown> check

Apr 22 2022, 3:16 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.

Update title/message

Apr 22 2022, 2:43 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.

After speaking with Ben, we decided it makes more sense to just remove the reference entirely.

Apr 22 2022, 2:42 PM · Restricted Project, Restricted Project
bnbarham added inline comments to D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.
Apr 22 2022, 11:44 AM · Restricted Project, Restricted Project
bnbarham requested review of D124288: [Index] Remove reference to `UnresolvedUsingIfExists`.
Apr 22 2022, 11:40 AM · Restricted Project, Restricted Project

Apr 15 2022

bnbarham accepted D123856: [clang][lex] NFCI: Use FileEntryRef in ModuleMap::diagnoseHeaderInclusion().

Shadowing of FE almost tripped me up there 😅

Apr 15 2022, 9:18 AM · Restricted Project, Restricted Project
bnbarham accepted D123854: [clang][lex] NFCI: Use DirectoryEntryRef in FrameworkCacheEntry.
Apr 15 2022, 9:16 AM · Restricted Project, Restricted Project
bnbarham accepted D123853: [clang] NFCI: Use DirectoryEntryRef in FrontendAction::BeginSourceFile().
Apr 15 2022, 9:08 AM · Restricted Project, Restricted Project
bnbarham accepted D123771: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*().
Apr 15 2022, 9:07 AM · Restricted Project, Restricted Project

Apr 14 2022

bnbarham accepted D123770: [clang] NFCI: Use FileEntryRef in FileManagerTest.
Apr 14 2022, 9:42 AM · Restricted Project, Restricted Project
bnbarham accepted D123772: [clang][lex] NFC: Use FileEntryRef in PreprocessorLexer::getFileEntry().
Apr 14 2022, 9:39 AM · Restricted Project, Restricted Project
bnbarham accepted D123768: [clang][CodeGen] NFCI: Use FileEntryRef.
Apr 14 2022, 9:39 AM · Restricted Project, Restricted Project
bnbarham added inline comments to D123771: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*().
Apr 14 2022, 9:36 AM · Restricted Project, Restricted Project
bnbarham accepted D123769: [clang] NFCI: Use DirectoryEntryRef in collectIncludePCH.
Apr 14 2022, 9:12 AM · Restricted Project, Restricted Project
bnbarham accepted D123767: [clang][parse] NFCI: Use FileEntryRef in Parser::ParseModuleImport().

Thanks for doing all these Jan!

Apr 14 2022, 9:10 AM · Restricted Project, Restricted Project

Apr 12 2022

bnbarham accepted D123574: [clang][lex] NFCI: Use FileEntryRef in PPCallbacks::InclusionDirective().

Thanks Jan :)!

Apr 12 2022, 11:54 AM · Restricted Project, Restricted Project, Restricted Project

Apr 11 2022

bnbarham committed rGfe2478d44e4f: [VFS] RedirectingFileSystem only replace path if not already mapped (authored by bnbarham).
[VFS] RedirectingFileSystem only replace path if not already mapped
Apr 11 2022, 2:53 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham closed D123398: [VFS] RedirectingFileSystem only replace path if not already mapped.
Apr 11 2022, 2:53 PM · Restricted Project, Restricted Project, Restricted Project

Apr 8 2022

bnbarham added a comment to D123398: [VFS] RedirectingFileSystem only replace path if not already mapped.

Does clang/test/VFS/external-names-multi-overlay.c need to be formatted or is it fine? It uses split-file so I'd really like to avoid the format here (makes it pretty silly).

Apr 8 2022, 12:31 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham abandoned D123104: [Modules] Use looked-up filename when looking for module maps.

Looks like there's more changes required for modulemap-collision.m to actually pass. I'll try figure those out when I have the time.

Apr 8 2022, 9:06 AM · Restricted Project, Restricted Project
bnbarham requested review of D123398: [VFS] RedirectingFileSystem only replace path if not already mapped.
Apr 8 2022, 9:04 AM · Restricted Project, Restricted Project, Restricted Project

Apr 5 2022

bnbarham updated the diff for D123104: [Modules] Use looked-up filename when looking for module maps.

Added a potential plan to remove the FileManager hacks.

Apr 5 2022, 6:16 PM · Restricted Project, Restricted Project
bnbarham added inline comments to D123104: [Modules] Use looked-up filename when looking for module maps.
Apr 5 2022, 6:01 PM · Restricted Project, Restricted Project
bnbarham added a reverting change for rG3fda0edc51fd: [VFS] RedirectingFileSystem only replace path if not already mapped: rGf65b0b5dcfeb: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham committed rGf65b0b5dcfeb: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped" (authored by bnbarham).
Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a reverting change for D122549: [VFS] RedirectingFileSystem only replace path if not already mapped: rGf65b0b5dcfeb: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham closed D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project

Apr 4 2022

bnbarham added a comment to D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".

This broke crash reproducers in very specific circumstances, see https://reviews.llvm.org/D123104. I'll re-commit with adding ExposesExternalVFSPath instead of replacing IsVFSMapped.

Apr 4 2022, 5:26 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham requested review of D123104: [Modules] Use looked-up filename when looking for module maps.
Apr 4 2022, 5:20 PM · Restricted Project, Restricted Project
bnbarham added a reverting change for rG3fda0edc51fd: [VFS] RedirectingFileSystem only replace path if not already mapped: D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 4 2022, 5:20 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham requested review of D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 4 2022, 5:19 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a reverting change for D122549: [VFS] RedirectingFileSystem only replace path if not already mapped: D123103: Revert "[VFS] RedirectingFileSystem only replace path if not already mapped".
Apr 4 2022, 5:19 PM · Restricted Project, Restricted Project, Restricted Project

Mar 30 2022

bnbarham committed rG3fda0edc51fd: [VFS] RedirectingFileSystem only replace path if not already mapped (authored by bnbarham).
[VFS] RedirectingFileSystem only replace path if not already mapped
Mar 30 2022, 11:56 AM · Restricted Project, Restricted Project, Restricted Project
bnbarham closed D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.
Mar 30 2022, 11:56 AM · Restricted Project, Restricted Project, Restricted Project

Mar 29 2022

bnbarham updated the diff for D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.

Keep using the redirection hack for the time being

Mar 29 2022, 12:42 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added inline comments to D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.
Mar 29 2022, 12:30 PM · Restricted Project, Restricted Project, Restricted Project

Mar 28 2022

bnbarham added inline comments to D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.
Mar 28 2022, 6:25 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added inline comments to D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.
Mar 28 2022, 4:15 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham updated the diff for D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.

Rename IsVFSMapped as suggested by Duncan. Update comments.

Mar 28 2022, 4:05 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

Is it just failing on Windows? I wonder (rather speculatively...) whether https://reviews.llvm.org/D121733 would help.

Mar 28 2022, 11:24 AM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.

clang-apply-replacements/relative-paths.cpp is failing, I haven't looked into it but my guess would be that it's from the Status.getName() == Filename -> !Status.IsVFSMapped change. That seems very odd to me.

Mar 28 2022, 11:16 AM · Restricted Project, Restricted Project, Restricted Project

Mar 27 2022

bnbarham requested review of D122549: [VFS] RedirectingFileSystem only replace path if not already mapped.
Mar 27 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project

Mar 18 2022

bnbarham added a comment to D121733: Clean pathnames in FileManager..

I've been somewhat afraid to touch this code because of symlinks. Is that something we need to worry about? Consider this path: root/symlink/../foo.h. remove_dots will turn this into root/foo.h, but if symlink points to some unrelated directory, the .. directory entry points to something other than root. I can't imagine that people rely on this behavior often, but I could be wrong.

Mar 18 2022, 4:00 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121733: Clean pathnames in FileManager..

There's also some others where I wouldn't expect them to be failing in this patch, eg. the ones from / -> {{[/\\]}}.

These are failing because remove_dots (un-intuitively) also changes "foo/bar\\baz" to "foo\\bar\\baz"

Are there tests that we can't just fix to expect either / or \\?

That's what this change is doing (but there is more Winx64 cleanup pending; sorry, I should have sent this as a draft, but I am still learning the ropes here).

Mar 18 2022, 11:51 AM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121733: Clean pathnames in FileManager..

However, FileManager changes sometimes have odd side effects... and it's possible that somewhere in clang relies on having FileManager::getFileRef() return precisely the same path that was requested. Tagging a few other people that have some context... please share your opinions!

Mar 18 2022, 11:13 AM · Restricted Project, Restricted Project, Restricted Project

Mar 17 2022

bnbarham planned changes to D121424: [VFS] Simplify RedirectingFileSystem to perform mapping only.

Blocked on the dependents

Mar 17 2022, 7:04 PM · Restricted Project, Restricted Project
bnbarham planned changes to D121425: [VFS] Add a new getVFSFromYAMLs API.

Blocked on the dependent

Mar 17 2022, 7:02 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham planned changes to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

We had some discussions around the issue of needing to replace the returned path with the original, but that not being possible because of use-external-names: true. I'm going to put up a separate patch that should make it possible to detect when an external path was returned and then not replace in that case. We can then use that here.

Mar 17 2022, 6:47 PM · Restricted Project, Restricted Project
bnbarham committed rG4125524112e0: [VFS] Add print/dump to the whole FileSystem hierarchy (authored by bnbarham).
[VFS] Add print/dump to the whole FileSystem hierarchy
Mar 17 2022, 1:03 PM · Restricted Project
bnbarham closed D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.
Mar 17 2022, 1:03 PM · Restricted Project, Restricted Project

Mar 16 2022

bnbarham updated the diff for D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.

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

Mar 16 2022, 8:14 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.

Slight description update

Mar 16 2022, 2:05 PM · Restricted Project, Restricted Project

Mar 15 2022

bnbarham added a comment to D121425: [VFS] Add a new getVFSFromYAMLs API.

Can you be more detailed about the overall state at the time of cat, which causes it to fail?

Mar 15 2022, 8:41 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121425: [VFS] Add a new getVFSFromYAMLs API.

There's two failing tests with this change:

  • VFSFromYAMLTest.ReturnsExternalPathVFSHit
  • VFSFromYAMLTest.ReturnsInternalPathVFSHit

Apparently we allow relative paths in external-contents *without* specifying overlay-relative: true. In this case the relative paths are resolved based on the CWD at the time of the operation. Do we really want that? I believe this wouldn't have worked prior to https://reviews.llvm.org/D109128 as there was no canonicalising before then as well. @keith was that change intentional? If we want it then it will *require* setting CWD on each FS in OverlayFS, which I was really hoping to avoid.

Mar 15 2022, 5:42 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121425: [VFS] Add a new getVFSFromYAMLs API.

There's two failing tests with this change:

  • VFSFromYAMLTest.ReturnsExternalPathVFSHit
  • VFSFromYAMLTest.ReturnsInternalPathVFSHit
Mar 15 2022, 4:09 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham abandoned D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API.
Mar 15 2022, 3:49 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API.

Merged into D121425 instead.

Mar 15 2022, 3:48 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham updated the diff for D121424: [VFS] Simplify RedirectingFileSystem to perform mapping only.

Re-order to be after D121425

Mar 15 2022, 3:47 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121425: [VFS] Add a new getVFSFromYAMLs API.

Re-order to be before D121424 and merge with D121426

Mar 15 2022, 3:44 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham updated the diff for D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

Pass absolute paths down into contained FS

Mar 15 2022, 3:42 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.

Final review comments and small addition

Mar 15 2022, 3:19 PM · Restricted Project, Restricted Project
bnbarham added a comment to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

Do you really mean real path here (system call to realpath, that sees through symlinks), or just absolute path?

Is it only when RealFileSystem has a CWD? If RealFileSystem has a CWD, then in practice it needs to use absolute paths for calls to sys::fs since the process CWD cannot be trusted. I suspect it's just failing to model the working directory for the paths it returns (which is sometimes hard, in the presence of symlinks). Maybe that's fine for now.

Is this behaviour (overlayfilesystem's behaviour when wrapping a RealFileSystem) a regression/change or is it the same as before?

Mar 15 2022, 1:20 PM · Restricted Project, Restricted Project
bnbarham added a comment to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

If we could resolve the path in OverlayFileSystem then we could just do a status + check that it's a directory on each FS and if so, set CWD in OverlayFileSystem and not any underlying FS.

Maybe we should just do that though with one exception - if the returned path is not the one we gave then *don't* replace the path...? External names is a bit of a hack anyway, so hack for a hack 😆?

Mar 15 2022, 10:41 AM · Restricted Project, Restricted Project

Mar 14 2022

bnbarham added a comment to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.
  • The client's view of the OverlayFS *should* be a single (virtual) filesystem. But without top-level CWD modelling, we can't actually provide that. Consider:
overlay:
  base:
    /file
  memory:
    /dir/

operations:
  cd /dir
  cat ../file

This *should* give the content from /file in base. But even with our proposed changes it's not going to work correctly. I think one way to be correct would be:

  • Find the "first" FS (like CWD?) where getRealPath(dirname(relativepath)) does not fail
  • Then combine that real-path with filename(relativepath), and go through the overlays with that path

... but this would probably be too slow. Not sure if you have ideas on how to fix it; and I don't think it's made worse by this patch (off topic?). Maybe worth adding a FIXME or description somewhere documenting that it doesn't work.

(Regardless, I think your plan should work.)

Mar 14 2022, 3:47 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.

Forgot to update dump (have a release build locally)

Mar 14 2022, 3:08 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121421: [VFS] Add print/dump to the whole FileSystem hierarchy.

Address review comments

Mar 14 2022, 1:58 PM · Restricted Project, Restricted Project
bnbarham committed rG3466b8e23d9c: [Support] Add const to `FileError::getFileName` (authored by bnbarham).
[Support] Add const to `FileError::getFileName`
Mar 14 2022, 11:45 AM · Restricted Project
bnbarham closed D121495: [Support] Add const to `FileError::getFileName`.
Mar 14 2022, 11:45 AM · Restricted Project, Restricted Project
bnbarham committed rGcc63ae42d772: [VFS] Rename `RedirectingFileSystem::dump` to `print` (authored by bnbarham).
[VFS] Rename `RedirectingFileSystem::dump` to `print`
Mar 14 2022, 11:44 AM · Restricted Project
bnbarham closed D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`.
Mar 14 2022, 11:44 AM · Restricted Project, Restricted Project, Restricted Project

Mar 11 2022

bnbarham added inline comments to D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API.
Mar 11 2022, 10:53 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121426: [VFS] Update uses of getVFSFromYAML to use the new getVFSFromYAMLs API.

Includes two test fixes (since chained mappings are no longer allowed)
and adds a new test for multiple overlays.

Are we sure no one needs to support chained mappings? Has there been a clang-dev discourse discussion about it already? Just concerned that some vendor might rely on being able to support this.

Mar 11 2022, 10:52 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121424: [VFS] Simplify RedirectingFileSystem to perform mapping only.

Can you explain more why you don't think it's a good idea in this case?

For complicated changes, it's not uncommon to add the new API (with tests for it), and then do adoption later.

Actually, I wonder if you could even just land these as three separate commits by doing D121424 last:

  • D121425: add new API (causes any created RedirectingFS to be created with these options turned off)
  • D121426: change clang to use it
  • D121424: remove now-unneeded RedirectingFS code and add tests proving that A->B->C case no longer works
Mar 11 2022, 10:31 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`.

Removed LLVM_DUMP_METHOD from .cpp

Mar 11 2022, 4:34 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121424: [VFS] Simplify RedirectingFileSystem to perform mapping only.

In this case:

  • I think there might be a way to re-sequence D121425 ahead of this commit.
    • The new API can be added and unit tested without deleting features from RedirectingFS.
    • There may be some tests that fail until this commit lands; you can either check for the temporary wrong behaviour and fix them in this commit, or shift those tests entirely to this commit if it's too confusing. But it'll help to see what tests suddenly start passing related to the new API.
  • Maybe this commit could be made runtime-configurable (OverlayFS constructor, off-by-default, only used in the new API to start, then the old behaviour removed later), to land it ahead of D121426? Maybe that's too complicated; just pointing out the flexibility.
    • Else, I'd suggest merging D121426 with this one. The changes don't touch the same files and it's not a massive patch so I don't think it'll confuse the review much.
Mar 11 2022, 4:15 PM · Restricted Project, Restricted Project
bnbarham added a comment to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

I've thought of another concern: the "recursively call CWD" behaviour can result in CWD being called multiple times on a filesystem instance.

Mar 11 2022, 4:05 PM · Restricted Project, Restricted Project
bnbarham updated the diff for D121494: [VFS] Rename `RedirectingFileSystem::dump` to `print`.

Added VFS.cpp, removed implementation from header

Mar 11 2022, 3:35 PM · Restricted Project, Restricted Project, Restricted Project
bnbarham added a comment to D121424: [VFS] Simplify RedirectingFileSystem to perform mapping only.

This allowed:

  • Overlay paths to be dependent on previous -ivfsoverlay, ie. a second -ivfsoverlay could specify a virtual path for its overlay if that path is in the first overlay.
  • Paths from one overlay to affect those in an earlier, ie. if the right-most overlay specified a mapping from A -> B and the left-most a mapping from B -> C, A could now map to C.
  • The real filesystem to be skipped, ie. all specified paths have to be contained in an overlay (which would still map to the real filesystem).

FileSystem also had a get/set added for the current working directory
to allow for filesystem-specific CWD rather than a process-wide CWD,
further complicating the RedirectingFileSystem implementation.

It's not clear from the description what the plan is for the RedirectingFileSystem's CWD. It sounds like you're suggesting dropping it. If so, what happens if the redirecting filesystem remaps things to a new directory /new/dir that doesn't exist in the underlying filesystem, and the client calls cd /new/dir?

Mar 11 2022, 3:26 PM · Restricted Project, Restricted Project
bnbarham added a comment to D121423: [VFS] OverlayFileSystem should consistently use the last-added FS first.

I think my explanation was a bit muddled.

  • Before this patch:
    • cd /c would have returned "directory not found". Clients think we're in /.
    • cat a/x will give content aligning with /a/x.
    • This seems correct.
  • After this patch:
    • cd /c has no error. Clients think we're in /c.
    • cat a/x will give content aligning with /a/x/.
    • This seems incorrect. Since the cd /c succeeded, we should get /c/a/x.
Mar 11 2022, 3:17 PM · Restricted Project, Restricted Project
bnbarham added a comment to D121495: [Support] Add const to `FileError::getFileName`.

@dexonsmith added you to reviewers since you were automatically added as a subscriber :)

Mar 11 2022, 2:39 PM · Restricted Project, Restricted Project