[PATCH] Cygwin: mmap: Remove AT_ROUND_TO_PAGE workaround

Corinna Vinschen corinna-cygwin@cygwin.com
Tue Jul 21 07:35:59 GMT 2020


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 <corinna@vinschen.de>
> > [...]
> > @@ -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


More information about the Cygwin-patches mailing list