[PATCH] D42925: Call FlushFileBuffers on readwrite file mappings.

[Reid Kleckner via Phabricator via llvm-commits]

rnk accepted this revision.
rnk added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D42925#998357, @zturner wrote:

> In https://reviews.llvm.org/D42925#998354, @rnk wrote:
>
> > One concern I have is that this is a common pattern on Linux:
> >
> >   int fd = open(...);
> >   void *mem = mmap(fd, ...);
> >   close(fd);
> >   // use mem
> >   
> >
> > Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?
>
>
> We don't do that anywhere *at the moment* that I can find, but I suppose it's a concern for future proofing this code.  Someone could come along and add code like this after the fact.  Currently the only place where we open a `mapped_file_region` for readwrite is in `OnDiskBuffer`, which stores a `TempFile` and then renames it atomically to the final path.  We could put the flush logic inside of `TempFile::keep()` so that it flushes it before renaming.  That might be a better option, although it wouldn't catch the case where someone comes along later and tries to use a `mapped_file_region` for something else.  Perhaps it's ok for the time being though?


It's probably fine. I tried to look for this pattern, but got lazy and punted it to you.


https://reviews.llvm.org/D42925