Re: [PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround
Hi Ken,
On Jul 20 17:26, Ken Brown via Cygwin-patches wrote:
> Hi Corinna,
>
> On 7/20/2020 2:55 PM, Corinna Vinschen wrote:
> > From: Corinna Vinschen
> > [...]
> > @@ -1089,6 +1073,7 @@ go_ahead:
> > }
> > #ifdef __x86_64__
> > + orig_len = roundup2 (orig_len, pagesize);
Urgh, this line was supposed to go *outside* the #ifdef bracket. Duh!
Thanks for catching.
> > if (!wincap.has_extended_mem_api ())
> > addr = mmap_alloc.alloc (addr, orig_len ?: len, fixed (flags));
> > #else
> > [...]
>
> 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;
> + }
Oh, yes, that also simplifies the logic, great!
> }
>
>/* 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
Thanks a lot for checking and the attached full patch!
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Re: [PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround
eating 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
>From 6c26194304efa42394fdb509a94d618931ba8279 Mon Sep 17 00:00:00 2001
From: Corinna Vinschen
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
---
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__
[PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround
From: Corinna Vinschen
It's working on 32 bit OSes only anyway. It even fails on WOW64.
Signed-off-by: Corinna Vinschen
---
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,
- ),
- wcscmp (fname.Buffer, L"conftest.txt") == 0)
- && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), ,
-, 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,
+ ),
+ wcscmp (fname.Buffer, L"conftest.txt") == 0)
+ && NT_SUCCESS (NtQueryInformationFile (fh->get_handle (), ,
+, sizeof fsi,
+FileStandardInformation))
+ &&
