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 <[email protected]>
> > [...]
> > @@ -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
