Support lib changes to allow VFS implementation handling working dir locally.

[ilya-biryukov]

Summary

This is in unfinished state as of now and not ready for submission.

• Has a bunch of TODOs that must be fixed.
• Laking Windows implementation.

[ilya-biryukov]

Hi, @chapuni,

@klimek told me you could probably help with Windows-specific questions.

We're working on making clang's vfs::RealFileSystem more thread-friendly. Current implementation uses process-global working directory, we want to get rid of that. (vfs::FileSystem has setCurrentWorkingDirectory and must do all relative paths lookups according to its value).
To do that, we want to store file descriptor of working directory in vfs::RealFileSystem and do all lookups relatively to that. On Unix we achieve that by calling openat and their equivalents.
This review contains a prototype of the changes we need to Support library to make it work on Unix. The question is how to do it properly on Windows.

After doing some research, it seems that we should use NtCreateFile and pass proper ObjectAttributes.RootDirectory (e.g. see this stackoverflow question).
But AFAIU, this means we'll have to rewrite the Windows version Path.inc to use Nt-functions everywhere.
What are your thoughts on that? Is it ok to rewrite Path.inc to use Nt functions? Are there any alternative implementations/APIs that would work for us?

[zturner]

It's not just Windows, but not even all unixes support openat. Minimum required version of various Unixes are:

DragonFly 2.3.
FreeBSD 8.0.
Linux 2.6.16
NetBSD 7.0.
OpenBSD 5.0.
OS X 10.10.

I don't know how old some of these are, but you should check first to make sure we don't support any versions of these unix variants with versions older than specified above. As for Windows, If we're going to use Nt functions, I would rather they be entirely separate functions. i.e. not done via an optional parameter to an existing function, but a completely new function.

[zturner]

Also, even without using the Nt functions, you could get the working directory via the process's HANDLE, then stitch together a new working directory and build an absolute path out of it. It's not entirely atomic the way openat is because someone could change the working directory of the process after you query its working directory, but I suspect that wouldn't matter?

[ilya-biryukov]

In D36265#830856, @zturner wrote:

Also, even without using the Nt functions, you could get the working directory via the process's HANDLE, then stitch together a new working directory and build an absolute path out of it. It's not entirely atomic the way openat is because someone could change the working directory of the process after you query its working directory, but I suspect that wouldn't matter?

Right, that does not matter. I actually made a patch that does exactly that in clang's vfs::FileSystem, it doesn't require any changes in the Support library and all LLVM tests seem to pass if you do that: https://reviews.llvm.org/D36226?vs=on&id=109362&whitespace=ignore-most#toc.
But there's a major flaw in that implementation: if your working dir is symlinked and it changes during the lifetime of your program, you will be looking into different directories on each file lookup.
Unfortunately, that's what would break us at Google, since we to access the code via a FUSE filesystem that has a symlink that will change its destination on each request, we really need to read its file descriptor only once.

In D36265#830854, @zturner wrote:

It's not just Windows, but not even all unixes support openat. Minimum required version of various Unixes are:

DragonFly 2.3.
FreeBSD 8.0.
Linux 2.6.16
NetBSD 7.0.
OpenBSD 5.0.
OS X 10.10.

I don't know how old some of these are, but you should check first to make sure we don't support any versions of these unix variants with versions older than specified above. As for Windows, If we're going to use Nt functions, I would rather they be entirely separate functions. i.e. not done via an optional parameter to an existing function, but a completely new function.

Thanks, we definitely don't want to make LLVM incompatible with those platforms. I imagine we still need to keep LLVM running on those versions, they aren't too old and probably still widely used.
Now it seems we'll have to put this as a separate implementation under a configurable macro, so that it can be turned off on systems that don't support openat.
Maybe there's a list of minimal versions that LLVM strives to support?

Technically, creating separate functions should not be too hard.
There are two concerns that I have with having both APIs:

1. We must be very careful to not mix Win32 and Nt APIs (i.e. file opened via NtCreateFile should be read via NtReadFile, not ReadFile).
2. Having both Win32 and Nt functions is that Nt variants may stay untested if most of LLVM and Clang will be using Win32.

