[PATCH] D95494: Support: Add mapped_file_region::sync(), equivalent to msync

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

> I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.

Then I think this change would be fine.

But I'm not sure the test is testing that.

On Windows, there are three kinds of objects at play:  the file, the mapping, and the view.  I don't know the Posix model well enough to know how this corresponds.  I also don't know all the relevant details behind `fs::mapped_file_region` and `MemoryBuffer`.

On Windows, two views derived from the same file mapping will be coherent--even across processes--without user syncing.  Furthermore, all views ultimately backed by the same (non-remote) file will also be coherent.  In the test, the second "user" of the file is a MemoryBuffer.  Since MemoryBuffer is also opening the file for memory mapping (I believe), it seems its views are going to be coherent regardless of whether you do a manual sync.  Thus the test might always pass, regardless of whether your function is correct.

What's not necessarily coherent is regular file i/o with a file that's also using memory mapped i/o.  That's the case where you would definitely need a manual sync.  This is rare in my experience since Microsoft recommends opening a file in exclusive mode if you're going to use memory mapped i/o.

Although the docs say you can close views and mappings in any order, I've only seen it done in strictly reverse order.  And I'm not sure you can close the file itself while mappings still exist.  I assume that would just decrement a reference count and the file would remain open until the last mapping object goes away.  Compounding this uncertainty is that the test calls `close` on the file descriptor rather than the file object itself.

So my recommendations are (1) that the test be written so that one of the accesses is via memory mapped i/o and the other is through file i/o, and (2) that the test be shown to fail without the msync call.


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

https://reviews.llvm.org/D95494