Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Taylor R Campbell
> Date: Wed, 1 Apr 2020 07:42:53 -0700
> From: Jason Thorpe 
> 
> If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
> PAGE_SHIFT)?

Without running the following program, can you tell me what it will
print?

It might work to define PAGE_SIZE to be ((size_t)1 << PAGE_SHIFT) but
the consequences are not immediately clear to me (and might be tricky
on, e.g., i386pae).


#include 
#include 
#include 

#define PAGE_SIZE_S (1 << 12)
#define PAGE_SIZE_U (1u << 12)

uint64_taddr = 0x7fff01234567;

int
main(void)
{

printf("%"PRIx64"\n", addr & ~(PAGE_SIZE_S - 1));
printf("%"PRIx64"\n", addr & ~(PAGE_SIZE_U - 1));

fflush(stdout);
return ferror(stdout);
}


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas
I think that PAGESIZE is not always a constant in our code.

christos




signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Robert Elz
Date:Wed, 1 Apr 2020 16:31:01 +0200
From:Kamil Rytarowski 
Message-ID:  

  | Does it look better?
  |
  | http://netbsd.org/~kamil/patch-00244-fopsmapper-PAGE_SIZE.txt

Apart from it needing to be (expressed in the relevant
make syntax, whatever that is)

if (WARNS > 3) WARNE=3

rather than simply WARNS=3 then as a hack type fix that looks fine.

But this is really a gcc bug, we shouldn't be needing to change our
sources at all because of this.

The signed vs unsigned comparison warning is because if the signed number
happens to be negative, when treated as unsigned it will compare larger
(usually) than the unsigned value - whereas most people expect negative
numbers to be less than positive ones (whatever the magnitudes).   Warning
for that potential problem is entirely reasonable.

But here we have a known positive (though signed) constant.   It can never
magically become negative, and so the problem can never exist.  There should
not (ever) be a warning for this case, it is simply stupid, and we should
not need to modify either code, or makefiles, to make it go away.

kre




Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas

> If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
> PAGE_SHIFT)?

It will *probably* work unless we have 'if (negative_int > PAGESIZE)'
somewhere. I guess if you make the change and the kernel boots... :-)

christos


signature.asc
Description: Message signed with OpenPGP


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Jason Thorpe


> On Apr 1, 2020, at 7:17 AM, Christos Zoulas  wrote:
> 
> Which we have been slowly fixing. I think in this case the sign-compare
> warnings are annoying, but putting casts on each warning is cluttering
> the code needlessly. Unfortunately the alternative (to make the PAGESIZE
> constant unsigned is more dangerous -- unless of course you are willing to 
> examine all the places it is used and make sure that the semantics don't
> change). Another way would be to make:
> 
>#define PAGESIZE_U ((unsigned)PAGESIZE)

If PAGE_SIZE is ostensibly a vsize_t / size_t, why not define it as (1U << 
PAGE_SHIFT)?

-- thorpej



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 16:17, Christos Zoulas wrote:
> In article ,
> Paul Goyette   wrote:
>> On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>>
>>> On 01.04.2020 15:47, Robert Elz wrote:
 Date:Wed, 1 Apr 2020 11:45:53 +
 From:"Kamil Rytarowski" 
 Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

   | Log Message:
   | Avoid comparison between signed and unsigned integer
   |
   | Cast PAGE_SIZE to size_t.

 This kind of pedantry is going way too far, PAGE_SIZE is a compile
 time constant (1 << PAGE_SHIFT) which is an int (and so signed,
 nominally) but one which is known to be positive.

>>>
>>> I got reports that certain ports no longer build due to:
>>>
>>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>>> comparison between signed and unsigned integer expressions
>>> [-Werror=sign-compare]
>>>  if (size != PAGE_SIZE)
>>>   ^~
>>> cc1: all warnings being treated as errors
>>
>>
>> There's a lot of modules that fail for this with WARNS=5 when being
>> built as loadable modules.
>>
>> That's why so many of the individual module Makefiles have explicit
>> WARNS=4.  (It seems that when modules are built-in as part of the
>> kernel, they are by default built with WARNS=4.)
>
> Which we have been slowly fixing. I think in this case the sign-compare
> warnings are annoying, but putting casts on each warning is cluttering
> the code needlessly. Unfortunately the alternative (to make the PAGESIZE
> constant unsigned is more dangerous -- unless of course you are willing to
> examine all the places it is used and make sure that the semantics don't
> change). Another way would be to make:
>
> #define PAGESIZE_U ((unsigned)PAGESIZE)
>
> and use that instead; eventually when all instances of PAGESIZE have been
> converted to PAGESIZE_U it can be removed. But is it worth it? There are
> perhaps better things to do. But anyway you want to proceed should be
> discussed in tech-kern. Adding piecemeal casts everywhere does not make
> the world a better place.
>
> christos
>

Does it look better?

http://netbsd.org/~kamil/patch-00244-fopsmapper-PAGE_SIZE.txt

Perhaps we could switch PAGE_SIZE to unsigned.. but it is too much for now.


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Christos Zoulas
In article ,
Paul Goyette   wrote:
>On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>
>> On 01.04.2020 15:47, Robert Elz wrote:
>>> Date:Wed, 1 Apr 2020 11:45:53 +
>>> From:"Kamil Rytarowski" 
>>> Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>>>
>>>   | Log Message:
>>>   | Avoid comparison between signed and unsigned integer
>>>   |
>>>   | Cast PAGE_SIZE to size_t.
>>>
>>> This kind of pedantry is going way too far, PAGE_SIZE is a compile
>>> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
>>> nominally) but one which is known to be positive.
>>>
>>
>> I got reports that certain ports no longer build due to:
>>
>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>> comparison between signed and unsigned integer expressions
>> [-Werror=sign-compare]
>>  if (size != PAGE_SIZE)
>>   ^~
>> cc1: all warnings being treated as errors
>
>
>There's a lot of modules that fail for this with WARNS=5 when being
>built as loadable modules.
>
>That's why so many of the individual module Makefiles have explicit
>WARNS=4.  (It seems that when modules are built-in as part of the
>kernel, they are by default built with WARNS=4.)

