Re: [PATCH] Cygwin: Fix error return for madvise()

Hi Mark,

On Tue, 7 Jul 2026 00:58:48 -0700
Mark Geisert wrote:
> Hi Takashi,
> 
> On 7/6/2026 5:45 PM, Takashi Yano wrote:
> > Hi Mark,
> > 
> > On Mon,  6 Jul 2026 16:47:43 -0700
> > Mark Geisert wrote:
> >> Currently madvise() and posix_madvise() are wired together as one
> >> function: the latter.  But their error returns should be different.
> >> Make madvise a first-class export in cygwin.din; code a new madvise()
> >> that calls posix_madvise() and massages any error return.
> >>
> >> Reported-by: Christian Franke <[email protected]>
> >> Addresses: https://cygwin.com/pipermail/cygwin/2026-July/259872.html
> >> Signed-off-by: Mark Geisert <[email protected]>
> >> Fixes: 61522196c715 (* Merge in cygwin-64bit-branch.)
> >>
> >> ---
> >>   winsup/cygwin/cygwin.din               |  2 +-
> >>   winsup/cygwin/include/cygwin/version.h |  3 ++-
> >>   winsup/cygwin/mm/mmap.cc               | 12 ++++++++++++
> >>   3 files changed, 15 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/winsup/cygwin/cygwin.din b/winsup/cygwin/cygwin.din
> >> index 2e53bc819..937eacdaf 100644
> >> --- a/winsup/cygwin/cygwin.din
> >> +++ b/winsup/cygwin/cygwin.din
> >> @@ -951,7 +951,7 @@ lseek SIGFE
> >>   lsetxattr SIGFE
> >>   lstat SIGFE
> >>   lutimes SIGFE
> >> -madvise = posix_madvise SIGFE
> >> +madvise SIGFE
> >>   makecontext NOSIGFE
> >>   mallinfo SIGFE
> >>   malloc SIGFE
> >> diff --git a/winsup/cygwin/include/cygwin/version.h b/winsup/cygwin/include/cygwin/version.h
> >> index 71ac5282b..fc838e23e 100644
> >> --- a/winsup/cygwin/include/cygwin/version.h
> >> +++ b/winsup/cygwin/include/cygwin/version.h
> >> @@ -502,12 +502,13 @@ details. */
> >>     360: Add RLIMIT_NPROC.
> >>     361: Export _Fork.
> >>     362: Export C23 stdbit functions.
> >> +  363: Export madvise separately from posix_madvise.
> >>   
> >>     Note that we forgot to bump the api for ualarm, strtoll, strtoull,
> >>     sigaltstack, sethostname. */
> >>   
> >>   #define CYGWIN_VERSION_API_MAJOR 0
> >> -#define CYGWIN_VERSION_API_MINOR 362
> >> +#define CYGWIN_VERSION_API_MINOR 363
> >>   
> >>   /* There is also a compatibity version number associated with the shared memory
> >>      regions.  It is incremented when incompatible changes are made to the shared
> > 
> > I don't think we should change CYGWIN_VERSION_API_MINOR value
> > because the API itself is not changed. This patch fixes a bug
> > in madvice() implementation.
> 
> I went back and forth internally on whether the minor version should be 
> bumped.  One point was whether divorcing madvise() from posix_madvise() 
> in cygwin.din warranted an API bump: without a bump won't existing 
> programs be unable to access the new error return behavior?  Another 
> point was that the error return behavior of madvise() is being changed; 
> isn't that behavior part of the API?  I agree this is a bug fix but such 
> fixes could cause API changes.

I could not found the case that API version bump for behavioral change
in the past.

The adding MACROs such as:
  148: Add open(2) flags O_SYNC, O_RSYNC, O_DSYNC and O_DIRECT.
  149: Add open(2) flag O_NOFOLLOW.
could be a 'behavioral change', but header files for user were also
changed in these cases.

> This is a case where I'd like to get input from other folks who might've 
> made similar changes to Cygwin over its long history.  I am totally OK 
> with removing the API bump in a v2 patch if that's the consensus opinion.

What do others think about this?

> Thank you for the review!
> 
> ..mark
> 


-- 
Takashi Yano <[email protected]>