[PATCH] D117650: [llvm][vfs] Implement in-memory symlinks

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

In D117650#3256232 <https://reviews.llvm.org/D117650#3256232>, @dexonsmith wrote:

> @sammccall, you've had objections in the past to exposing symbolic links in the `vfs::FileSystem` API, but this patch doesn't do that. It's just modelling symlink behaviour in an `InMemoryFileSystem`. Any concerns?

No, this seems fine to me. And implementation seems solid :-)

(IIRC my previous concerns weren't about VFS->VFS symlinks, but rather ability to "resolve" a VFS file to a non-virtual file in order to open it through native APIs. It's possible I misunderstood that proposal of course)

In principle I don't see a problem with exposing symlinks in the VFS layer either, but I guess it'd be really complicated to get right. (e.g. ensure handling symlinks externally vs within the VFS matches, conceptual problems with overlay filesystems, etc)



================
Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:493
+  /// directory for \param To isn't automatically created.
+  bool addSymbolicLink(const Twine &From, const Twine &To,
+                       time_t ModificationTime, Optional<uint32_t> User = None,
----------------
FWIW I find `From`/`to` confusing, because I had to rewire my brain to think of `ln` as a variant of `cp` in order to remember which order the args go in :-)

I think `NewLink`/`Target` would be much clearer, but there's value in being consistent with the existing `addHardLink` too, so up to you.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:902
+                   detail::InMemoryDirectory *RootDir, const Twine &P,
+                   bool FollowSymlinks, size_t SymlinkDepth = 0) {
   SmallString<128> Path;
----------------
FollowFinalSymlink?
It's wordy, but it's not clear to me what the behavior is if you look up "link/link/file" with FollowSymlinks=false. Also the plural is a bit misleading.

It might be worth adding a comment here with the most important contract: if Follow is true, then it will never return a symlink node.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:983
   // before. Resolved ToPath must be a File.
   if (!ToNode || FromNode || !isa<detail::InMemoryFile>(*ToNode))
     return false;
----------------
This fails if ToNode is a symbolic link. Linux allows hardlinks to symlinks, apparently macOS resolves the symlink and hardlinks to the target, posix allows either (implementation-defined) but doesn't allow it to just fail.

This isn't posix so you can do whatever you like, but I think the choice deserves a comment.


================
Comment at: llvm/lib/Support/VirtualFileSystem.cpp:1056
       case detail::IME_HardLink:
+      case detail::IME_SymbolicLink:
         Type = sys::fs::file_type::regular_file;
----------------
This doesn't look correct to me, if the symlink points to a directory we'll claim it's a file and then operations on it will fail.

Since the VFS interface doesn't know about symlinks, it's probably best to resolve the symlink (chain) here and report the target. But returning symlink_file might be ok...

If you resolve and it dangles, you have to choose between reporting regular_file, symlink_file, or hiding the entry entirely... not really sure which is best.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117650