What problems do you foresee if we switch to Nt API everywhere?

[ilya-biryukov]

I'll start to work on an implementation that is only enable via a configurable macro.

That way, we could have it available for Unix variants that support openat.
Other systems will have to fallback to converting the paths to absolute.

[zturner]

In D36265#831534, @ilya-biryukov wrote:

Technically, creating separate functions should not be too hard.
There are two concerns that I have with having both APIs:

1. We must be very careful to not mix Win32 and Nt APIs (i.e. file opened via NtCreateFile should be read via NtReadFile, not ReadFile).
2. Having both Win32 and Nt functions is that Nt variants may stay untested if most of LLVM and Clang will be using Win32.

What problems do you foresee if we switch to Nt API everywhere?

The main concern I have is that the Nt functions are not intended for public consumption. They're considered internal, and as such they are subject to all the caveats listed here.

I fear it would also be impossible to safely implement some of the functions with the Nt methods. For example, openFileForRead returns an fd in one of its arguments. The way this is implemented is by calling CreateFile and then taking the resulting HANDLE and calling _open_osfhandle on it. Does this work when it's an NtCreate handle? Maybe? I don't know, but if we can't mix functions then this is just one of the problems. The point is that some of our APIs expose these handles and descriptors to the outside world, and we can't expect external users to use the Nt APIs.

If all this is hidden behind a separate function, then the requirements for that function could be documented independently. Regardless, the main thing is that it's an internal API, and it seems like a very bad idea to have our entire filesystem library for all of LLVM be based entirely on unsupported, internal OS APIs.

[krytarowski]

In D36265#830854, @zturner wrote:

It's not just Windows, but not even all unixes support openat. Minimum required version of various Unixes are:

DragonFly 2.3.
FreeBSD 8.0.
Linux 2.6.16
NetBSD 7.0.
OpenBSD 5.0.
OS X 10.10.

I don't know how old some of these are, but you should check first to make sure we don't support any versions of these unix variants with versions older than specified above. As for Windows, If we're going to use Nt functions, I would rather they be entirely separate functions. i.e. not done via an optional parameter to an existing function, but a completely new function.

Almost certainly nobody maintains newer LLVM on older versions of mentioned BSD systems.

[ilya-biryukov]

In D36265#832143, @zturner wrote:

The main concern I have is that the Nt functions are not intended for public consumption. They're considered internal, and as such they are subject to all the caveats listed here.

I totally agree that is not ideal. Even though Nt functions for file manipulations have been there for ages. I personally highly doubt they will go away in the foreseeable future, but there is no guarantee.

I fear it would also be impossible to safely implement some of the functions with the Nt methods. For example, openFileForRead returns an fd in one of its arguments. The way this is implemented is by calling CreateFile and then taking the resulting HANDLE and calling _open_osfhandle on it. Does this work when it's an NtCreate handle? Maybe? I don't know, but if we can't mix functions then this is just one of the problems. The point is that some of our APIs expose these handles and descriptors to the outside world, and we can't expect external users to use the Nt APIs.

That is certainly an issue I overlooked. There are no indications if _open_osfhandle will work properly on handles created by Nt functions.

If all this is hidden behind a separate function, then the requirements for that function could be documented independently. Regardless, the main thing is that it's an internal API, and it seems like a very bad idea to have our entire filesystem library for all of LLVM be based entirely on unsupported, internal OS APIs.

Another thing is that for Windows we may get away with just converting the paths to absolute all the time. The symlinks changing their target rapidly (i.e. during a single compile) is something we're having trouble with only on Linux, and I doubt this use-case would ever come up on Windows.
My intention currently is to provide the openat-based APIs only on Unix that have a relevant library support.
The final aim to implement a clang::vfs::FileSystem that does not rely on process-wide working directory. It would either be based on top of openat-based APIs or would convert paths to absolute on each request.

In D36265#842612, @krytarowski wrote:

Almost certainly nobody maintains newer LLVM on older versions of mentioned BSD systems.

Good to know, thanks.

