Re: Add PAGE_SIZE, PAGE_SHIFT, PAGE_MASK to sys/param.h

[Nicholas Wourms <nwourms at netscape dot net>]

cygwin-patches@cygwin.com wrote:
[SNIP]
> > 2003-10-28  Nicholas Wourms  <nwourms@netscape.net>
> >
> >    * include/sys/param.h: Define some page counting macros.
> >    (PAGE_SHIFT): Define.
> >    (PAGE_SIZE): Define.
> >    (PAGE_MASK): Define.
> >    Tidy tab/whitespace formatting from last patch.
>
>
> Sorry, but I have several problems with this patch:
>
> - The formatting of the ChangeLog entry (no TABS).

That proves my later concerns, as I did put TABS in the ChangeLog entry. 
 Unfortunately, this was a bug in Mozilla about the time NS-7.1 was 
released against that code base (Those on the LKML will recall the 
problems ppl had with Mozilla stripping tabs).  See my later comments on 
this.

> - The ChangeLog and your above description are missing the fact, that
>   you also added a NBPW definition.

D'oh, sorry I forgot about that one.

> - The definition of PAGE_MASK is... a problem.  Your definition is the
>   BSD definition (PAGE_SIZE-1), while Linux defines it as the negation
>   thereof, (~(PAGE_SIZE-1)).  I don't know what way to follow here.
>   I guess it's all one, considering that we don't use it in Cygwin so
>   far.  While we once decided that, if SUSv3 fails to lend us a hand,
>   we would try to map the Linux behaviour, the sys/param.h file is
>   a header for mostly BSD definitions.

I know, but I couldn't find this defined like that in any other OS.  I 
felt guilty enough by casting the bitvector, I was worried about be 
"accused" of stealing GPL'ed code.  Thus I thought it better to stick 
with the BSD definition.  Interestingly enough, Wine's 
`memory/virtual.c' has `PAGE_SHIFT' defined to `12' and PAGE_MASK 
defined to 0xfff or 4095 (4096-1).  log2(4096) gives a float answer of 
12.  Also Doug Lea's malloc defines PAGE_MASK as (PAGE_SIZE-1) and then 
negates it where necessary.  OTOH, I found that dlltool from 
binutils-2.7 used to define it as a negation.  Since you are more of an 
expert on mmap then I, I'll leave it others to decide.  If you want to 
leave it out for now, that would be ok, too.  Primarily, I was after 
PAGE_SHIFT & PAGE_SIZE but decided to add PAGE_MASK since it was 
clustered with the others in all the headers I looked at.

> - Neither BSD nor Linux define these highly machine dependent values
>   in sys/param.h.  What about adding a asm/param.h file and include
>   that in sys/param.h?

I wasn't sure what to do, so if you think asm/param.h is better, that 
will be fine.  Of course bsd/linux has it in a machine dependent asm 
dirs, so now that I think about it that would make sense.

> - Don't attach gzipped patches, please.  Mozilla doesn't scramble
>   text attachments, AFAIK.

Unfortunately, as stated previously, it seems that NS-7.1 does.  I have 
to use this to read my netscape mail if I don't want to use the web 
interface.  However, I'll resend the patch with your suggested 
improvements using pine to see if that works.

Cheers,
Nicholas