Improve the robustness of mmap

[zturner]

Summary

When we mmap a file, we don't take into consideration whether the file is on a remote share. mmaping such a file is problematic because if the network share goes away -- which can happen for reasons outside of the user's control -- anything trying to read from that file will have undefined behavior.

LLDB already has logic to check whether a file is local or remote, but it was not implemented on Windows.

Also, mmap on newer versions of OSX have some additional flags which LLDB requires and which I see no reason to ever NOT include even for general usage, so the low level call to mmap is udpated to use those flags when available.

Note that I do not have any non-Windows machine to test on, so I'm including a lot of people from various platforms here to look over this:

Getting the statvfs / statfs distiction correct was tricky, but hopefully I got this correct. FWIU,

NetBSD: statfs does not exist! Must use statvfs for everything. Reference
FreeBSD: statvfs::f_flag does not contain MNT_LOCAL. Must use statfs. Reference
OpenBSD: Both functions exist, but documentation does not specify whether the flags differ between the two.

Since FreeBSD cannot use statvfs to check for MNT_LOCAL, I'm adjusting the conditional in Path.inc so that FreeBSD and OpenBSD follow the same codepath. Nothing was broken before, it's only now that we need to use f_flag & MNT_LOCAL that FreeBSD requires a change to use statfs instead.

[amccarth]

The Windows bits look right to me. Double check the clang-format to make sure those long lines are wrapped nicely.

[zturner]

Had to manually force clang-format to get the .inc files, it ignores them by default.

[rnk]

Looks right to me. I'm not familiar with the stat syscall, though, so I'd look for a reviewer for that.

llvm/unittests/Support/Path.cpp

1158 ↗

(On Diff #88619)

I guess mounting a remote filesystem is out of scope for a unit test. :)

[zturner]

I pulled the non-Windows code for this from here, btw:

https://github.com/llvm-mirror/lldb/blob/master/source/Host/posix/FileSystem.cpp#L216-L254

[ruiu]

llvm/lib/Support/Unix/Path.inc

346 ↗