Which we have been slowly fixing. I think in this case the sign-compare
warnings are annoying, but putting casts on each warning is cluttering
the code needlessly. Unfortunately the alternative (to make the PAGESIZE
constant unsigned is more dangerous -- unless of course you are willing to 
examine all the places it is used and make sure that the semantics don't
change). Another way would be to make:

#define PAGESIZE_U ((unsigned)PAGESIZE)

and use that instead; eventually when all instances of PAGESIZE have been
converted to PAGESIZE_U it can be removed. But is it worth it? There are
perhaps better things to do. But anyway you want to proceed should be
discussed in tech-kern. Adding piecemeal casts everywhere does not make
the world a better place.

christos



Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 16:10, Paul Goyette wrote:
> On Wed, 1 Apr 2020, Kamil Rytarowski wrote:
>
>> On 01.04.2020 15:47, Robert Elz wrote:
>>>     Date:    Wed, 1 Apr 2020 11:45:53 +
>>>     From:    "Kamil Rytarowski" 
>>>     Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>>>
>>>   | Log Message:
>>>   | Avoid comparison between signed and unsigned integer
>>>   |
>>>   | Cast PAGE_SIZE to size_t.
>>>
>>> This kind of pedantry is going way too far, PAGE_SIZE is a compile
>>> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
>>> nominally) but one which is known to be positive.
>>>
>>
>> I got reports that certain ports no longer build due to:
>>
>> src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
>> comparison between signed and unsigned integer expressions
>> [-Werror=sign-compare]
>>  if (size != PAGE_SIZE)
>>   ^~
>> cc1: all warnings being treated as errors
>
>
> There's a lot of modules that fail for this with WARNS=5 when being
> built as loadable modules.
>
> That's why so many of the individual module Makefiles have explicit
> WARNS=4.  (It seems that when modules are built-in as part of the
> kernel, they are by default built with WARNS=4.)
>

I tracked it to WARNS=3

# Rather than our usual WARNS=5, we need to use 3, since there are a
# lot of signed-vs-unsigned compares

WARNS=  3

Personally I have got no preference. I'm fine with an explicit cast as
it is in a single place only needed (hopefully).


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Paul Goyette

On Wed, 1 Apr 2020, Kamil Rytarowski wrote:


On 01.04.2020 15:47, Robert Elz wrote:

Date:Wed, 1 Apr 2020 11:45:53 +
From:"Kamil Rytarowski" 
Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

  | Log Message:
  | Avoid comparison between signed and unsigned integer
  |
  | Cast PAGE_SIZE to size_t.

This kind of pedantry is going way too far, PAGE_SIZE is a compile
time constant (1 << PAGE_SHIFT) which is an int (and so signed,
nominally) but one which is known to be positive.



I got reports that certain ports no longer build due to:

src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
 if (size != PAGE_SIZE)
  ^~
cc1: all warnings being treated as errors



There's a lot of modules that fail for this with WARNS=5 when being
built as loadable modules.

That's why so many of the individual module Makefiles have explicit
WARNS=4.  (It seems that when modules are built-in as part of the
kernel, they are by default built with WARNS=4.)



++--+---+
| Paul Goyette   | PGP Key fingerprint: | E-mail addresses: |
| (Retired)  | FA29 0E3B 35AF E8AE 6651 | p...@whooppee.com |
| Software Developer | 0786 F758 55DE 53BA 7731 | pgoye...@netbsd.org   |
++--+---+


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Kamil Rytarowski
On 01.04.2020 15:47, Robert Elz wrote:
> Date:Wed, 1 Apr 2020 11:45:53 +
> From:"Kamil Rytarowski" 
> Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>
>
>   | Log Message:
>   | Avoid comparison between signed and unsigned integer
>   |
>   | Cast PAGE_SIZE to size_t.
>
> This kind of pedantry is going way too far, PAGE_SIZE is a compile
> time constant (1 << PAGE_SHIFT) which is an int (and so signed,
> nominally) but one which is known to be positive.
>

I got reports that certain ports no longer build due to:

src/sys/modules/examples/fopsmapper/fopsmapper.c:118:11: error:
comparison between signed and unsigned integer expressions
[-Werror=sign-compare]
  if (size != PAGE_SIZE)
   ^~
cc1: all warnings being treated as errors


Re: CVS commit: src/sys/modules/examples/fopsmapper

2020-04-01 Thread Robert Elz
Date:Wed, 1 Apr 2020 11:45:53 +
From:"Kamil Rytarowski" 
Message-ID:  <20200401114554.05167f...@cvs.netbsd.org>

  | Log Message:
  | Avoid comparison between signed and unsigned integer
  |
  | Cast PAGE_SIZE to size_t.

This kind of pedantry is going way too far, PAGE_SIZE is a compile
time constant (1 << PAGE_SHIFT) which is an int (and so signed,
nominally) but one which is known to be positive.

What is to be next?  Given an unsigned var (any unsigned type) 'u'
are we going to be required to write

if (u != 0U)
or
if (u != (unsigned)0)

instead of just
if (u != 0)
?

Get rid of the cast, it isn't needed in this case,
and anything that believes it is, is wrong (broken).

kre