# Projects

User does not belong to any projects.

# User Details

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

# Fri, Aug 5

Fri, Aug 5, 12:06 PM · Restricted Project, Restricted Project

# Wed, Aug 3

Wed, Aug 3, 1:01 PM · Restricted Project, Restricted Project

# Tue, Aug 2

Tue, Aug 2, 1:33 PM · Restricted Project, Restricted Project

Thanks 🙇

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

# Mon, Aug 1

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

LGTM, thanks for doing this 🙇

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

# Jun 13 2022

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

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
Jun 13 2022, 11:15 AM · Restricted Project, Restricted Project
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
Jun 13 2022, 10:55 AM · Restricted Project, Restricted Project
Jun 13 2022, 10:54 AM · Restricted Project, Restricted Project
Jun 13 2022, 10:01 AM · Restricted Project, Restricted Project, Restricted Project
Jun 13 2022, 9:42 AM · Restricted Project, Restricted Project

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

# May 9 2022

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
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

Shadowing of FE almost tripped me up there 😅

Apr 15 2022, 9:18 AM · Restricted Project, Restricted Project
Apr 15 2022, 9:16 AM · Restricted Project, Restricted Project
Apr 15 2022, 9:08 AM · Restricted Project, Restricted Project
Apr 15 2022, 9:07 AM · Restricted Project, Restricted Project

# Apr 14 2022

Apr 14 2022, 9:42 AM · Restricted Project, Restricted Project
Apr 14 2022, 9:39 AM · Restricted Project, Restricted Project
Apr 14 2022, 9:39 AM · Restricted Project, Restricted Project
Apr 14 2022, 9:36 AM · Restricted Project, Restricted Project
Apr 14 2022, 9:12 AM · Restricted Project, Restricted Project

Thanks for doing all these Jan!

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

# Apr 12 2022

Thanks Jan :)!

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

# Apr 11 2022

[VFS] RedirectingFileSystem only replace path if not already mapped
Apr 11 2022, 2:53 PM · Restricted Project, Restricted Project, Restricted Project
Apr 11 2022, 2:53 PM · Restricted Project, Restricted Project, Restricted Project

# Apr 8 2022

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

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
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
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
Revert "[VFS] RedirectingFileSystem only replace path if not already mapped"
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project
Apr 5 2022, 2:25 PM · Restricted Project, Restricted Project, Restricted Project

# Apr 4 2022

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
Apr 4 2022, 5:20 PM · Restricted Project, Restricted Project, Restricted Project
Apr 4 2022, 5:19 PM · Restricted Project, Restricted Project, Restricted Project
Apr 4 2022, 5:19 PM · Restricted Project, Restricted Project, Restricted Project

# Mar 30 2022

[VFS] RedirectingFileSystem only replace path if not already mapped
Mar 30 2022, 11:56 AM · Restricted Project, Restricted Project, Restricted Project
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
Mar 29 2022, 12:30 PM · Restricted Project, Restricted Project, Restricted Project

# Mar 28 2022

Mar 28 2022, 6:25 PM · Restricted Project, Restricted Project, Restricted Project
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

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

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

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

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

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
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
Mar 15 2022, 3:49 PM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project

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

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.

Mar 15 2022, 3:19 PM · Restricted Project, Restricted Project

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

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

• 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.

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
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
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

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

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

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

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.

Mar 11 2022, 3:35 PM · Restricted Project, Restricted Project, Restricted Project

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).

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

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