(On Diff #88619)

Does this function return other than std::error_code()?

[zturner]

llvm/lib/Support/Unix/Path.inc

346 ↗

(On Diff #88619)

No, it can't encounter an error. I guess I should update the function signature to reflect that.

[zturner]

Fixed suggestion from ruiu. In fixing that I noticed that there was no break in between cases in the Linux preprocessor branch, so I fixed that too. Good thing you pointed that out!

[krytarowski]

llvm/lib/Support/Unix/Path.inc

68 ↗

(On Diff #88785)

Is this equivalent to defined(__linux__) || defined(__NetBSD__)? If so it might look easier to parse presented this in the way of what system is used for this.

[zturner]

llvm/lib/Support/Unix/Path.inc

68 ↗

(On Diff #88785)

If you scroll down the file about 30 lines you'll see this:

#if defined(__FreeBSD__) || defined (__NetBSD__) || defined(__Bitrig__) || \
    defined(__OpenBSD__) || defined(__minix) || defined(__FreeBSD_kernel__) || \
    defined(__linux__) || defined(__CYGWIN__) || defined(__DragonFly__) || \
    defined(_AIX)

I don't know the ins and outs of all these definitions, but I'm pretty sure at least some of them will fall into this #if branch. And there could even be other ones that are not listed here but which define other platforms. So I'm not entirely sure what the inverse of this conditional is.

[krytarowski]

llvm/lib/Support/Unix/Path.inc

68 ↗

(On Diff #88785)

Yes I saw this switch and I was thinking how many OS'es are really used there. Other than that NetBSD platform part looks fine. I will test build it.

[ruiu]

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

[zturner]

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

[ruiu]

Thank you for the reply. That's very helpful to understand the motivation of this patch. Please add that to somewhere in this patch as a comment.

[krytarowski]

In file included from /tmp/pkgsrc-tmp/wip/llvm-git/work/llvm/lib/Support/Path.cpp:1174:0:
/tmp/pkgsrc-tmp/wip/llvm-git/work/llvm/lib/Support/Unix/Path.inc: In function 'std::error_code llvm::sys::fs::is_local(int, bool&)':
/tmp/pkgsrc-tmp/wip/llvm-git/work/llvm/lib/Support/Unix/Path.inc:376:35: error: too many arguments to function 'bool llvm::sys::fs::is_local_impl(statvfs&)'
   return is_local_impl(Vfs, Result);
                                   ^
/tmp/pkgsrc-tmp/wip/llvm-git/work/llvm/lib/Support/Unix/Path.inc:346:13: note: declared here
 static bool is_local_impl(struct STATVFS &Vfs) {
             ^
/tmp/pkgsrc-tmp/wip/llvm-git/work/llvm/lib/Support/Unix/Path.inc:376:23: error: could not convert 'llvm::sys::fs::is_local_impl(Vfs)' from 'bool' to 'std::error_code'
   return is_local_impl(Vfs, Result);
                       ^
[ 26%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/SearchForAddressOfSpecialSymbol.cpp.o
[ 26%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/Signals.cpp.o
--- lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o ---
*** [lib/Support/CMakeFiles/LLVMSupport.dir/Path.cpp.o] Error code 1

make[2]: stopped in /tmp/pkgsrc-tmp/wip/llvm-git/work/build

My host: NetBSD 7.99.59 amd64

[zturner]

Fixed careless error in last update of patch.

[zturner]

Improve the comment explaining the rationale behind this change.

[krytarowski]

It builds now. (NetBSD 7.99.59 amd64)

[labath]

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

The thing here is that the failure modes for read() are much saner than for mmap()ed memory. If the NFS share goes down *after* the read(), nothing happens. If it goes down *during* the read(), the syscall will just return -1, and that's an error you can easily handle.

In the mmap case, if the NFS share goes down after you have mmap()ed the memory, that memory will be silently removed from your address space, and next time you try to access it you will get a SIGBUS. It is still possible to recover from this situation, but it is much more tricky.

That said, local files are also not immune to the SIGBUS issue - the same thing can happen if someone does an truncate(2) on the mmap()ed file, but I guess those kinds of issues are much more rare in normal circumstances.

llvm/lib/Support/Unix/Path.inc

68 ↗

(On Diff #88785)

I guess the proper solution here would be to do a check at cmake-time for the appropriate symbols, and then branch here based on that.

[labath]

llvm/lib/Support/Unix/Path.inc

68 ↗

(On Diff #88785)

This does not compile on linux. struct statvfs does not have the f_type field. I guess you need to use the struct statfs branch on linux.

[zturner]

Make linux codepath use statfs.

[pcc]

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

[zturner]

In D30010#680007, @pcc wrote:

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

I actually considered doing something like that, and I'm not opposed to it if people think it's a good idea. One name I had come up with is bool Ephemeral, and it defaults to true (implying that by default mappings are short-lived, and only in the case that the mapping is long-lived would you want to "de-optimize" in this manner. Then LLDB could pass false. If we're worried that Ephemeral is a word people might not be familiar with, I could call it ShortLived or LongLived as well.

[zturner]

In D30010#680022, @zturner wrote:

In D30010#680007, @pcc wrote:

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

I actually considered doing something like that, and I'm not opposed to it if people think it's a good idea. One name I had come up with is bool Ephemeral, and it defaults to true (implying that by default mappings are short-lived, and only in the case that the mapping is long-lived would you want to "de-optimize" in this manner. Then LLDB could pass false. If we're worried that Ephemeral is a word people might not be familiar with, I could call it ShortLived or LongLived as well.

In case it's not clear btw, you were proposing re-using an existing argument whereas my idea was to add a new argument. The IsVolatileSize argument seems kind of orthogonal to what we're doing here, so it's hard to come up with an intuitive way to merge the two things into one variable.

[zturner]

Actually one way to do this while simultaneously reducing parameter list explosion might be to make a flags enum, and merge some of the existing ones in. Right now we already have RequiresNullTerminator and IsVolatileSize. We could have:

enum class MemBufferFlags {
  RequiresNullTerminator = 0x1,
  IsVolatileSize = 0x2,
  LongLived = 0x4
};

Then each function could just take a MemBufferFlags with appropriate default value instead of a list of bools.

[pcc]

In D30010#680027, @zturner wrote:

In D30010#680022, @zturner wrote:

In D30010#680007, @pcc wrote:

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

I actually considered doing something like that, and I'm not opposed to it if people think it's a good idea. One name I had come up with is bool Ephemeral, and it defaults to true (implying that by default mappings are short-lived, and only in the case that the mapping is long-lived would you want to "de-optimize" in this manner. Then LLDB could pass false. If we're worried that Ephemeral is a word people might not be familiar with, I could call it ShortLived or LongLived as well.

In case it's not clear btw, you were proposing re-using an existing argument whereas my idea was to add a new argument. The IsVolatileSize argument seems kind of orthogonal to what we're doing here, so it's hard to come up with an intuitive way to merge the two things into one variable.

I would rename the existing argument. I believe that the only thing that IsVolatileSize does is disable the use of mmap. Its original use case is pretty much the same I believe (libclang, which may be embedded in a text editor or some other interactive application, needed to arrange to prevent text editors from changing file contents beneath its feet -- NFS shares going away is just another instance of the same issue).

[zturner]

In D30010#680030, @pcc wrote:

In D30010#680027, @zturner wrote:

In D30010#680022, @zturner wrote:

In D30010#680007, @pcc wrote:

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

I actually considered doing something like that, and I'm not opposed to it if people think it's a good idea. One name I had come up with is bool Ephemeral, and it defaults to true (implying that by default mappings are short-lived, and only in the case that the mapping is long-lived would you want to "de-optimize" in this manner. Then LLDB could pass false. If we're worried that Ephemeral is a word people might not be familiar with, I could call it ShortLived or LongLived as well.

In case it's not clear btw, you were proposing re-using an existing argument whereas my idea was to add a new argument. The IsVolatileSize argument seems kind of orthogonal to what we're doing here, so it's hard to come up with an intuitive way to merge the two things into one variable.

I would rename the existing argument. I believe that the only thing that IsVolatileSize does is disable the use of mmap. Its original use case is pretty much the same I believe (libclang, which may be embedded in a text editor or some other interactive application, needed to arrange to prevent text editors from changing file contents beneath its feet -- NFS shares going away is just another instance of the same issue).

The only concern I have is that it's easy for someone to answer the questions "Does the file have a volatile size?" and "Does the file reside locally?" without understanding the implications of specifying true or false. It's harder for them to answer the question "Should I disable the use of mmap on this file?" So I can call the argument AllowMmap = true, but I worry this might be a step backwards from an API standpoint.

[pcc]

In D30010#680035, @zturner wrote:

In D30010#680030, @pcc wrote:

In D30010#680027, @zturner wrote:

In D30010#680022, @zturner wrote:

In D30010#680007, @pcc wrote:

In D30010#679201, @zturner wrote:

In D30010#679198, @ruiu wrote:

I wonder what this patch is trying to fix on Unix machines. mmap()'ing a file on NFS is not a special thing, and if something goes wrong with NFS, a process will hang whether a file is being used through read(2)/write(2) or mmap. What am I missing?

If you just read() the entire contents, in order for the call to fail the NFS would have to go down during the very short period of time where you are reading the entire file contents into memory. If you mmap the contents and then keep the mapping open for, say several hours, then if the NFS has a hiccup at any point during those several hours, a call to read could crash. Any time you are dealing with a network file system you of course might encounter problems, I think this just minimizes the likelihood of those problems occuring in practice.

The motivation for this change came when I tried to replace LLDB's mmap code with LLVM's. But I was told that checking if the file is local is a requirement, because they've had bugs in the past where they crash all of Xcode because they try to mmap some debug info off of a network share and then some time later during debugging, the share goes down and read crashes all of Xcode.

This seems like an LLDB-specific issue though (or in general it seems more specific to interactive applications). Wouldn't it be better to put the decision in the hands of the application? For example, LLDB could call is_local by itself and pass the result as the IsVolatileSize argument (which should probably have a better name).

I actually considered doing something like that, and I'm not opposed to it if people think it's a good idea. One name I had come up with is bool Ephemeral, and it defaults to true (implying that by default mappings are short-lived, and only in the case that the mapping is long-lived would you want to "de-optimize" in this manner. Then LLDB could pass false. If we're worried that Ephemeral is a word people might not be familiar with, I could call it ShortLived or LongLived as well.

In case it's not clear btw, you were proposing re-using an existing argument whereas my idea was to add a new argument. The IsVolatileSize argument seems kind of orthogonal to what we're doing here, so it's hard to come up with an intuitive way to merge the two things into one variable.

I would rename the existing argument. I believe that the only thing that IsVolatileSize does is disable the use of mmap. Its original use case is pretty much the same I believe (libclang, which may be embedded in a text editor or some other interactive application, needed to arrange to prevent text editors from changing file contents beneath its feet -- NFS shares going away is just another instance of the same issue).

The only concern I have is that it's easy for someone to answer the questions "Does the file have a volatile size?" and "Does the file reside locally?" without understanding the implications of specifying true or false. It's harder for them to answer the question "Should I disable the use of mmap on this file?" So I can call the argument AllowMmap = true, but I worry this might be a step backwards from an API standpoint.

The semantic effect of this argument is that the application requires that the MemoryBuffer contains a snapshot of the file contents at this point in time (which implies that it is immune to changes to file size, file contents, file system failures, etc.) If we document the flag in those terms and give it a suitable name (Snapshot maybe?), I feel that that would be sufficient from an API perspective.

[zturner]

Actually, now that I think about it, instead of calling the parameter IsVolatileSize, how about just IsVolatile? That includes the case that the entire file is volatile (and might disappear from a network share).

[pcc]

In D30010#680067, @zturner wrote:

Actually, now that I think about it, instead of calling the parameter IsVolatileSize, how about just IsVolatile? That includes the case that the entire file is volatile (and might disappear from a network share).

SGTM.

[zturner]

Rename parameter from IsVolatileSize to IsVolatile, and remove the logic in shouldUseMmap to check if the file is local.

[zturner]

Now that this basically doesn't change any existing behavior, I assume this is good to go since we're leaving it up to the applications?

[beanz]

Don't really have much to add here, but I really like what this is doing, and I think it will be a huge win for LLDB. Thanks @zturner!

[zturner]

Seems like everything is addressed and people have OK'ed the correctness of the implementation on multiple platforms. So I'll try to put this in tonight if there are no other objections.

[labath]

Seems to work for me after adding the magic.h include. I can't really guarantee that all bots will be happy with it though..

llvm/lib/Support/Unix/Path.inc

78 ↗

(On Diff #88918)

__ANDROID__ is a subset of __linux__. You can drop the former.

350 ↗

(On Diff #88918)

You need to #include <linux/magic.h> to get these constants.

llvm/trunk/include/llvm/Support/FileSystem.h

 
 /// @brief Simpler version of equivalent for clients that don't need to
 ///        differentiate between an error and false.
 inline bool equivalent(const Twine &A, const Twine &B) {
   bool result;
   return !equivalent(A, B, result) && result;
 }
 
+/// @brief Is the file mounted on a local filesystem?
+///
+/// @param path Input path.
+/// @param result Set to true if \a path is on fixed media such as a hard disk,
+///               false if it is not.
+/// @returns errc::success if result has been successfully set, otherwise a
+///          platform specific error_code.
+std::error_code is_local(const Twine &path, bool &result);
+
+/// @brief Version of is_local accepting an open file descriptor.
+std::error_code is_local(int FD, bool &result);
+
+/// @brief Simpler version of is_local for clients that don't need to
+///        differentiate between an error and false.
+inline bool is_local(const Twine &Path) {
+  bool Result;
+  return !is_local(Path, Result) && Result;
+}
+
+/// @brief Simpler version of is_local accepting an open file descriptor for
+///        clients that don't need to differentiate between an error and false.
+inline bool is_local(int FD) {
+  bool Result;
+  return !is_local(FD, Result) && Result;
+}
+
 /// @brief Does status represent a directory?
 ///
 /// @param status A file_status previously returned from status.
 /// @returns status.type() == file_type::directory_file.
 bool is_directory(file_status status);
 
 /// @brief Is path a directory?
 ///

llvm/trunk/include/llvm/Support/MemoryBuffer.h

public:
   /// from.
   virtual StringRef getBufferIdentifier() const { return "Unknown buffer"; }
 
   /// Open the specified file as a MemoryBuffer, returning a new MemoryBuffer
   /// if successful, otherwise returning null. If FileSize is specified, this
   /// means that the client knows that the file exists and that it has the
   /// specified size.
   ///
-  /// \param IsVolatileSize Set to true to indicate that the file size may be
-  /// changing, e.g. when libclang tries to parse while the user is
-  /// editing/updating the file.
+  /// \param IsVolatile Set to true to indicate that the contents of the file
+  /// can change outside the user's control, e.g. when libclang tries to parse
+  /// while the user is editing/updating the file or if the file is on an NFS.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFile(const Twine &Filename, int64_t FileSize = -1,
-          bool RequiresNullTerminator = true, bool IsVolatileSize = false);
+          bool RequiresNullTerminator = true, bool IsVolatile = false);
 
   /// Read all of the specified file into a MemoryBuffer as a stream
   /// (i.e. until EOF reached). This is useful for special files that
   /// look like a regular file but have 0 size (e.g. /proc/cpuinfo on Linux).
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFileAsStream(const Twine &Filename);
 
   /// Given an already-open file descriptor, map some slice of it into a
   /// MemoryBuffer. The slice is specified by an \p Offset and \p MapSize.
   /// Since this is in the middle of a file, the buffer is not null terminated.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getOpenFileSlice(int FD, const Twine &Filename, uint64_t MapSize,
-                   int64_t Offset);
+                   int64_t Offset, bool IsVolatile = false);
 
   /// Given an already-open file descriptor, read the file and return a
   /// MemoryBuffer.
   ///
-  /// \param IsVolatileSize Set to true to indicate that the file size may be
-  /// changing, e.g. when libclang tries to parse while the user is
-  /// editing/updating the file.
+  /// \param IsVolatile Set to true to indicate that the contents of the file
+  /// can change outside the user's control, e.g. when libclang tries to parse
+  /// while the user is editing/updating the file or if the file is on an NFS.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getOpenFile(int FD, const Twine &Filename, uint64_t FileSize,
-              bool RequiresNullTerminator = true, bool IsVolatileSize = false);
+              bool RequiresNullTerminator = true, bool IsVolatile = false);
 
   /// Open the specified memory range as a MemoryBuffer. Note that InputData
   /// must be null terminated if RequiresNullTerminator is true.
   static std::unique_ptr<MemoryBuffer>
   getMemBuffer(StringRef InputData, StringRef BufferName = "",
                bool RequiresNullTerminator = true);
 
   static std::unique_ptr<MemoryBuffer>
public:
   /// Open the specified file as a MemoryBuffer, or open stdin if the Filename
   /// is "-".
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
   getFileOrSTDIN(const Twine &Filename, int64_t FileSize = -1,
                  bool RequiresNullTerminator = true);
 
   /// Map a subrange of the specified file as a MemoryBuffer.
   static ErrorOr<std::unique_ptr<MemoryBuffer>>
-  getFileSlice(const Twine &Filename, uint64_t MapSize, uint64_t Offset);
+  getFileSlice(const Twine &Filename, uint64_t MapSize, uint64_t Offset, bool IsVolatile = false);
 
   //===--------------------------------------------------------------------===//
   // Provided for performance analysis.
   //===--------------------------------------------------------------------===//
 
   /// The kind of memory backing used to support the MemoryBuffer.
   enum BufferKind {
     MemoryBuffer_Malloc,

llvm/trunk/lib/Support/MemoryBuffer.cpp

public:
   BufferKind getBufferKind() const override {
     return MemoryBuffer_Malloc;
   }
 };
 }
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize, 
-           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatileSize);
+           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatile);
 
 std::unique_ptr<MemoryBuffer>
 MemoryBuffer::getMemBuffer(StringRef InputData, StringRef BufferName,
                            bool RequiresNullTerminator) {
   auto *Ret = new (NamedBufferAlloc(BufferName))
       MemoryBufferMem(InputData, RequiresNullTerminator);
   return std::unique_ptr<MemoryBuffer>(Ret);
 }
MemoryBuffer::getFileOrSTDIN(const Twine &Filename, int64_t FileSize,
 
   if (NameRef == "-")
     return getSTDIN();
   return getFile(Filename, FileSize, RequiresNullTerminator);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFileSlice(const Twine &FilePath, uint64_t MapSize, 
-                           uint64_t Offset) {
-  return getFileAux(FilePath, -1, MapSize, Offset, false, false);
+                           uint64_t Offset, bool IsVolatile) {
+  return getFileAux(FilePath, -1, MapSize, Offset, false, IsVolatile);
 }
 
 
 //===----------------------------------------------------------------------===//
 // MemoryBuffer::getFile implementation.
 //===----------------------------------------------------------------------===//
 
 namespace {
getMemoryBufferForStream(int FD, const Twine &BufferName) {
   } while (ReadBytes != 0);
 
   return MemoryBuffer::getMemBufferCopy(Buffer, BufferName);
 }
 
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getFile(const Twine &Filename, int64_t FileSize,
-                      bool RequiresNullTerminator, bool IsVolatileSize) {
+                      bool RequiresNullTerminator, bool IsVolatile) {
   return getFileAux(Filename, FileSize, FileSize, 0,
-                    RequiresNullTerminator, IsVolatileSize);
+                    RequiresNullTerminator, IsVolatile);
 }
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getOpenFileImpl(int FD, const Twine &Filename, uint64_t FileSize,
                 uint64_t MapSize, int64_t Offset, bool RequiresNullTerminator,
-                bool IsVolatileSize);
+                bool IsVolatile);
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize,
-           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatileSize) {
+           uint64_t Offset, bool RequiresNullTerminator, bool IsVolatile) {
   int FD;
   std::error_code EC = sys::fs::openFileForRead(Filename, FD);
   if (EC)
     return EC;
 
   ErrorOr<std::unique_ptr<MemoryBuffer>> Ret =
       getOpenFileImpl(FD, Filename, FileSize, MapSize, Offset,
-                      RequiresNullTerminator, IsVolatileSize);
+                      RequiresNullTerminator, IsVolatile);
   close(FD);
   return Ret;
 }
 
 static bool shouldUseMmap(int FD,
                           size_t FileSize,
                           size_t MapSize,
                           off_t Offset,
                           bool RequiresNullTerminator,
                           int PageSize,
-                          bool IsVolatileSize) {
+                          bool IsVolatile) {
   // mmap may leave the buffer without null terminator if the file size changed
   // by the time the last page is mapped in, so avoid it if the file size is
   // likely to change.
-  if (IsVolatileSize)
+  if (IsVolatile)
     return false;
 
   // We don't use mmap for small files because this can severely fragment our
   // address space.
   if (MapSize < 4 * 4096 || MapSize < (unsigned)PageSize)
     return false;
 
   if (!RequiresNullTerminator)
     return true;
 
-
   // If we don't know the file size, use fstat to find out.  fstat on an open
   // file descriptor is cheaper than stat on a random path.
   // FIXME: this chunk of code is duplicated, but it avoids a fstat when
   // RequiresNullTerminator = false and MapSize != -1.
   if (FileSize == size_t(-1)) {
     sys::fs::file_status Status;
     if (sys::fs::status(FD, Status))
       return false;

 #endif
 
   return true;
 }
 
 static ErrorOr<std::unique_ptr<MemoryBuffer>>
 getOpenFileImpl(int FD, const Twine &Filename, uint64_t FileSize,
                 uint64_t MapSize, int64_t Offset, bool RequiresNullTerminator,
-                bool IsVolatileSize) {
+                bool IsVolatile) {
   static int PageSize = sys::Process::getPageSize();
 
   // Default is to map the full file.
   if (MapSize == uint64_t(-1)) {
     // If we don't know the file size, use fstat to find out.  fstat on an open
     // file descriptor is cheaper than stat on a random path.
     if (FileSize == uint64_t(-1)) {
       sys::fs::file_status Status;
if (FileSize == uint64_t(-1)) {
         return getMemoryBufferForStream(FD, Filename);
 
       FileSize = Status.getSize();
     }
     MapSize = FileSize;
   }
 
   if (shouldUseMmap(FD, FileSize, MapSize, Offset, RequiresNullTerminator,
-                    PageSize, IsVolatileSize)) {
+                    PageSize, IsVolatile)) {
     std::error_code EC;
     std::unique_ptr<MemoryBuffer> Result(
         new (NamedBufferAlloc(Filename))
         MemoryBufferMMapFile(RequiresNullTerminator, FD, MapSize, Offset, EC));
     if (!EC)
       return std::move(Result);
   }
 
#endif
     BufPtr += NumRead;
   }
 
   return std::move(Buf);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getOpenFile(int FD, const Twine &Filename, uint64_t FileSize,
-                          bool RequiresNullTerminator, bool IsVolatileSize) {
+                          bool RequiresNullTerminator, bool IsVolatile) {
   return getOpenFileImpl(FD, Filename, FileSize, FileSize, 0,
-                         RequiresNullTerminator, IsVolatileSize);
+                         RequiresNullTerminator, IsVolatile);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 MemoryBuffer::getOpenFileSlice(int FD, const Twine &Filename, uint64_t MapSize,
-                               int64_t Offset) {
+                               int64_t Offset, bool IsVolatile) {
   assert(MapSize != uint64_t(-1));
-  return getOpenFileImpl(FD, Filename, -1, MapSize, Offset, false,
-                         /*IsVolatileSize*/ false);
+  return getOpenFileImpl(FD, Filename, -1, MapSize, Offset, false, IsVolatile);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>> MemoryBuffer::getSTDIN() {
   // Read in all of the data from stdin, we cannot mmap stdin.
   //
   // FIXME: That isn't necessarily true, we should try to mmap stdin and
   // fallback if it fails.
   sys::ChangeStdinToBinary();

llvm/trunk/lib/Support/Unix/Path.inc

 #undef feof
 
 // For GNU Hurd
 #if defined(__GNU__) && !defined(PATH_MAX)
 # define PATH_MAX 4096
 #endif
 
 #include <sys/types.h>
-#if !defined(__APPLE__) && !defined(__OpenBSD__) && !defined(__ANDROID__)
+#if !defined(__APPLE__) && !defined(__OpenBSD__) && !defined(__FreeBSD__) &&   \
+    !defined(__linux__)
 #include <sys/statvfs.h>
 #define STATVFS statvfs
+#define FSTATVFS fstatvfs
 #define STATVFS_F_FRSIZE(vfs) vfs.f_frsize
 #else
-#ifdef __OpenBSD__
+#if defined(__OpenBSD__) || defined(__FreeBSD__)
 #include <sys/param.h>
 #include <sys/mount.h>
-#elif defined(__ANDROID__)
+#elif defined(__linux__)
+#include <linux/magic.h>
 #include <sys/vfs.h>
 #else
 #include <sys/mount.h>
 #endif
 #define STATVFS statfs
+#define FSTATVFS fstatfs
 #define STATVFS_F_FRSIZE(vfs) static_cast<uint64_t>(vfs.f_bsize)
 #endif
 
+#if defined(__NetBSD__)
+#define STATVFS_F_FLAG(vfs) (vfs).f_flag
+#else
+#define STATVFS_F_FLAG(vfs) (vfs).f_flags
+#endif
 
 using namespace llvm;
 
 namespace llvm {
 namespace sys  {
 namespace fs {
 #if defined(__FreeBSD__) || defined (__NetBSD__) || defined(__Bitrig__) || \
     defined(__OpenBSD__) || defined(__minix) || defined(__FreeBSD_kernel__) || \
std::error_code remove(const Twine &path, bool IgnoreNonExisting) {
   if (::remove(p.begin()) == -1) {
     if (errno != ENOENT || !IgnoreNonExisting)
       return std::error_code(errno, std::generic_category());
   }
 
   return std::error_code();
 }
 
+static bool is_local_impl(struct STATVFS &Vfs) {
+#if defined(__linux__)
+  constexpr uint32_t CIFS_MAGIC_NUMBER = 0xFF534D42;
+  switch ((uint32_t)Vfs.f_type) {
+  case NFS_SUPER_MAGIC:
+  case SMB_SUPER_MAGIC:
+  case CIFS_MAGIC_NUMBER:
+    return false;
+  default:
+    return true;
+  }
+#else
+  return !!(STATVFS_F_FLAG(Vfs) & MNT_LOCAL);
+#endif
+}
+
+std::error_code is_local(const Twine &Path, bool &Result) {
+  struct STATVFS Vfs;
+  if (::STATVFS(Path.str().c_str(), &Vfs))
+    return std::error_code(errno, std::generic_category());
+
+  Result = is_local_impl(Vfs);
+  return std::error_code();
+}
+
+std::error_code is_local(int FD, bool &Result) {
+  struct STATVFS Vfs;
+  if (::FSTATVFS(FD, &Vfs))
+    return std::error_code(errno, std::generic_category());
+
+  Result = is_local_impl(Vfs);
+  return std::error_code();
+}
+
 std::error_code rename(const Twine &from, const Twine &to) {
   // Get arguments.
   SmallString<128> from_storage;
   SmallString<128> to_storage;
   StringRef f = from.toNullTerminatedStringRef(from_storage);
   StringRef t = to.toNullTerminatedStringRef(to_storage);
 
   if (::rename(f.begin(), t.begin()) == -1)

 }
 
 std::error_code mapped_file_region::init(int FD, uint64_t Offset,
                                          mapmode Mode) {
   assert(Size != 0);
 
   int flags = (Mode == readwrite) ? MAP_SHARED : MAP_PRIVATE;
   int prot = (Mode == readonly) ? PROT_READ : (PROT_READ | PROT_WRITE);
+#if defined(__APPLE__)
+//----------------------------------------------------------------------
+// Newer versions of MacOSX have a flag that will allow us to read from
+// binaries whose code signature is invalid without crashing by using
+// the MAP_RESILIENT_CODESIGN flag. Also if a file from removable media
+// is mapped we can avoid crashing and return zeroes to any pages we try
+// to read if the media becomes unavailable by using the
+// MAP_RESILIENT_MEDIA flag.
+//----------------------------------------------------------------------
+#if defined(MAP_RESILIENT_CODESIGN)
+  flags |= MAP_RESILIENT_CODESIGN;
+#endif
+#if defined(MAP_RESILIENT_MEDIA)
+  flags |= MAP_RESILIENT_MEDIA;
+#endif
+#endif // #if defined (__APPLE__)
+
   Mapping = ::mmap(nullptr, Size, prot, flags, FD, Offset);
   if (Mapping == MAP_FAILED)
     return std::error_code(errno, std::generic_category());
   return std::error_code();
 }
 
 mapped_file_region::mapped_file_region(int fd, mapmode mode, uint64_t length,
                                        uint64_t offset, std::error_code &ec)

llvm/trunk/lib/Support/Windows/Path.inc

std::error_code remove(const Twine &path, bool IgnoreNonExisting) {
   if (!::DeleteFileW(c_str(path_utf16))) {
     std::error_code EC = mapWindowsError(::GetLastError());
     if (EC != errc::no_such_file_or_directory || !IgnoreNonExisting)
       return EC;
   }
   return std::error_code();
 }
 
+static std::error_code is_local_internal(SmallVectorImpl<wchar_t> &Path,
+                                         bool &Result) {
+  SmallVector<wchar_t, 128> VolumePath;
+  size_t Len = 128;
+  while (true) {
+    VolumePath.resize(Len);
+    BOOL Success =
+        ::GetVolumePathNameW(Path.data(), VolumePath.data(), VolumePath.size());
+
+    if (Success)
+      break;
+
+    DWORD Err = ::GetLastError();
+    if (Err != ERROR_INSUFFICIENT_BUFFER)
+      return mapWindowsError(Err);
+
+    Len *= 2;
+  }
+  // If the output buffer has exactly enough space for the path name, but not
+  // the null terminator, it will leave the output unterminated.  Push a null
+  // terminator onto the end to ensure that this never happens.
+  VolumePath.push_back(L'\0');
+  VolumePath.set_size(wcslen(VolumePath.data()));
+  const wchar_t *P = VolumePath.data();
+
+  UINT Type = ::GetDriveTypeW(P);
+  switch (Type) {
+  case DRIVE_FIXED:
+    Result = true;
+    return std::error_code();
+  case DRIVE_REMOTE:
+  case DRIVE_CDROM:
+  case DRIVE_RAMDISK:
+  case DRIVE_REMOVABLE:
+    Result = false;
+    return std::error_code();
+  default:
+    return make_error_code(errc::no_such_file_or_directory);
+  }
+  llvm_unreachable("Unreachable!");
+}
+
+std::error_code is_local(const Twine &path, bool &result) {
+  if (!llvm::sys::fs::exists(path) || !llvm::sys::path::has_root_path(path))
+    return make_error_code(errc::no_such_file_or_directory);
+
+  SmallString<128> Storage;
+  StringRef P = path.toStringRef(Storage);
+
+  // Convert to utf-16.
+  SmallVector<wchar_t, 128> WidePath;
+  if (std::error_code ec = widenPath(P, WidePath))
+    return ec;
+  return is_local_internal(WidePath, result);
+}
+
+std::error_code is_local(int FD, bool &Result) {
+  SmallVector<wchar_t, 128> FinalPath;
+  HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+
+  size_t Len = 128;
+  do {
+    FinalPath.reserve(Len);
+    Len = ::GetFinalPathNameByHandleW(Handle, FinalPath.data(),
+                                      FinalPath.capacity() - 1, VOLUME_NAME_NT);
+    if (Len == 0)
+      return mapWindowsError(::GetLastError());
+  } while (Len > FinalPath.capacity());
+
+  FinalPath.set_size(Len);
+
+  return is_local_internal(FinalPath, Result);
+}
+
 std::error_code rename(const Twine &from, const Twine &to) {
   // Convert to utf-16.
   SmallVector<wchar_t, 128> wide_from;
   SmallVector<wchar_t, 128> wide_to;
   if (std::error_code ec = widenPath(from, wide_from))
     return ec;
   if (std::error_code ec = widenPath(to, wide_to))
     return ec;

llvm/trunk/unittests/Support/Path.cpp

if (!ResultPath.empty()) {
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(TempPath), D1));
     ASSERT_NO_ERROR(fs::getUniqueID(Twine(ResultPath), D2));
     ASSERT_EQ(D1, D2);
   }
 
   ::close(FileDescriptor);
 }
 
+TEST_F(FileSystemTest, is_local) {
+  SmallString<128> CurrentPath;
+  ASSERT_NO_ERROR(fs::current_path(CurrentPath));
+
+  bool Result;
+  ASSERT_NO_ERROR(fs::is_local(CurrentPath, Result));
+  EXPECT_TRUE(Result);
+  EXPECT_TRUE(fs::is_local(CurrentPath));
+
+  int FD;
+  SmallString<64> TempPath;
+  ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));
+  FileRemover Cleanup(TempPath);
+
+  // Make sure it exists.
+  ASSERT_TRUE(sys::fs::exists(Twine(TempPath)));
+
+  ASSERT_NO_ERROR(fs::is_local(FD, Result));
+  EXPECT_TRUE(Result);
+  EXPECT_TRUE(fs::is_local(FD));
+}
+
 TEST_F(FileSystemTest, set_current_path) {
   SmallString<128> path;
 
   ASSERT_NO_ERROR(fs::current_path(path));
   ASSERT_NE(TestDirectory, path);
 
   struct RestorePath {
     SmallString<128> path;