include/llvm/Support/FileSystem.h

 /// @param result Set to the file status.
 /// @param follow When true, follows symlinks.  Otherwise, the symlink itself is
 ///               statted.
 /// @returns errc::success if result has been successfully set, otherwise a
 ///          platform-specific error_code.
 std::error_code status(const Twine &path, file_status &result,
                        bool follow = true);
 
+/// @brief Get file status as if by POSIX fstatat().
+///
+/// @param DirFD File descriptor of the working directory.
+/// @param path Input path.
+/// @param result Set to the file status.
+/// @param follow When true, follows symlinks.  Otherwise, the symlink itself is
+///               statted.
+/// @returns errc::success if result has been successfully set, otherwise a
+///          platform-specific error_code.
+std::error_code status_at(int DirFD, const Twine &path, file_status &result,
+                       bool follow = true);
+
+
 /// @brief A version for when a file descriptor is already available.
 std::error_code status(int FD, file_status &Result);
 
 /// @brief Set file permissions.
 ///
 /// @param Path File to set permissions on.
 /// @param Permissions New file permissions.
 /// @returns errc::success if the permissions were successfully set, otherwise

 }
 
 std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
                                  OpenFlags Flags, unsigned Mode = 0666);
 
 std::error_code openFileForRead(const Twine &Name, int &ResultFD,
                                 SmallVectorImpl<char> *RealPath = nullptr);
 
