[PATCH] D51641: [VFS] Cache the current working directory for the real FS.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D51641#1351283 <https://reviews.llvm.org/D51641#1351283>, @labath wrote:

> I wholeheartedly support an openat(2) based VFS, as the current one falls short of the expectations you have of it and is pretty broken right now.


Let me bait-and-switch then... D56545 <https://reviews.llvm.org/D56545> adds a VFS that **emulates** `openat()` using path manipulation.

Since `openat` isn't available everywhere, to use it we'd need:

1. `openat`-like implementation of path ops
2. a fallback path-based implementation
3. new APIs to expose this from `llvm::sys::fs::`
4. a factory for the `openat`-like VFS

but I think it's less invasive and controversial to start with just 2 and 4.

> In fact, it still happens now, because the VFS is so bad at having a local CWD. So, the only reason we actually discovered this was because  VFS->getCurrentWorkingDirectory returned an empty string if a it's CWD has never been set. This later translated to a null pointer and a crash.

(Hmm, `RealFileSystem::getCurrentWorkingDirectory shouldn't return empty unless `fs::current_path()` does. But certainly here be dragons)

> So it almost sounds to me like we should have two implementations of the VFS. A "real" one, which shared the CWD with the OS, and an "almost real" one, which has a local CWD. Another position would be to just say that lldb was wrong to use the VFS in the first place, as it does not promise we want (and it should use a different abstraction instead). That view would be supported by the fact that there are other interface disconnects relating to the use of VFS in lldb (FILE* and friends). However, that's something you and Jonas will have to figure out (I'm just the peanut gallery here :P).

Yeah - I will confess to finding LLDB's use of VFS difficult - doing some but not all operations through the VFS seems to constrains its implementation and makes the abstraction leakier.
But practically, I think you're right. And the default should probably stay as-is, at least until we have `openat()` on most platforms, or multithreading becomes more pervasive in LLVM-land.
D56545 <https://reviews.llvm.org/D56545> rips out the cache for the default version, and explicitly guarantees synchronization with the OS workdir.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51641/new/

https://reviews.llvm.org/D51641