[PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround
Ken Brown
kbrown@cornell.edu
Mon Jul 20 21:26:32 GMT 2020
Hi Corinna,
On 7/20/2020 2:55 PM, Corinna Vinschen wrote:
> From: Corinna Vinschen <corinna@vinschen.de>
>
> It's working on 32 bit OSes only anyway. It even fails on WOW64.
>
> Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
> ---
>
> Notes:
> Hi Ken,
>
> can you please review this patch and check if it doesn't break
> your testcase again?
>
> Thanks,
> Corinna
>
> winsup/cygwin/mmap.cc | 117 ++++++++++++------------------------------
> 1 file changed, 34 insertions(+), 83 deletions(-)
>
> diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
> index 1fccc6c58ee9..8ac96606c2e6 100644
> --- a/winsup/cygwin/mmap.cc
> +++ b/winsup/cygwin/mmap.cc
> @@ -195,12 +195,7 @@ MapView (HANDLE h, void *addr, size_t len, DWORD openflags,
> DWORD protect = gen_create_protect (openflags, flags);
> void *base = addr;
> SIZE_T viewsize = len;
> -#ifdef __x86_64__ /* AT_ROUND_TO_PAGE isn't supported on 64 bit systems. */
> ULONG alloc_type = MEM_TOP_DOWN;
> -#else
> - ULONG alloc_type = (base && !wincap.is_wow64 () ? AT_ROUND_TO_PAGE : 0)
> - | MEM_TOP_DOWN;
> -#endif
>
> #ifdef __x86_64__
> /* Don't call NtMapViewOfSectionEx during fork. It requires autoloading
> @@ -878,6 +873,10 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
>
> if (!anonymous (flags) && fd != -1)
> {
> + UNICODE_STRING fname;
> + IO_STATUS_BLOCK io;
> + FILE_STANDARD_INFORMATION fsi;
> +
> /* Ensure that fd is open */
> cygheap_fdget cfd (fd);
> if (cfd < 0)
> @@ -896,19 +895,16 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
>
> /* The autoconf mmap test maps a file of size 1 byte. It then tests
> every byte of the entire mapped page of 64K for 0-bytes since that's
> - what POSIX requires. The problem is, we can't create that mapping on
> - 64 bit systems. The file mapping will be only a single page, 4K, and
> - since 64 bit systems don't support the AT_ROUND_TO_PAGE flag, the
> - remainder of the 64K slot will result in a SEGV when accessed.
> -
> - So, what we do here is cheating for the sake of the autoconf test
> - on 64 bit systems. The justification is that there's very likely
> - no application actually utilizing the map beyond EOF, and we know that
> - all bytes beyond EOF are set to 0 anyway. If this test doesn't work
> - on 64 bit systems, it will result in not using mmap at all in a
> - package. But we want that mmap is treated as usable by autoconf,
> - regardless whether the autoconf test runs on a 32 bit or a 64 bit
> - system.
> + what POSIX requires. The problem is, we can't create that mapping.
> + The file mapping will be only a single page, 4K, and the remainder
> + of the 64K slot will result in a SEGV when accessed.
> +
> + So, what we do here is cheating for the sake of the autoconf test.
> + The justification is that there's very likely no application actually
> + utilizing the map beyond EOF, and we know that all bytes beyond EOF
> + are set to 0 anyway. If this test doesn't work, it will result in
> + not using mmap at all in a package. But we want mmap being treated
> + as usable by autoconf.
>
> Ok, so we know exactly what autoconf is doing. The file is called
> "conftest.txt", it has a size of 1 byte, the mapping size is the
> @@ -916,31 +912,19 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
> mapping is MAP_SHARED, the offset is 0.
>
> If all these requirements are given, we just return an anonymous map.
> - This will help to get over the autoconf test even on 64 bit systems.
> The tests are ordered for speed. */
> -#ifdef __x86_64__
> - if (1)
> -#else
> - if (wincap.is_wow64 ())
> -#endif
> - {
> - UNICODE_STRING fname;
> - IO_STATUS_BLOCK io;
> - FILE_STANDARD_INFORMATION fsi;
> -
> - if (len == pagesize
> - && prot == (PROT_READ | PROT_WRITE)
> - && flags == MAP_SHARED
> - && off == 0
> - && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
> - &fname),
> - wcscmp (fname.Buffer, L"conftest.txt") == 0)
> - && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), &io,
> - &fsi, sizeof fsi,
> - FileStandardInformation))
> - && fsi.EndOfFile.QuadPart == 1LL)
> - flags |= MAP_ANONYMOUS;
> - }
> + if (len == pagesize
> + && prot == (PROT_READ | PROT_WRITE)
> + && flags == MAP_SHARED
> + && off == 0
> + && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
> + &fname),
> + wcscmp (fname.Buffer, L"conftest.txt") == 0)
> + && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), &io,
> + &fsi, sizeof fsi,
> + FileStandardInformation))
> + && fsi.EndOfFile.QuadPart == 1LL)
> + flags |= MAP_ANONYMOUS;
> }
>
> if (anonymous (flags) || fd == -1)
> @@ -1089,6 +1073,7 @@ go_ahead:
> }
>
> #ifdef __x86_64__
> + orig_len = roundup2 (orig_len, pagesize);
> if (!wincap.has_extended_mem_api ())
> addr = mmap_alloc.alloc (addr, orig_len ?: len, fixed (flags));
> #else
> @@ -1099,7 +1084,6 @@ go_ahead:
> deallocated and the address we got is used as base address for the
> subsequent real mappings. This ensures that we have enough space
> for the whole thing. */
> - orig_len = roundup2 (orig_len, pagesize);
> PVOID newaddr = VirtualAlloc (addr, orig_len, MEM_TOP_DOWN | MEM_RESERVE,
> PAGE_READWRITE);
> if (!newaddr)
> @@ -1132,51 +1116,18 @@ go_ahead:
> if (orig_len)
> {
> /* If the requested length is bigger than the file size, the
> - remainder is created as anonymous mapping. Actually two
> - mappings are created, first the remainder from the file end to
> - the next 64K boundary as accessible pages with the same
> - protection as the file's pages, then as much pages as necessary
> - to accomodate the requested length, but as reserved pages which
> - raise a SIGBUS when trying to access them. AT_ROUND_TO_PAGE
> - and page protection on shared pages is only supported by the
> - 32 bit environment, so don't even try on 64 bit or even WOW64.
> - This results in an allocation gap in the first 64K block the file
> - ends in, but there's nothing at all we can do about that. */
> -#ifdef __x86_64__
> - len = roundup2 (len, wincap.allocation_granularity ());
> - orig_len = roundup2 (orig_len, wincap.allocation_granularity ());
> -#else
> - len = roundup2 (len, wincap.is_wow64 () ? wincap.allocation_granularity ()
> - : wincap.page_size ());
> -#endif
> + remainder is created as anonymous mapping, as reserved pages which
> + raise a SIGBUS when trying to access them. This results in an
> + allocation gap in the first 64K block the file ends in, but there's
> + nothing at all we can do about that. */
> + len = roundup2 (len, pagesize);
> if (orig_len - len)
> {
> - orig_len -= len;
> - size_t valid_page_len = 0;
> -#ifndef __x86_64__
> - if (!wincap.is_wow64 ())
> - valid_page_len = orig_len % pagesize;
> -#endif
> - size_t sigbus_page_len = orig_len - valid_page_len;
> + size_t sigbus_page_len = orig_len - len;
>
> - caddr_t at_base = base + len;
> - if (valid_page_len)
> - {
> - prot |= __PROT_FILLER;
> - flags &= MAP_SHARED | MAP_PRIVATE;
> - flags |= MAP_ANONYMOUS | MAP_FIXED;
> - at_base = mmap_worker (NULL, &fh_anonymous, at_base,
> - valid_page_len, prot, flags, -1, 0, NULL);
> - if (!at_base)
> - {
> - fh->munmap (fh->get_handle (), base, len);
> - set_errno (ENOMEM);
> - goto out_with_unlock;
> - }
> - at_base += valid_page_len;
> - }
> if (sigbus_page_len)
> {
> + caddr_t at_base = base + len;
> prot = PROT_READ | PROT_WRITE | __PROT_ATTACH;
> flags = MAP_ANONYMOUS | MAP_NORESERVE | MAP_FIXED;
> at_base = mmap_worker (NULL, &fh_anonymous, at_base,
I think you still left in some 32 bit code that should be removed, and also
orig_len now doesn't get rounded up on 32 bit. Here's an additional diff that I
think is needed beyond your patch:
diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
index 8ac96606c..fa9266825 100644
--- a/winsup/cygwin/mmap.cc
+++ b/winsup/cygwin/mmap.cc
@@ -1009,20 +1009,8 @@ mmap64 (void *addr, size_t len, int prot, int flags, int
fd, off_t off)
goto go_ahead;
}
fsiz -= off;
- /* We're creating the pages beyond EOF as reserved, anonymous pages.
- Note that 64 bit environments don't support the AT_ROUND_TO_PAGE
- flag, which is required to get this right for the remainder of
- the first 64K block the file ends in. We perform the workaround
- nevertheless to support expectations that the range mapped beyond
- EOF can be safely munmap'ed instead of being taken by another,
- totally unrelated mapping. */
- if ((off_t) len > fsiz && !autogrow (flags))
- orig_len = len;
-#ifdef __i386__
- else if (!wincap.is_wow64 () && roundup2 (len, wincap.page_size ())
- < roundup2 (len, pagesize))
- orig_len = len;
-#endif
+ /* We're creating the pages beyond EOF as reserved, anonymous
+ pages if MAP_AUTOGROW is not set. */
if ((off_t) len > fsiz)
{
if (autogrow (flags))
@@ -1037,9 +1025,12 @@ mmap64 (void *addr, size_t len, int prot, int flags, int
fd, off_t off)
}
}
else
- /* Otherwise, don't map beyond EOF, since Windows would change
- the file to the new length, in contrast to POSIX. */
- len = fsiz;
+ {
+ /* Otherwise, don't map beyond EOF, since Windows would change
+ the file to the new length, in contrast to POSIX. */
+ orig_len = len;
+ len = fsiz;
+ }
}
/* If the requested offset + len is <= file size, drop MAP_AUTOGROW.
@@ -1072,8 +1063,8 @@ go_ahead:
}
}
-#ifdef __x86_64__
orig_len = roundup2 (orig_len, pagesize);
+#ifdef __x86_64__
if (!wincap.has_extended_mem_api ())
addr = mmap_alloc.alloc (addr, orig_len ?: len, fixed (flags));
#else
I'm attaching an amended commit.
I could easily have missed something, and I don't have a 32 bit OS to test on,
so just ignore my changes if I'm wrong.
But I've retested the php test case, and it's still OK with this patch.
Ken
-------------- next part --------------
>From 6c26194304efa42394fdb509a94d618931ba8279 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen <corinna@vinschen.de>
Date: Mon, 20 Jul 2020 20:55:43 +0200
Subject: [PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround
It's working on 32 bit OSes only anyway. It even fails on WOW64.
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
---
winsup/cygwin/mmap.cc | 142 +++++++++++++-----------------------------
1 file changed, 42 insertions(+), 100 deletions(-)
diff --git a/winsup/cygwin/mmap.cc b/winsup/cygwin/mmap.cc
index 1fccc6c58..fa9266825 100644
--- a/winsup/cygwin/mmap.cc
+++ b/winsup/cygwin/mmap.cc
@@ -195,12 +195,7 @@ MapView (HANDLE h, void *addr, size_t len, DWORD openflags,
DWORD protect = gen_create_protect (openflags, flags);
void *base = addr;
SIZE_T viewsize = len;
-#ifdef __x86_64__ /* AT_ROUND_TO_PAGE isn't supported on 64 bit systems. */
ULONG alloc_type = MEM_TOP_DOWN;
-#else
- ULONG alloc_type = (base && !wincap.is_wow64 () ? AT_ROUND_TO_PAGE : 0)
- | MEM_TOP_DOWN;
-#endif
#ifdef __x86_64__
/* Don't call NtMapViewOfSectionEx during fork. It requires autoloading
@@ -878,6 +873,10 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
if (!anonymous (flags) && fd != -1)
{
+ UNICODE_STRING fname;
+ IO_STATUS_BLOCK io;
+ FILE_STANDARD_INFORMATION fsi;
+
/* Ensure that fd is open */
cygheap_fdget cfd (fd);
if (cfd < 0)
@@ -896,19 +895,16 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
/* The autoconf mmap test maps a file of size 1 byte. It then tests
every byte of the entire mapped page of 64K for 0-bytes since that's
- what POSIX requires. The problem is, we can't create that mapping on
- 64 bit systems. The file mapping will be only a single page, 4K, and
- since 64 bit systems don't support the AT_ROUND_TO_PAGE flag, the
- remainder of the 64K slot will result in a SEGV when accessed.
-
- So, what we do here is cheating for the sake of the autoconf test
- on 64 bit systems. The justification is that there's very likely
- no application actually utilizing the map beyond EOF, and we know that
- all bytes beyond EOF are set to 0 anyway. If this test doesn't work
- on 64 bit systems, it will result in not using mmap at all in a
- package. But we want that mmap is treated as usable by autoconf,
- regardless whether the autoconf test runs on a 32 bit or a 64 bit
- system.
+ what POSIX requires. The problem is, we can't create that mapping.
+ The file mapping will be only a single page, 4K, and the remainder
+ of the 64K slot will result in a SEGV when accessed.
+
+ So, what we do here is cheating for the sake of the autoconf test.
+ The justification is that there's very likely no application actually
+ utilizing the map beyond EOF, and we know that all bytes beyond EOF
+ are set to 0 anyway. If this test doesn't work, it will result in
+ not using mmap at all in a package. But we want mmap being treated
+ as usable by autoconf.
Ok, so we know exactly what autoconf is doing. The file is called
"conftest.txt", it has a size of 1 byte, the mapping size is the
@@ -916,31 +912,19 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
mapping is MAP_SHARED, the offset is 0.
If all these requirements are given, we just return an anonymous map.
- This will help to get over the autoconf test even on 64 bit systems.
The tests are ordered for speed. */
-#ifdef __x86_64__
- if (1)
-#else
- if (wincap.is_wow64 ())
-#endif
- {
- UNICODE_STRING fname;
- IO_STATUS_BLOCK io;
- FILE_STANDARD_INFORMATION fsi;
-
- if (len == pagesize
- && prot == (PROT_READ | PROT_WRITE)
- && flags == MAP_SHARED
- && off == 0
- && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
- &fname),
- wcscmp (fname.Buffer, L"conftest.txt") == 0)
- && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), &io,
- &fsi, sizeof fsi,
- FileStandardInformation))
- && fsi.EndOfFile.QuadPart == 1LL)
- flags |= MAP_ANONYMOUS;
- }
+ if (len == pagesize
+ && prot == (PROT_READ | PROT_WRITE)
+ && flags == MAP_SHARED
+ && off == 0
+ && (RtlSplitUnicodePath (fh->pc.get_nt_native_path (), NULL,
+ &fname),
+ wcscmp (fname.Buffer, L"conftest.txt") == 0)
+ && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), &io,
+ &fsi, sizeof fsi,
+ FileStandardInformation))
+ && fsi.EndOfFile.QuadPart == 1LL)
+ flags |= MAP_ANONYMOUS;
}
if (anonymous (flags) || fd == -1)
@@ -1025,20 +1009,8 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
goto go_ahead;
}
fsiz -= off;
- /* We're creating the pages beyond EOF as reserved, anonymous pages.
- Note that 64 bit environments don't support the AT_ROUND_TO_PAGE
- flag, which is required to get this right for the remainder of
- the first 64K block the file ends in. We perform the workaround
- nevertheless to support expectations that the range mapped beyond
- EOF can be safely munmap'ed instead of being taken by another,
- totally unrelated mapping. */
- if ((off_t) len > fsiz && !autogrow (flags))
- orig_len = len;
-#ifdef __i386__
- else if (!wincap.is_wow64 () && roundup2 (len, wincap.page_size ())
- < roundup2 (len, pagesize))
- orig_len = len;
-#endif
+ /* We're creating the pages beyond EOF as reserved, anonymous
+ pages if MAP_AUTOGROW is not set. */
if ((off_t) len > fsiz)
{
if (autogrow (flags))
@@ -1053,9 +1025,12 @@ mmap64 (void *addr, size_t len, int prot, int flags, int fd, off_t off)
}
}
else
- /* Otherwise, don't map beyond EOF, since Windows would change
- the file to the new length, in contrast to POSIX. */
- len = fsiz;
+ {
+ /* Otherwise, don't map beyond EOF, since Windows would change
+ the file to the new length, in contrast to POSIX. */
+ orig_len = len;
+ len = fsiz;
+ }
}
/* If the requested offset + len is <= file size, drop MAP_AUTOGROW.
@@ -1088,6 +1063,7 @@ go_ahead:
}
}
+ orig_len = roundup2 (orig_len, pagesize);
#ifdef __x86_64__
if (!wincap.has_extended_mem_api ())
addr = mmap_alloc.alloc (addr, orig_len ?: len, fixed (flags));
@@ -1099,7 +1075,6 @@ go_ahead:
deallocated and the address we got is used as base address for the
subsequent real mappings. This ensures that we have enough space
for the whole thing. */
- orig_len = roundup2 (orig_len, pagesize);
PVOID newaddr = VirtualAlloc (addr, orig_len, MEM_TOP_DOWN | MEM_RESERVE,
PAGE_READWRITE);
if (!newaddr)
@@ -1132,51 +1107,18 @@ go_ahead:
if (orig_len)
{
/* If the requested length is bigger than the file size, the
- remainder is created as anonymous mapping. Actually two
- mappings are created, first the remainder from the file end to
- the next 64K boundary as accessible pages with the same
- protection as the file's pages, then as much pages as necessary
- to accomodate the requested length, but as reserved pages which
- raise a SIGBUS when trying to access them. AT_ROUND_TO_PAGE
- and page protection on shared pages is only supported by the
- 32 bit environment, so don't even try on 64 bit or even WOW64.
- This results in an allocation gap in the first 64K block the file
- ends in, but there's nothing at all we can do about that. */
-#ifdef __x86_64__
- len = roundup2 (len, wincap.allocation_granularity ());
- orig_len = roundup2 (orig_len, wincap.allocation_granularity ());
-#else
- len = roundup2 (len, wincap.is_wow64 () ? wincap.allocation_granularity ()
- : wincap.page_size ());
-#endif
+ remainder is created as anonymous mapping, as reserved pages which
+ raise a SIGBUS when trying to access them. This results in an
+ allocation gap in the first 64K block the file ends in, but there's
+ nothing at all we can do about that. */
+ len = roundup2 (len, pagesize);
if (orig_len - len)
{
- orig_len -= len;
- size_t valid_page_len = 0;
-#ifndef __x86_64__
- if (!wincap.is_wow64 ())
- valid_page_len = orig_len % pagesize;
-#endif
- size_t sigbus_page_len = orig_len - valid_page_len;
+ size_t sigbus_page_len = orig_len - len;
- caddr_t at_base = base + len;
- if (valid_page_len)
- {
- prot |= __PROT_FILLER;
- flags &= MAP_SHARED | MAP_PRIVATE;
- flags |= MAP_ANONYMOUS | MAP_FIXED;
- at_base = mmap_worker (NULL, &fh_anonymous, at_base,
- valid_page_len, prot, flags, -1, 0, NULL);
- if (!at_base)
- {
- fh->munmap (fh->get_handle (), base, len);
- set_errno (ENOMEM);
- goto out_with_unlock;
- }
- at_base += valid_page_len;
- }
if (sigbus_page_len)
{
+ caddr_t at_base = base + len;
prot = PROT_READ | PROT_WRITE | __PROT_ATTACH;
flags = MAP_ANONYMOUS | MAP_NORESERVE | MAP_FIXED;
at_base = mmap_worker (NULL, &fh_anonymous, at_base,
--
2.27.0
More information about the Cygwin-patches
mailing list