+std::error_code openFileForRead(int DirFD, const Twine &Name, int &ResultFD,
+                                SmallVectorImpl<char> *RealPath = nullptr);
+
 std::error_code getUniqueID(const Twine Path, UniqueID &Result);
 
 /// @brief Get disk space usage information.
 ///
 /// Note: Users must be careful about "Time Of Check, Time Of Use" kind of bug.
 /// Note: Windows reports results according to the quota allocated to the user.
 ///
 /// @param Path Input path.

 /// @}
 /// @name Iterators
 /// @{
 
 /// directory_entry - A single entry in a directory. Caches the status either
 /// from the result of the iteration syscall, or the first time status is
 /// called.
 class directory_entry {
+  llvm::Optional<int> CWDFD;
   std::string Path;
   bool FollowSymlinks;
   mutable file_status Status;
 
 public:
   explicit directory_entry(const Twine &path, bool follow_symlinks = true,
                            file_status st = file_status())
-      : Path(path.str()), FollowSymlinks(follow_symlinks), Status(st) {}
+                           : directory_entry(llvm::None, path, follow_symlinks, st) {}
+
+  explicit directory_entry(llvm::Optional<int> CWDFD, const Twine &path, bool follow_symlinks = true,
+                           file_status st = file_status())
+      : CWDFD(CWDFD), Path(path.str()), FollowSymlinks(follow_symlinks), Status(st) {}
 
   directory_entry() = default;
 
   void assign(const Twine &path, file_status st = file_status()) {
     Path = path.str();
     Status = st;
   }
 
   void replace_filename(const Twine &filename, file_status st = file_status());
 
+  llvm::Optional<int> cwd_fd() const { return CWDFD; }
+
   const std::string &path() const { return Path; }
   std::error_code status(file_status &result) const;
 
   bool operator==(const directory_entry& rhs) const { return Path == rhs.Path; }
   bool operator!=(const directory_entry& rhs) const { return !(*this == rhs); }
   bool operator< (const directory_entry& rhs) const;
   bool operator<=(const directory_entry& rhs) const;
   bool operator> (const directory_entry& rhs) const;
   bool operator>=(const directory_entry& rhs) const;
 };
 
 namespace detail {
+  std::error_code open_path_fd(llvm::Optional<int> cwd_fd, const Twine& Name, int& result_fd)
 
   struct DirIterState;
 
-  std::error_code directory_iterator_construct(DirIterState &, StringRef, bool);
+  std::error_code directory_iterator_construct(DirIterState &, llvm::Optional<int>, StringRef, bool);
   std::error_code directory_iterator_increment(DirIterState &);
   std::error_code directory_iterator_destruct(DirIterState &);
 
   /// Keeps state for the directory_iterator.
   struct DirIterState {
     ~DirIterState() {
       directory_iterator_destruct(*this);
     }
 
     intptr_t IterationHandle = 0;
     directory_entry CurrentEntry;
   };
 
 } // end namespace detail
 
 /// directory_iterator - Iterates through the entries in path. There is no
 /// operator++ because we need an error_code. If it's really needed we can make
 /// it call report_fatal_error on error.
 class directory_iterator {
   std::shared_ptr<detail::DirIterState> State;
   bool FollowSymlinks = true;
 
 public:
+  explicit directory_iterator(int CWDFD, const Twine &path, std::error_code &ec,
+                              bool follow_symlinks = true)
+      : FollowSymlinks(follow_symlinks) {
+    State = std::make_shared<detail::DirIterState>();
+    SmallString<128> path_storage;
+    ec = detail::directory_iterator_construct(
+        *State, CWDFD, path.toStringRef(path_storage), FollowSymlinks);
+  }
+
   explicit directory_iterator(const Twine &path, std::error_code &ec,
                               bool follow_symlinks = true)
       : FollowSymlinks(follow_symlinks) {
     State = std::make_shared<detail::DirIterState>();
     SmallString<128> path_storage;
     ec = detail::directory_iterator_construct(
-        *State, path.toStringRef(path_storage), FollowSymlinks);
+        *State, llvm::None, path.toStringRef(path_storage), FollowSymlinks);
   }
 
   explicit directory_iterator(const directory_entry &de, std::error_code &ec,
                               bool follow_symlinks = true)
       : FollowSymlinks(follow_symlinks) {
     State = std::make_shared<detail::DirIterState>();
     ec =
-        detail::directory_iterator_construct(*State, de.path(), FollowSymlinks);
+        detail::directory_iterator_construct(*State, de.cwd_fd(), de.path(), FollowSymlinks);
   }
 
   /// Construct end iterator.
   directory_iterator() = default;
 
   // No operator++ because we need error_code.
   directory_iterator &increment(std::error_code &ec) {
     ec = directory_iterator_increment(*State);

lib/Support/Unix/Path.inc

std::error_code status(const Twine &Path, file_status &Result, bool Follow) {
   SmallString<128> PathStorage;
   StringRef P = Path.toNullTerminatedStringRef(PathStorage);
 
   struct stat Status;
   int StatRet = (Follow ? ::stat : ::lstat)(P.begin(), &Status);
   return fillStatus(StatRet, Status, Result);
 }
 
+std::error_code status_at(int DirFD, const Twine &Path, file_status &Result, bool Follow) {
+  SmallString<128> PathStorage;
+  StringRef P = Path.toNullTerminatedStringRef(PathStorage);
+
+  struct stat Status;
+  int StatRet = ::fstatat(DirFD, P.begin(), &Status, Follow ? 0 : AT_SYMLINK_NOFOLLOW);
+  return fillStatus(StatRet, Status, Result);
+}
+
+
 std::error_code status(int FD, file_status &Result) {
   struct stat Status;
   int StatRet = ::fstat(FD, &Status);
   return fillStatus(StatRet, Status, Result);
 }
 
 std::error_code setPermissions(const Twine &Path, perms Permissions) {
   SmallString<128> PathStorage;
const char *mapped_file_region::const_data() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return reinterpret_cast<const char*>(Mapping);
 }
 
 int mapped_file_region::alignment() {
   return Process::getPageSize();
 }
 
+std::error_code detail::open_path_fd(llvm::Optional<int> cwd_fd, const Twine& Name, int& result_fd) {
+}
+
 std::error_code detail::directory_iterator_construct(detail::DirIterState &it,
+                                                     llvm::Optional<int> cwd_fd,
                                                      StringRef path,
                                                      bool follow_symlinks) {
   SmallString<128> path_null(path);
-  DIR *directory = ::opendir(path_null.c_str());
+  int directory_fd;
+  if (cwd_fd) {
+    // TODO: O_CLOEXEC?
+    directory_fd = ::openat(*cwd_fd, path_null.c_str(), O_RDONLY);
+  } else {
+    // TODO: O_CLOEXEC?
+    directory_fd = ::open(path_null.c_str(), O_RDONLY);
+  }
+  if (directory_fd == -1)
+    return std::error_code(errno, std::generic_category());
+
+  DIR *directory = ::fdopendir(directory_fd);
   if (!directory)
     return std::error_code(errno, std::generic_category());
 
   it.IterationHandle = reinterpret_cast<intptr_t>(directory);
   // Add something for replace_filename to replace.
   path::append(path_null, ".");
   it.CurrentEntry = directory_entry(path_null.str(), follow_symlinks);
   return directory_iterator_increment(it);

 static bool hasProcSelfFD() {
   // If we have a /proc filesystem mounted, we can quickly establish the
   // real name of the file with readlink
   static const bool Result = (::access("/proc/self/fd", R_OK) == 0);
   return Result;
 }
 #endif
 
-std::error_code openFileForRead(const Twine &Name, int &ResultFD,
+static std::error_code openFileForReadImpl(llvm::Optional<int> DirFD, const Twine &Name, int &ResultFD,
                                 SmallVectorImpl<char> *RealPath) {
   SmallString<128> Storage;
   StringRef P = Name.toNullTerminatedStringRef(Storage);
   int OpenFlags = O_RDONLY;
 #ifdef O_CLOEXEC
   OpenFlags |= O_CLOEXEC;
 #endif
+  if (!DirFD) {
+    // Use 'open'.
     if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags)) < 0)
       return std::error_code(errno, std::generic_category());
+  } else {
+    // Use 'openat'.
+     if ((ResultFD = sys::RetryAfterSignal(-1, openat, *DirFD, P.begin(), OpenFlags)) < 0)
+    return std::error_code(errno, std::generic_category());
+  }
 #ifndef O_CLOEXEC
   int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC);
   (void)r;
   assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed");
 #endif
   // Attempt to get the real name of the file, if the user asked
   if(!RealPath)
     return std::error_code();
   RealPath->clear();
 #if defined(F_GETPATH)
   // When F_GETPATH is availble, it is the quickest way to get
   // the real path name.
   char Buffer[MAXPATHLEN];
   if (::fcntl(ResultFD, F_GETPATH, Buffer) != -1)
     RealPath->append(Buffer, Buffer + strlen(Buffer));
 #else
   char Buffer[PATH_MAX];
   if (hasProcSelfFD()) {
     char ProcPath[64];
     snprintf(ProcPath, sizeof(ProcPath), "/proc/self/fd/%d", ResultFD);
     ssize_t CharCount = ::readlink(ProcPath, Buffer, sizeof(Buffer));
     if (CharCount > 0)
       RealPath->append(Buffer, Buffer + CharCount);
   } else {
+    if (DirFD)
+      return std::error_code(); // there's no equivalent of ::realpath that has a file descriptor.
     // Use ::realpath to get the real path name
     if (::realpath(P.begin(), Buffer) != nullptr)
       RealPath->append(Buffer, Buffer + strlen(Buffer));
   }
 #endif
   return std::error_code();
 }
 
+std::error_code openDirectoryForAt(const Twine &Name, int &ResultFD, SmallVectorImpl<char> *RealPath) {
+  // TODO: might use O_PATH flag here
+  return openFileForRead(Name, ResultFD, RealPath);
+}
+
+std::error_code openFileForRead(const Twine &Name, int &ResultFD,
+                                SmallVectorImpl<char> *RealPath) {
+  return openFileForReadImpl(llvm::None, Name, ResultFD, RealPath);
+}
+
+std::error_code openFileForRead(int DirFD, const Twine &Name, int &ResultFD,
+                                SmallVectorImpl<char> *RealPath) {
+  return openFileForReadImpl(DirFD, Name, ResultFD, RealPath);
+}
+
 std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
                             sys::fs::OpenFlags Flags, unsigned Mode) {
   // Verify that we don't have both "append" and "excl".
   assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) &&
          "Cannot specify both 'excl' and 'append' file creation flags!");
 
   int OpenFlags = O_CREAT;