Re: [IDEA+RFC] Possible solution for min()/max() war

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

>   if sizeof(typeof(a)) != sizeof(typeof(b))
>       BUG() // sizes differ

this is not neccessarily a problem. should work with char/short char/int
short/int comparison.

only problem seems to be signed/unsigned int comparison.

>   const (typeof(a)) _a = ~(typeof(a))0
>   const (typeof(b)) _b = ~(typeof(b))0
>   if _a < 0 && _b > 0 || _a > 0 && b < 0
>       BUG() // one signed, the other unsigned
>   standard_max(a,b)

if sizeof(typeof(a))==sizeof(int) && sizeof(typeof(b))==sizeof(int) &&
   ( _a < 0 && _b > 0 || _a > 0 && b < 0 )
	BUG() // signed unsigned int compare

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Herbert Rosmanith wrote:"
> 
> >   if sizeof(typeof(a)) != sizeof(typeof(b))
> >       BUG() // sizes differ
> 
> this is not neccessarily a problem. should work with char/short char/int
> short/int comparison.

I got the impression that linus wanted two opposite things:

   1) every possible bug made to scream out loud until the author
      paid special attention to it and put it to bed with a 
      soothing declaration

   2) completely bona fide promotions made to go silent and not
      warn at all.

I don't see how he can really see 3arg_min/max as satisfying (2), since
the author will have to look at each and every existing min/max to
decide on a type with which to frontload the new min/max.  So I felt
justified in mostly ignoring (2).

On the other hand, he did say something about -Wsignedstuff being too
verbose, so I'm not sure. Maybe the -W really does make noise about
some compares that wouldn't be flagged by the  two constraints I
suggested? If so, perhaps that was what he wanted: a weaker and
more closely controlled -Wsignedgunggg.

> only problem seems to be signed/unsigned int comparison.

I don't know how you know that! You could be right - I don't know.
I haven't seen a tight enough specification of the basic problem
for me to be able to begin to formulate a generic solution. I've just
seen some examples, some more convincing than others.

> >   const (typeof(a)) _a = ~(typeof(a))0
> >   const (typeof(b)) _b = ~(typeof(b))0
> >   if _a < 0 && _b > 0 || _a > 0 && b < 0
> >       BUG() // one signed, the other unsigned
> >   standard_max(a,b)
> 
> if sizeof(typeof(a))==sizeof(int) && sizeof(typeof(b))==sizeof(int) &&
>    ( _a < 0 && _b > 0 || _a > 0 && b < 0 )
> 	BUG() // signed unsigned int compare

What makes sizeof(int) so magic? Are you saying that the problems
don't arise between signed long long and unsigned long long? I saw
an example that seemed generic for all signed unsigned comparisons,
namely that signed int and unsigned int are compared as unsigned, so 
(unsigned)2 < (signed)-1.  Are you saying that signed long and unsigned
long are NOT compared as unsigned iff sizeof(long) != sizeof(int).
Woooo!  Who wrote the C spec? No kidding? There is a special clause
for comparisons that use the native machine word?

And I was hoping that somebody could produce some gcc magic
replacement for BUG() that means "don't compile me". Perhaps
a bit of illegal assembler code with a line reference in?
Surely gcc must have something like __builtin_wont_compile()?
It just needs to be a leaf of the RTL that evokes a compile error.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

ptb@it.uc3m.es said:
>  And I was hoping that somebody could produce some gcc magic
> replacement for BUG() that means "don't compile me". Perhaps a bit of
> illegal assembler code with a line reference in? Surely gcc must have
> something like __builtin_wont_compile()? It just needs to be a leaf of
> the RTL that evokes a compile error.

It's done. I believe it was called __builtin_ct_assertion(). I don't think 
it got merged (yet?).


--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"David Woodhouse wrote:"
> 
> ptb@it.uc3m.es said:
> >  And I was hoping that somebody could produce some gcc magic
> > replacement for BUG() that means "don't compile me". Perhaps a bit of
> > illegal assembler code with a line reference in? Surely gcc must have
> > something like __builtin_wont_compile()? It just needs to be a leaf of
> > the RTL that evokes a compile error.
> 
> It's done. I believe it was called __builtin_ct_assertion(). I don't think 
> it got merged (yet?).

Now I think of it, I suppose

    unsafe_min_or_max_at_line_##__LINE__()

will definitely evoke a meaningful link error.

I still suspect that illegal assembler will do the job, since it must
be treated after gcc has produced assembler itself and line references
must still be present then for the assembler to be able to give meaningful
error messages (;), but assembler is not something I write, so someone
else needs to say.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

ptb@it.uc3m.es said:
> Now I think of it, I suppose
>     unsafe_min_or_max_at_line_##__LINE__()
> will definitely evoke a meaningful link error.

Unfortunately, there's no guarantee that it won't emit a reference to that 
symbol even if the test for mismatching arguments can be proved at compile 
time to always evaluate to false.

Code which relies on "if(0) __call_nonexistent_function();" actually compiling 
is just broken.

You'd have thought we'd have learned by now to stop relying on the observed 
current behaviour of gcc and start trying to get it right, wouldn't you?

The answer in this case is that gcc can't safely do what we require for this
and for other compile-time checks, until something like David's
__builtin_ct_assertion() is added.

--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago David Woodhouse wrote:"
> Code which relies on "if(0) __call_nonexistent_function();" actually compiling 
> is just broken.

You got me curious enough to try it.  It compiles and links fine with
-O1 and higher under

       gcc version 2.95.2 20000220 (Debian GNU/Linux)
       gcc version 2.8.1
       gcc version 2.7.2.3

> You'd have thought we'd have learned by now to stop relying on the observed 
> current behaviour of gcc and start trying to get it right, wouldn't you?

One CAN rely on this behaviour so long as branch reduction (well,
whatever it's called) is an optimizing step following constant
expression evaluation.  The fn call will never make it outside of gcc's
internal repreentation.

> The answer in this case is that gcc can't safely do what we require for this
> and for other compile-time checks, until something like David's
> __builtin_ct_assertion() is added.


Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

ptb@it.uc3m.es said:
>  You got me curious enough to try it.  It compiles and links fine with
> -O1 and higher under

>        gcc version 2.95.2 20000220 (Debian GNU/Linux)
>        gcc version 2.8.1
>        gcc version 2.7.2.3 

Oh well, then it _must_ be safe then - gcc has never changed unspecified 
behaviour on us before, has it?

The gcc engineer who took one look at the __buggy_udelay cruft, raised his
eyebrows, swore and wandered off muttering must just have been having a bad
day or something.

--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago David Woodhouse wrote:"
> 
> ptb@it.uc3m.es said:
> >  You got me curious enough to try it.  It compiles and links fine with
> > -O1 and higher under
> 
> >        gcc version 2.95.2 20000220 (Debian GNU/Linux)
> >        gcc version 2.8.1
> >        gcc version 2.7.2.3 
> 
> Oh well, then it _must_ be safe then - gcc has never changed unspecified 
> behaviour on us before, has it?

Well, I understand what you mean, but if the linux kernel wants it
and the C spec doesn't forbid it, then it'll either stay that way
or an "official" way will be found of evoking the desired behaviour.

The kernel already relies on -O1 expanding outb().

> The gcc engineer who took one look at the __buggy_udelay cruft, raised his
> eyebrows, swore and wandered off muttering must just have been having a bad
> day or something.

Actually, I was a bit more worried that 

  const unsigned int i = 1;
  if (i < 0)
      foo();

would generate warnings about the comparison always failing. But it
doesn't, at least not with -Wall. It does generate a warning about
implicitly declaring the function foo()! So I guess one has to add a
decl for it just above the call.  But it doesn't emit code for the call
itself, so the link is fine.

OTOH I now can't get #__LINE__ to expand as I want it where I want it.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

ptb@it.uc3m.es said:
>  Well, I understand what you mean, but if the linux kernel wants it
> and the C spec doesn't forbid it, then it'll either stay that way or
> an "official" way will be found of evoking the desired behaviour. 

In the world I've been living in for the last few years, GCC people don't
seem to take that much care to avoid breaking the kernel, especially when
the kernel is being gratuitously broken. David's __builtin_ct_assertion()
stuff looks like a sane way of getting the desired behaviour without having
to sacrifice any chickens, and getting that merged into GCC so that we can 
start to use it seems like a good idea.

>  OTOH I now can't get #__LINE__ to expand as I want it where I want
> it. 

Heh. That's magic.

--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 30, 2001 11:56 pm, Peter T. Breuer wrote:
> One CAN rely on this behaviour so long as branch reduction (well,
> whatever it's called) is an optimizing step following constant
> expression evaluation.

"Dead code removal"

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[Mark Zealey]

On Thu, Aug 30, 2001 at 11:32:55PM +0200, Peter T. Breuer wrote:

> Now I think of it, I suppose
> 
>     unsafe_min_or_max_at_line_##__LINE__()
> 
> will definitely evoke a meaningful link error.

umm, no, the ## is removed and you are left with:
undefined reference to `unsafe_min_or_max_at_line___LINE__'

The best I can think of would be something like:
asm(".unsafe_use_of_min_or_max_in_" __FUNCTION__)

which would not give you the line number, as the line number is only avalable in
integer form, I doubt you will be able to get that very well. The assembler will
give a line in relation to the asm, rather than the C, which is not what you
want...

> I still suspect that illegal assembler will do the job, since it must
> be treated after gcc has produced assembler itself and line references
> must still be present then for the assembler to be able to give meaningful
> error messages (;), but assembler is not something I write, so someone
> else needs to say.

Those are in relation to the assembler, not the C code..

-- 

Mark Zealey
mark@itsolve.co.uk

UL++++>$ G!>(GCM/GCS/GS/GM) dpu? s:-@ a16! C++++>$ P++++>+++++$ L+++>+++++$
!E---? W+++>$ N- !o? !w--- O? !M? !V? !PS !PE--@ PGP+? r++ !t---?@ !X---?
!R- b+ !tv b+ DI+ D+? G+++ e>+++++ !h++* r!-- y--

(www.geekcode.com)

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Mark Zealey wrote:"
> On Thu, Aug 30, 2001 at 11:32:55PM +0200, Peter T. Breuer wrote:
> >     unsafe_min_or_max_at_line_##__LINE__()
> > 
> umm, no, the ## is removed and you are left with:
> undefined reference to `unsafe_min_or_max_at_line___LINE__'

Not sure about that ... I'm willing to believe you, but even so surely
it would be possible to get round with the usual kind of cpp fiddle?
Something like

    #define LINE(x,y) x##y
    ...
    LINE(unsafe_min_or_max_at_line_,__LINE__)()

> The best I can think of would be something like:
> asm(".unsafe_use_of_min_or_max_in_" __FUNCTION__)
> 
> which would not give you the line number, as the line number is only avalable in

  #define stringify(x) #x
  asm(".unsafe_use_of_min_or_max_at_line_" stringify(__LINE__))

> integer form, I doubt you will be able to get that very well. The assembler will

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Mark Zealey]

On Fri, Aug 31, 2001 at 12:06:22AM +0200, Peter T. Breuer wrote:

>     #define LINE(x,y) x##y
>     ...
>     LINE(unsafe_min_or_max_at_line_,__LINE__)()

Hmm, that doesn't work here, but I think the asm one would work better anyway.

>   #define stringify(x) #x
>   asm(".unsafe_use_of_min_or_max_at_line_" stringify(__LINE__))

Yep, I missed this. The following will work:

#define _S(x) #x
#define S(x) x
asm(".error_with_min_or_max_at_line_" S(__LINE__))

-- 

Mark Zealey
mark@itsolve.co.uk

UL++++>$ G!>(GCM/GCS/GS/GM) dpu? s:-@ a16! C++++>$ P++++>+++++$ L+++>+++++$
!E---? W+++>$ N- !o? !w--- O? !M? !V? !PS !PE--@ PGP+? r++ !t---?@ !X---?
!R- b+ !tv b+ DI+ D+? G+++ e>+++++ !h++* r!-- y--

(www.geekcode.com)

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

> > only problem seems to be signed/unsigned int comparison.
> 
> I don't know how you know that! You could be right - I don't know.

because I asked the only competent instanace around: my computer.

  : ferkel:~ # more sc.c
  : #include        <stdio.h>
  : 
  : int main() {
  : signed char ca;
  : unsigned char cb;
  : 
  : signed int ia;
  : unsigned int ib;
  : 
  :         ca=-1;
  :         cb=2;
  :         if (ca<cb) printf("ca<cb\n");
  :         else if (ca>cb) printf("ca>cb\n");
  :         else printf("ca==cb\n");
  : 
  :         ia=-1;
  :         ib=2;
  :         if (ia<ib) printf("ia<ib\n");
  :         else if (ia>ib) printf("ia>ib\n");
  :         else printf("ia==ib\n");
  : 
  : }

this will give the result:

  : ferkel:~ # ./sc
  : ca<cb
  : ia>ib


you see? 

if you _explictely_ cast the signed value to unsigned, the result will
be wrong just the same way as if you did not cast it at all. if you
cast the unsigned value to signed, you will have the same problem if
the unsigned value is >  0x7fffffff. so the new macros are absolutely
unneccessary, because they do not fix anything.

> for me to be able to begin to formulate a generic solution. I've just
> seen some examples, some more convincing than others.
> 
> > >   const (typeof(a)) _a = ~(typeof(a))0
> > >   const (typeof(b)) _b = ~(typeof(b))0
> > >   if _a < 0 && _b > 0 || _a > 0 && b < 0
> > >       BUG() // one signed, the other unsigned
> > >   standard_max(a,b)
> > 
> > if sizeof(typeof(a))==sizeof(int) && sizeof(typeof(b))==sizeof(int) &&
> >    ( _a < 0 && _b > 0 || _a > 0 && b < 0 )
> > 	BUG() // signed unsigned int compare
> 
> What makes sizeof(int) so magic? Are you saying that the problems
> don't arise between signed long long and unsigned long long? I saw

no, that was just an example. long long doesnt work either.  and the
sizeof(int) is bound to 32bit platforms. unfortunately, I dont have a
64bit CPU around. the problem seems to arise where the size of a datatype
equals or extends the size of a machineword. so, really correct would
be:

  : if sizeof(typeof(a))>=sizeof(machineword) &&
  :    sizeof(typeof(b))>=sizeof(machineword)
   

however I dont know where to get this "machineword".

Re: [IDEA+RFC] Possible solution for min()/max() war

[Richard B. Johnson]

On Thu, 30 Aug 2001, Herbert Rosmanith wrote:

> 
> >   if sizeof(typeof(a)) != sizeof(typeof(b))
> >       BUG() // sizes differ
> 
> this is not neccessarily a problem. should work with char/short char/int
> short/int comparison.
> 
> only problem seems to be signed/unsigned int comparison.
> 
> >   const (typeof(a)) _a = ~(typeof(a))0
> >   const (typeof(b)) _b = ~(typeof(b))0
> >   if _a < 0 && _b > 0 || _a > 0 && b < 0
> >       BUG() // one signed, the other unsigned
> >   standard_max(a,b)
> 
> if sizeof(typeof(a))==sizeof(int) && sizeof(typeof(b))==sizeof(int) &&
>    ( _a < 0 && _b > 0 || _a > 0 && b < 0 )
> 	BUG() // signed unsigned int compare
> 



The problem really can't be solved with macros. Here is a little script
that you can run, which shows that some versions of gcc don't even
perform macro-expansion correctly.

SNIP-------
#!/bin/bash
cat >/tmp/xxx.c <<EOF
#include <stdio.h>
#undef MIN
#define MIN(a, b) ((unsigned int)(a) < (unsigned int)(b) ? (a) : (b)) 
int main(void);
int main() {
   int i;
   unsigned int j;
   i = j = 0;
   printf("%08x\n", MIN(i, j));
   return 0;
}
EOF
gcc -Wall -Wsign-compare -c -o /dev/null /tmp/xxx.c
rm -f /tmp/xxx.c
gcc --version
SNIP------

Here's a "good" execution:

Script started on Thu Aug 30 17:00:10 2001
# sh -v xxx.sh
#!/bin/bash
cat >/tmp/xxx.c <<EOF
gcc -Wall -Wsign-compare -c -o /dev/null /tmp/xxx.c
rm -f /tmp/xxx.c
gcc --version
egcs-2.91.66
# exit
exit
Script done on Thu Aug 30 17:00:24 2001

Here's a "bad" execution:

Script started on Thu Aug 30 16:55:30 2001
[root@blackhole /root]# sh -v xxx.sh
#!/bin/bash
cat >/tmp/xxx.c <<EOF
gcc -Wall -Wsign-compare -c -o /dev/null /tmp/xxx.c
/tmp/xxx.c: In function `main':
/tmp/xxx.c:9: warning: signed and unsigned type in conditional expression
rm -f /tmp/xxx.c
gcc --version
2.96

[root@blackhole /root]# exit
Script done on Thu Aug 30 16:55:58 2001
This version was shipped with RedHat 7.x

As you can see, the casts are !!!IGNORED!!! in gcc 2.96.

The min() macro is not really used for the mathematical min, anywhere
I've found it in the kernel. It's used as a whatever_will_fit() macro
where the writer wanted to prevent a buffer overflow. In these cases,
the compare should always be unsigned, even if the input values are
signed integers. It is possible to have a buffer of 0xffffffff bytes
in length and certainly 2 bytes will fit into it. Because of the
overloading of common functions to return -1 and other signed values,
it is commonplace to use signed integers to store large values without
regard for sign. Whether or not this is a design error is moot. It's
done "everywhere".

An attempt to discover signed compare problems by redefining a common
macro is doomed to fail. No matter what you do, it can be shown to
be wrong. I suggest we just leave the damn thing alone and fix any
bugs found in the normal way, i.e., "If it ain't broke, don't fix it."

Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

    I was going to compile a list of innovations that could be
    attributed to Microsoft. Once I realized that Ctrl-Alt-Del
    was handled in the BIOS, I found that there aren't any.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Thomas Dodd]

Richard B. Johnson wrote:
> that you can run, which shows that some versions of gcc don't even
> perform macro-expansion correctly.
> This version was shipped with RedHat 7.x
> 
> As you can see, the casts are !!!IGNORED!!! in gcc 2.96.

If you use cpp, you see that it is expanded.
But it still warns about it, which it shouldn't.
$ gcc -v
Reading specs from /usr/lib/gcc-lib/i386-redhat-linux/2.96/specs
gcc version 2.96 20000731 (Red Hat Linux 7.1 2.96-85)

File a bug.

	-Thomas

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Richard B. Johnson wrote:"
> The problem really can't be solved with macros. Here is a little script
> that you can run, which shows that some versions of gcc don't even
> perform macro-expansion correctly.

That's not an experiment which proves that. To show what you claim
by experiment, you would have to expand out the macro yourself and
demonstrate that gcc treats the code with the macro and the code
with your (verbatim!) expansion differently..

Since we know that gcc does a -E first (do we?), the whole idea
is absurd! You must intend to say that gcc treats the code wrongly,
whether or not it's derived from a macro or not:

> #define MIN(a, b) ((unsigned int)(a) < (unsigned int)(b) ? (a) : (b)) 

>    unsigned int j;
>    i = j = 0;
>    printf("%08x\n", MIN(i, j));

> /tmp/xxx.c:9: warning: signed and unsigned type in conditional expression
> rm -f /tmp/xxx.c
> gcc --version
> 2.96

> As you can see, the casts are !!!IGNORED!!! in gcc 2.96.

ummmm .... well, yes, one would have to say that they are ignored
for the purpose of delivering the warning to you. After all, you
requested to be warned, and it seems reasonable to me that gcc
undo your possibly mistaken casts in order to try and advise you of an
underlying problem.

One can see the algorithm in use here :-) it marks expressions as
signed or unsigned and propagates the signedness up through operators.
I think it's smart to ignore sign-changing casts, since they don't
change the _fact_ of a possible incorrect result. OTOH if you always
compare quantities that are both fundamentally signed or fundamentally
unsigned, no warning will issue and indeed there is no possibility
of an incorrect result (modulo arithmetic overflows ...).

> in length and certainly 2 bytes will fit into it. Because of the
> overloading of common functions to return -1 and other signed values,
> it is commonplace to use signed integers to store large values without
> regard for sign. Whether or not this is a design error is moot. It's
> done "everywhere".

Yes, I know what you mean. But we are reaching limits with 31 bit
numbers in many areas of the file system and vm code. It is certainly
necessary to try and expose possible bugs.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

root@chaos.analogic.com said:
> /tmp/xxx.c:9: warning: signed and unsigned type in conditional expression 
> As you can see, the casts are !!!IGNORED!!! in gcc 2.96.

No, if the casts were ignored, it would complain:
/tmp/xxx.c:9: warning: comparison between signed and unsigned

What gcc 2.96 is complaining about is not the comparison in the 
condition, but the the fact that the two possible
results of your conditional expression (x?a:b) are different. 

Change it to read:
	#define MIN(a, b) ((a) < (b) ? (int)(a) : (int)(b)) 
... and you'll see the warning you thought you saw before. 

	#define MIN(a, b) ((a) < (b) ? (a) : (b))
... and you'll see both.

--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Wagner]

Richard B. Johnson wrote:
>The min() macro is not really used for the mathematical min, anywhere
>I've found it in the kernel. It's used as a whatever_will_fit() macro
>where the writer wanted to prevent a buffer overflow. [...]
>
>I suggest we just leave the damn thing alone and fix any
>bugs found in the normal way, i.e., "If it ain't broke, don't fix it."

The problem is that this tends not to work out so well when it
comes to security.  Patch-and-pray gets pretty painful after a
while; proactive measures would seem to be called for, if security
is on the line.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bernd Schmidt]

On Thu, 30 Aug 2001, Richard B. Johnson wrote:
> #define MIN(a, b) ((unsigned int)(a) < (unsigned int)(b) ? (a) : (b))

> [root@blackhole /root]# sh -v xxx.sh
> #!/bin/bash
> cat >/tmp/xxx.c <<EOF
> gcc -Wall -Wsign-compare -c -o /dev/null /tmp/xxx.c
> /tmp/xxx.c: In function `main':
> /tmp/xxx.c:9: warning: signed and unsigned type in conditional expression
> rm -f /tmp/xxx.c
> gcc --version
> 2.96
>
> As you can see, the casts are !!!IGNORED!!! in gcc 2.96.

Nope.  The warning refers to the use of a and b in the right-hand side of
the conditional.  You have a type mismatch in the two arms of the
expression.


Bernd

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Bill Pringlemeir wrote:"
> I think that names like __reserved aren't suppose to be used and so a
> "double underbar" was used instead of a single one.  That is what I
> meant anyways.  I suppose that is another somewhat murky issue ;-)

If I thought hard, I think I could generate a file and line -specific
name.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Rick Hohensee]

>VDA
>#define min2(a,b) ({ \
>    typeof(a) __a = (a); \
>    typeof(b) __b = (b); \
>    if( sizeof(a) != sizeof(b) ) BUG(); \
>    if( ~(typeof(a))0 > 0 && ~(typeof(b))0 < 0) BUG(); \
>    if( ~(typeof(a))0 < 0 && ~(typeof(b))0 > 0) BUG(); \
>    (__a < __b) ? __a : __b; \
>    })
>
>#define min3(type,a,b) ({ \
>    type __a = (a); \
>    type __b = (b); \
>    if( sizeof(a) > sizeof(type) ) BUG(); \
>    if( sizeof(b) > sizeof(type) ) BUG(); \
>    (__a < __b) ? __a : __b; \
>    })
>


DesJardin's argument is finely crafted, but does this support it? Is min3
intended to be what Linus was talking about? I usually use m4 when I need
conditionals in macros, and I don't use C conditional statements much, but
isn't what Linus was saying simply something like...


#define min(type,a,b)		(type) a < (type) b ? (type) a : (type) b ;


Looking at the trade-offs should account for the simplicity. Even if my
code is wrong, it's about reflective of the required complexity, y/n?
The simple form doesn't catch things. That's OK for a default. Maybe
that's what you want. If it's simple you know what you have, regardless. 
"programmer error" is frequently a sign of compiler mis-design.

One reason I'm speaking in support of Linus on this one is that there's
something oddly Forth-like to a simple min(type,a,b). No coder nowhere
knows simplicity like Chuck Moore. Linux is ripe for a bit of that. The
other thing about the 3-arg min thing is that it's rather original, and it
amuses me to see Linus get disrespected for a quantum of real creativity
in his own forum. Linux is also ripe for a bit of free thinking. What's
open source for if all it helps is closed minds?

Rick Hohensee
                www.
                           cLIeNUX
                                          .com
                                                        humbubba@smart.net

Re: [IDEA+RFC] Possible solution for min()/max() war

[VDA]

>>#define min2(a,b) ({ \
>>    typeof(a) __a = (a); \
>>    typeof(b) __b = (b); \
>>    if( sizeof(a) != sizeof(b) ) BUG(); \
>>    if( ~(typeof(a))0 > 0 && ~(typeof(b))0 < 0) BUG(); \
>>    if( ~(typeof(a))0 < 0 && ~(typeof(b))0 > 0) BUG(); \
>>    (__a < __b) ? __a : __b; \
>>    })
>>
>>#define min3(type,a,b) ({ \
>>    type __a = (a); \
>>    type __b = (b); \
>>    if( sizeof(a) > sizeof(type) ) BUG(); \
>>    if( sizeof(b) > sizeof(type) ) BUG(); \
>>    (__a < __b) ? __a : __b; \
>>    })

RH> DesJardin's argument is finely crafted, but does this support it? Is min3
RH> intended to be what Linus was talking about?

My min2 requires types to be the same size and sign (after char/short->int
promotion - C always does that, seems we cannot control it - run
my test program and you'll see). If type is changed elsewhere later, min2
will barf - and this is good for preventing hard to track bugs.

Min3 is more permissive but requires programmer to indicate type
explicitly. However, it will bite you when you try convert long to
int, so it will catch some bugs too when someone decide to change
type of the variable.

Can you make better min? Another min addressing different type of bug?
It could be interesting. (We have similar issues with "less than" and
the like ops)

Don't know what Linus originally intended. I thought my two cents
might be useful.

RH> isn't what Linus was saying simply something like...

RH> #define min(type,a,b)           (type) a < (type) b ? (type) a : (type) b;

Looks too dangerous to me. Double evaluation of a and b, no ()
around them...

RH> Looking at the trade-offs should account for the simplicity.

Ifs inside my min2 and min3 optimize out to nothing. Checked that.
-- 
Best regards,
VDA
mailto:VDA@port.imtp.ilyichevsk.odessa.ua
http://port.imtp.ilyichevsk.odessa.ua/vda/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Petr Vandrovec]

On  4 Sep 01 at 12:09, VDA wrote:
> min2 performs a very strict checking.
> min3 gives you full control, but checks for too small target type.
> Do anybody see any flaws? Any improvements?
> For compiler folks: Why GCC compiles ...f(1);f(1);f(1)... to
> ...
>         pushl $1
>         call f
>         addl $32,%esp  <-- My grandma optimizes better
>         addl $-12,%esp <-- ?
>         pushl $1
>         call f
>         addl $16,%esp  <-- ?
>         addl $-12,%esp <-- ?

Compile with 'gcc -mpreferred-stack-boundary=2', gcc generates
unbelievable stupid code for other values.
                                        Best regards,
                                            Petr Vandrovec
                                            vandrove@vc.cvut.cz
                                            

Re: [IDEA+RFC] Possible solution for min()/max() war

[VDA]

A test program.
min2 performs a very strict checking.
min3 gives you full control, but checks for too small target type.
Do anybody see any flaws? Any improvements?
For compiler folks: Why GCC compiles ...f(1);f(1);f(1)... to
...
        pushl $1
        call f
        addl $32,%esp  <-- My grandma optimizes better
        addl $-12,%esp <-- ?
        pushl $1
        call f
        addl $16,%esp  <-- ?
        addl $-12,%esp <-- ?
...
--------------------------------------------------------------------
#include <stdio.h>

#define min2(a,b) ({ \
    typeof(a) __a = (a); \
    typeof(b) __b = (b); \
    if( sizeof(a) != sizeof(b) ) BUG(); \
    if( ~(typeof(a))0 > 0 && ~(typeof(b))0 < 0) BUG(); \
    if( ~(typeof(a))0 < 0 && ~(typeof(b))0 > 0) BUG(); \
    (__a < __b) ? __a : __b; \
    })

#define min3(type,a,b) ({ \
    type __a = (a); \
    type __b = (b); \
    if( sizeof(a) > sizeof(type) ) BUG(); \
    if( sizeof(b) > sizeof(type) ) BUG(); \
    (__a < __b) ? __a : __b; \
    })

#define BUG() printf("BUG at %i\n",__LINE__)
#define llong long long

// Uncomment {} for compile, comment for gcc -S -O2 optimizer test
void f(int v);
// void f(int v) {}

int main() {
    signed char  sc=1; unsigned char  uc=1;
    signed short ss=1; unsigned short us=1;
    signed int   si=1; unsigned int   ui=1;
    signed long  sl=1; unsigned long  ul=1;
    signed llong su=1; unsigned llong uu=1;

    f(min2(sc,sc)); // not a BUG
    f(min2(sc,uc)); // not a BUG: (typeof(x))0 first expanded to signed int
    f(1); // optimizer test: all the mins must be optimized to 
    f(1); // f(1) so do gcc -S -O2 and inspect .s file
    f(min2(ss,ss)); // not a BUG
    f(min2(ss,us)); // not a BUG: (typeof(x))0 first expanded to signed int
    f(min2(si,si)); // not a BUG
    f(min2(si,ui)); // BUG: signedness mismatch
    f(min2(sl,sl)); // not a BUG
    f(min2(sl,ul)); // BUG: signedness mismatch
    f(min2(su,su)); // not a BUG
    f(min2(su,uu)); // BUG: signedness mismatch
    f(min2(sc,ss)); // BUG: size mismatch
    f(min3(unsigned char, sc,uc)); // not a BUG
    f(min3(int,           sc,ss)); // not a BUG
    f(min3(unsigned llong,ss,uu)); // not a BUG
    f(min3(unsigned long, ss,uu)); // BUG: target type is too small
    
    return 0;
}
-------------------------------------------------------------
Best regards, VDA
mailto:VDA@port.imtp.ilyichevsk.odessa.ua
http://port.imtp.ilyichevsk.odessa.ua/vda/

Re: [IDEA+RFC] Possible solution for min()/max() war

[David desJardins]

Linus Torvalds <torvalds@transmeta.com> writes:
> For example, let's look at this perfectly natural code:
> 
> static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned
> *hashp)
> {
> if (len <= sizeof(short) || len > sizeof(*sunaddr))
> return -EINVAL;
> ...

> code that is totally correct, and that it would make _no_ sense in
> writing any other way.

The code is correct, but if one is adding explicit types, for clarity
and to avoid introducing bugs, then I think that code like this is
exactly where they most belong:

  if ((size_t) len <= sizeof(short) || (size_t) len > sizeof(*sunaddr))

If that prevents one person from later writing buggy code like:

  if ((size_t) len <= sizeof(short))

then it's a Good Thing.

  -- David desJardins

Re: [IDEA+RFC] Possible solution for min()/max() war

[David desJardins]

Linus Torvalds <torvalds@transmeta.com> writes:
> I have one thing to say to people who do not like the new min/max 
> functions: most of them probably never even _understood_ the multitude 
> of bugs that are inherent in the classical 
> 
>         #define min(x,y) ((x) < (y) ? (x) : (y)) 

I think you're mistaken about what people understand.

The explicit typing of MIN and MAX will avoid some bugs that have to do
with the comparison of entities of different types.  The explicit typing
will also introduce some bugs, when a user uses the wrong type (e.g.,
when the code is initially written with the correct type, but then
someone later changes the type of the variables, and doesn't remember to
change the type in the macro call).  Overall, I think it will be about a
wash.

If this is really a serious problem, then people shouldn't be using the
comparison operators like "<" either, as they have exactly the same
problem when used to compare objects of different types (particularly
since C has such broken rules for deciding what type to use when
comparing objects of different types).  Instead, we should have
LESSTHAN(type,a,b), etc.

I think the best approach to bug avoidance would be two-argument MIN and
MAX functions which require that both arguments have the same type, but
where that can be any type.  Then users who want to compare objects of
different types would have to explicitly cast one to the type of the
other (or both to a third common type).  In the most common case where
users are simply comparing objects of the same type, there would be no
need or reason to change the code.

It's simple enough for an external checker to confirm that this rule is
followed.  The same checker could enforce the same rule for "<" and
other comparison operators, which would probably help eliminate several
bugs (without the unacceptably clunky LESSTHAN macro).

I think no one who is comparing objects of two different types can
legitimately complain about being asked to cast one (or both) of them to
a common type: the user, rather than the compiler, should indeed choose
the type of the comparison.  But that's different from redundantly
adding the type everywhere to comparisons of objects of the same type,
which introduces, rather than eliminating, a source of error.

  -- David desJardins

Re: [IDEA+RFC] Possible solution for min()/max() war

[VDA]

Monday, September 03, 2001, 11:35:52 PM, David desJardins
<david@desjardins.org> wrote:

Dd> The explicit typing of MIN and MAX will avoid some bugs that have to do
Dd> with the comparison of entities of different types.  The explicit typing
Dd> will also introduce some bugs, when a user uses the wrong type (e.g.,
Dd> when the code is initially written with the correct type, but then
Dd> someone later changes the type of the variables, and doesn't remember to
Dd> change the type in the macro call).  Overall, I think it will be about a
Dd> wash.
...
Dd> I think the best approach to bug avoidance would be two-argument MIN and
Dd> MAX functions which require that both arguments have the same type, but
Dd> where that can be any type.  Then users who want to compare objects of
Dd> different types would have to explicitly cast one to the type of the
Dd> other (or both to a third common type).  In the most common case where
Dd> users are simply comparing objects of the same type, there would be no
Dd> need or reason to change the code.

This sounds right!

#define min2(a,b)      ...  <-- checks for a,b being the same type
#define min3(type,a,b) ...  <-- casts a,b to type first

min3(long long,a,b) will be a favorite in bug chasing :-)

Dd> It's simple enough for an external checker to confirm that this rule is
Dd> followed.  The same checker could enforce the same rule for "<" and
Dd> other comparison operators, which would probably help eliminate several
Dd> bugs (without the unacceptably clunky LESSTHAN macro).

Good candidate for Stanford checker, but there will be lots of false
positives.
-- 
Best regards,
VDA
mailto:VDA@port.imtp.ilyichevsk.odessa.ua
http://port.imtp.ilyichevsk.odessa.ua/vda/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ted Unangst]

On Fri, 31 Aug 2001, Peter T. Breuer wrote:

> "Roman Zippel wrote:"
> > [ptb wrote]
> > > Stanford checker? Is that a programmable C type checker? If so, lemmee
> > > at it. Have you a URL, btw?
> >
> > http://verify.stanford.edu/SVC/
> > You should search the archive to look for some good examples, how it can
> > help.
>
> Hmm .. it looks like a model checker, and only for 1st order logic
> (i.e. not CTL). It seems very primitive. What's the point of using this
> instead of the many sphisticated model checkers and theorem provers out
> there?

that's the wrong checker.  see http://hands.stanford.edu.





>
> Peter
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

--
"First, it was not a strip bar, it was an erotic club.  And second,
what can I say?  I'm a night owl."
      - M. Barry, Mayor of Washington, DC

Re: [IDEA+RFC] Possible solution for min()/max() war

[Andy Chou]

SVC isn't the "Stanford Checker" that has been resulting in kernel
patches.  The name "Stanford Checker" was invented by people on LKML; the
real name of the project is Meta-level compilation, and its URL is:

http://hands.stanford.edu

SVC isn't a model checker either.  It's an implementation of cooperating
decision procedures.

-Andy


> > Stanford checker? Is that a programmable C type checker? If so, lemmee
> > at it. Have you a URL, btw?

> http://verify.stanford.edu/SVC/
> You should search the archive to look for some good examples, how it can
> help.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Andy Chou wrote:

> SVC isn't the "Stanford Checker" that has been resulting in kernel
> patches.  The name "Stanford Checker" was invented by people on LKML; the
> real name of the project is Meta-level compilation, and its URL is:
> 
> http://hands.stanford.edu

Sorry, I should have checked it more carefully.
(bad google... :) )

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

From: Rik van Riel <riel@conectiva.com.br>
> On Fri, 31 Aug 2001, Herbert Rosmanith wrote:
> > "I contemplated about the way of Linus, and eventually I begin to
> > understand HIS aim."
> 
> Understanding his aim is useful. It allows you to get
> out of the way when he starts shooting.

Ah, eventually I begin to understand why Linus is exercising in
pistols with ESR.

> > excuse me. this is ridiculous.
> 
> Well, we knew that before the thread started. Can't win
> a holy war.

what I wanted to say is: it should not neccessary to <peter breuer>
"seeing this discussion fly past for a week I begin to
 understand what Linus' aim might be"
</peter breuer>

spend a week reading lkml and maybe catch a hint about what Linus
had in mind when introducing some code.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

From: "Peter T. Breuer" <ptb@it.uc3m.es>
> > From: "Herbert Rosmanith" <herp@wildsau.idv.uni-linz.ac.at>
> >
> > if sizeof(typeof(a))==sizeof(int) && sizeof(typeof(b))==sizeof(int) &&
> >    ( _a < 0 && _b > 0 || _a > 0 && b < 0 )
> >       BUG() // signed unsigned int compare
> 
> What makes sizeof(int) so magic?

ask your compiler.

> Are you saying that the problems don't arise between signed long long
> and unsigned long long?

no.

> I saw an example that seemed generic for all signed unsigned
> comparisons, namely that signed int and unsigned int are compared as
> unsigned, so (unsigned)2 < (signed)-1. 

which is wrong. so?

> Are you saying that signed long and unsigned long are NOT compared as
> unsigned iff sizeof(long) != sizeof(int).  Woooo!  Who wrote the C spec?
> No kidding? There is a special clause for comparisons that use the
> native machine word?

excuse me, pete, but being sarcastic does not suite you well:

in Message-Id: <200108311322.PAA12269@nbd.it.uc3m.es>
you write:
> #define MIN(x,y) ({\
>    const typeof(x) _x = ~(typeof(x))0; \
>    const typeof(y) _y = ~(typeof(y))0; \
>    if (sizeof(_x) != sizeof(_y)) \
>      MINMAX_BUG; \


this is just not right. you don't  even honour if x and y are of the
same signedness! if x and y are of the same signedness, a comparison
can be done at absolutely no risk regardless of the size!

and even *if* the sizes differ, there are cases, where the comparison
still can be done at no risk. namely:

 signed short / unsigned char       ->  OK
 unsigned short / signed char       ->  OK
 signed int / unsigned char         ->  OK
 signed int / unsigned short        ->  OK
 signed long long / unsigned char   ->  OK
 signed long long / unsigned short  ->  OK

(I don't know about signed long long / unsigned int, I'd have to ask the
compiler/assembler. but I guess it will be okay, too)

but:

  unsigned int / signed char        -> fails.
  unsigned int / signed short       -> fails.
  unsigned long long / signed char  -> fails.
  unsigned long long / signed short -> fails.
  unsigned long long / signed int   -> fails.

I haven't really verified all cases, they are a conclusion from seeing
the code gcc generates.

if we follow your minmax-macro code:

>    if ((_x > (typeof(x))0 && _y < (typeof(y))0) \
>    ||  (_x < (typeof(x))0 && _y > (typeof(y))0)) \
>      MINMAX_BUG; \
>    __MIN(x,y); \
>  })

if I see this right, after requiring the size be the same, you also
require the same signdness?

but:

 : 	char a; unsigned char b;
 : 
 : 	a=-1;	b=255;
 : 	if (a<b) ....

will work perfectly. same applies for short a/unsigned short b;

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

From: "Peter T. Breuer" <ptb@it.uc3m.es>
> And after seeing this discussion fly past for a week I begin to
> understand what Linus' aim might be.

haha. 
"I contemplated about the way of Linus, and eventually I begin to
understand HIS aim."

excuse me. this is ridiculous.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Rik van Riel]

On Fri, 31 Aug 2001, Herbert Rosmanith wrote:
> From: "Peter T. Breuer" <ptb@it.uc3m.es>
> > And after seeing this discussion fly past for a week I begin to
> > understand what Linus' aim might be.
>
> haha.
> "I contemplated about the way of Linus, and eventually I begin to
> understand HIS aim."

Understanding his aim is useful. It allows you to get
out of the way when he starts shooting.

> excuse me. this is ridiculous.

Well, we knew that before the thread started. Can't win
a holy war.

cheers,

Rik
-- 
IA64: a worthy successor to i860.

http://www.surriel.com/		http://distro.conectiva.com/

Send all your spam to aardvark@nl.linux.org (spam digging piggy)

Re: [IDEA+RFC] Possible solution for min()/max() war

[ctm]

The following message is a courtesy copy of an article
that has been posted to fa.linux.kernel as well.

Linus Torvalds <torvalds@transmeta.com> writes:

> For example, let's look at this perfectly natural code:
> 
> 	static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
> 	{
> 		if (len <= sizeof(short) || len > sizeof(*sunaddr))
> 			return -EINVAL;
> 		...
> 
> Would you agree that the above is _good_ code, and code that makes
> perfect sense, and code that does exactly the right thing in testing its
> arguments?

Personally, I find the above code to be misleading, because

	len <= sizeof(short)

will evaluate to 1 if len is negative.  That is counter-intuitive.

The fact that the test on the right side of the || will get the code
to execute return -EINVAL does make the code correct in that the
right thing will happen when len is negative.  But if someone sees
that sizeof(*sunaddr) is being used in a lot of places and decides to
clean up the code by introducing a variable to hold the sizeof, you
get code that looks very similar to the above but that is now wrong,
because the right side no longer has the special property of "catching
the problem the left side created".

		int max_value;

		max_value = sizeof(*sunaddr);

 		if (len <= sizeof(short) || len > max_value)
 			return -EINVAL;

		... max_value this ...

		... max_value that ...

		... max_value the other thing...

One can argue that making max_value an int (or introducing it at all)
is a bad thing because it breaks the idiom that was originally used.
I won't disagree, but then again I understand signed and unsigned
comparisons and I see why the idiom breaks when you introduce
max_value as an int.

I understand why the sizeof operator is unsigned, but also know that
does introduce some problems.

Although it's ugly,

#define isizeof(x)  ((int) sizeof(x))

would provide a signed sizeof that could be used in all the above code
with no complaints from -Wsign-compare.  But I don't claim that there
wouldn't be other places where -Wsign-compare would produce spurious
complaints.  If there weren't, then I would think use of a min/max
that didn't have a cast forced into it combined with -Wsign-compare
and isizeof would make it more likely that the compiler would point
out real bugs than the min/max with the forced cast.

I don't maintain any linux kernel code, so I'm not lobbying for this
particular solution.  I just don't think Linus's example is _good_
code.  I think it's correct, but misleading, code.

Regards,

Cliff Matthews <ctm@ardi.com>

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

ctm@ardi.com wrote:

> I don't maintain any linux kernel code, so I'm not lobbying for this
> particular solution.  I just don't think Linus's example is _good_
> code.  I think it's correct, but misleading, code.

True, but the only argument I heard from Linus lately is, how bad
Wsign-compare is. It's not that difficult to prove that gcc produces
false warnings, even if it has the information to know better. That's
not the point.
What started the discussion is that Linus introduced a new version of
min/max macros. I brought up arguments, why I think these macros are
unsafe and I got no response to this. I made the mistake of mentioning
-Wsign-compare and I get a long explanation instead, how bad it is.
In the meantime people are trying to fix bugs without defining them,
ignoring the fact that there are already tools, which can help finding
real bugs. If the tool is too coarse, use it only from time to time or
use a different tool.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

> "A month of sundays ago Roman Zippel wrote:"
> > On Fri, 31 Aug 2001, Peter T. Breuer wrote:
> >
> > >    if (sizeof(_x) != sizeof(_y)) \
> > >      MIN_BUG(); \
> >
> > What bug are you trying to fix here?
>
> Wake up!

good morning, peter.

try comparing e.g. unsigned char with signed short.

roman is right if he asks "What bug are you trying to fix here?" sure,
sometimes there is a bug, sometimes there is not. so with the question
"*what* bug" roman surely means, which situation do you want to identify
as the buggy one?

if all you do is to compare the size of the operands without taking
into account the architectural limit, you will also mock about
different-sized comparison which are perfectly legal and not buggy
at all.

look at this code:

  : #include        <stdio.h>
  : 
  : int main() {
  : unsigned short us;
  : signed char sc;
  : 
  :         us=65535;
  :         sc=-2;
  :         if (us>sc) printf("us>sc %d %d\n",us,sc);
  : 
  :         return 0;
  : }

which compiles to:

  :      movw $65535,-2(%ebp)
  :      movb $-2,-3(%ebp)
  :      xorl %eax,%eax
  :      movw -2(%ebp),%ax
  :      movsbl -3(%ebp),%edx
  :      cmpl %edx,%eax
  :      jle .L3

you see this? short and char get expanded to 32bit int (I have a x86),
and a signed comparison can be done without danger. "jle" jumps if
SF != OF, which accounts to "op1 < op2" in a signed compare. the
"unsigned short" quantum gets expanded to an "unsigned int", which
is no problem, since the bit16-31 will be 0. the "signed char" will
also be expanded to 32bit and eventually (for negative values) have
bit31-bit8 "1". thus, a signed compare with 32bits will work perfectly
well for an unsiged 16bit value.

The problem arises when one of the variables cannot be expanded any
more. Of course, the compiler could generate code to i.e. expand
int32 to long long, but this has not be done, may be or may not be
done in a future release or whatever.

For instance:

  : int main() {
  : unsigned int ui;
  : signed char sc;
  : 
  :         ui=1;
  :         sc=-2;
  :         if (ui>sc) printf("ui>sc %d %d\n",ui,sc);
  : 
  :         return 0;
  : }

will not work as expected.

the reason is that:

  :         movl $1,-4(%ebp)
  :         movb $-2,-5(%ebp)
  :         movsbl -5(%ebp),%eax
  :         cmpl %eax,-4(%ebp)
  :         jbe .L3

"jbe" honours CF and ZF, which accounts to an *unsigned* compare.
however, the negative "signed char" got expanded to 0xffffffff,
which is, in an unsigned compare-context, greater than 0x00000001.

I hope I've shown that "sizeof(a) != sizeof(b)" is not neccessarily
*always* a buggy situation.

Even with int, a "signed int" with an "unsigned char" compare could
be done. The buggy situation is when the bigger variable is unsigned,
in size equal to the architectural limit, and the smaller variable
is signed.

I hope I got that right.

cheers,
herp
 

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

> which compiles to:
> 
>   :      movw $65535,-2(%ebp)
>   :      movb $-2,-3(%ebp)
>   :      xorl %eax,%eax
>   :      movw -2(%ebp),%ax
>   :      movsbl -3(%ebp),%edx
>   :      cmpl %edx,%eax
>   :      jle .L3
> 
> you see this? short and char get expanded to 32bit int (I have a x86),
> and a signed comparison can be done without danger. "jle" jumps if
> SF != OF, which accounts to "op1 < op2" in a signed compare. the
> "unsigned short" quantum gets expanded to an "unsigned int", which
                                               ^^^^^^^^^^^^^^

yikes, this should really read "signed int".

Re: [IDEA+RFC] Possible solution for min()/max() war

[Martin Knoblauch]

> Re: [IDEA+RFC] Possible solution for min()/max() war
> 
> From: Linus Torvalds (torvalds@transmeta.com)
> Date: Mon Aug 27 2001 - 23:59:41 EST
> 
> 
> There were a few alternatives that we looked at (or rather, David
> implemented, and I ended up rejecting due to various reasons), but they
> all boiled down to "how do we sanely generate min/max functions while at
> the same time forcing people to understand the types in question". Some
> of the intermediate patches had the type in the macro name, ie things
> like "min_uint()" and "min_slong()". The final version (ie the one in
> 2.4.9) was deemed to be the most flexible.
> 

 Reading the whole (entertaining) thread my question is: couldn't the
min/max change wait until 2.5? It seems to "break" some code and that
should not happen in a "stable" stream?

Martin
-- 
------------------------------------------------------------------
Martin Knoblauch         |    email:  Martin.Knoblauch@TeraPort.de
TeraPort GmbH            |    Phone:  +49-89-510857-309
C+ITS                    |    Fax:    +49-89-510857-111
http://www.teraport.de   |    Mobile: +49-170-4904759

Re: [IDEA+RFC] Possible solution for min()/max() war

[Rick Hohensee]

Linus' 3-arger is the way to go for mix/max defaults. Very simple,
particularly in terms of obscure features required.  Why you guys always
want to be at the nether reaches of the .info's is beyond me. 

Rick Hohensee
                www.
                           cLIeNUX
                                          .com
                                                        humbubba@smart.net

Re: [IDEA+RFC] Possible solution for min()/max() war

[Andy Chou]

How about:

#define AssertNow(x) switch(1) { case (x): case 0: }

as in:

AssertNow(sizeof(typeof(x)) == sizeof(typeof(y)));

I'm not sure if gcc optimizes this away, but it would be easy for someone
to find out.

I didn't invent this.

-Andy


> And I was hoping that somebody could produce some gcc magic
> replacement for BUG() that means "don't compile me". Perhaps
> a bit of illegal assembler code with a line reference in?
> Surely gcc must have something like __builtin_wont_compile()?
> It just needs to be a leaf of the RTL that evokes a compile error.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Gordon Oliver wrote:"
[Charset ISO-8859-1 unsupported, filtering to ASCII...]
> On 2001.08.30 16:27 Peter T. Breuer wrote:
> > Possibly. I have little clue as to the real extent of the problem.
> 
>   You've missed the _vast_ majority of the real problems. And there is
> no way to fix some of them...
>     - if the sizeof the arguments is the same, it is a bug to have
>       signed vs. unsigned.

That's caught by the solution proposed.

>     - if the sizeof the arguments is different, it is a bug to have
>       the _larger_ argument unsigned.

This can't be caught at compile time.

>     - if one of the arguments is a constant, typeof will give you
>       a mostly arbitrary value, making { unsigned int i,j; j = max(i,5);}
>       return a bug.

?? I don't follow this at all. Typeof is deterministic, since the
gcc computer program is deterministic. Typeof MUST return the type of 
the expression to which it applies.  All expressions in C have 
precisely computed types -I guess  what you are saying is that that
the type of an expression may be context dependent, which I can easily
imagine in a random computer language, but seriously doubt for C.
C really does type calculations via narrowing :-o! Oh yeah!

Show me an instance of an expression that two differnt types depending
on context. I am prepared to be surprised, but dubious.

(umm ... what type is the "5" in "(short)5" ?? Why, signed integer, I
believe. It's truncated to short, modulo a language lawyer's second
opinion).

>     - not forcing the person to actually set the type of the argument
>       will allow things like
>       {
> 	int user_land_value, buffer_size;

Nice integers.

> 	user_land_value = magic_from_user_land();

signed integer value.

> 	buffer_size = min(user_land_value, 10);

well, 10 is a signed integer constant. AFAIK you'd have to write "10u"
to get an unsigned integer constant. And yes, the latter would be
caught by my solution.


> 	buffer = memcpy(some_place, some_other_place, buffer_size); /* BOOM
> */
>       }
>       to not show a bug at all (10 is signed).

I see no bug.

There would be a bug if 10u were used and user_land_value were negative,
since then the result of min would be 10u (and this bug would be flagged
by my solution).  But as things are, the result is user_land_value.

I just checked.


> Please, can we take this off of lkml... (btw, see the attached file
> for the bugs).

Are you SURE you can find the bugs! :-)

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Peter T. Breuer wrote:"
> "Gordon Oliver wrote:"
> >     - if the sizeof the arguments is different, it is a bug to have
> >       the _larger_ argument unsigned.
> 
> This can't be caught at compile time.

Oops - I misinterpreted what you said. You mean "longer", not "larger".
Yes, this can be caught by a variant of the solution I proposed ...

   typeof(x) _x = ~(typeof(x))0;
   typeof(y) _y = ~(typeof(y))0;
   ...
   if (sizeof(_x) > sizeof(_y) && _x > 0)
       BUG();
   if (sizeof(_x) < sizeof(_y) && _y > 0)
       BUG();

Though I'm too sleepy to think about why there's a bug in this case ...
Isn't it only a bug if the longer arg is unsigned and the shorter is
signed? (I suppose the shorter is promoted with sign and then converted
to unsigned but I'm not going to get up and open a book to find out ..).

That would be:

   if (sizeof(_x) > sizeof(_y) && _x > 0 && _y < 0)
       BUG();
   if (sizeof(_x) < sizeof(_y) && _y > 0 && _x < 0)
       BUG();

Moral, always use ints.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Jonathan Lundell]

At 3:19 AM +0200 2001-08-31, Peter T. Breuer wrote:
>?? I don't follow this at all. Typeof is deterministic, since the
>gcc computer program is deterministic. Typeof MUST return the type of
>the expression to which it applies.  All expressions in C have
>precisely computed types -I guess  what you are saying is that that
>the type of an expression may be context dependent, which I can easily
>imagine in a random computer language, but seriously doubt for C.
>C really does type calculations via narrowing :-o! Oh yeah!
>
>Show me an instance of an expression that two differnt types depending
>on context. I am prepared to be surprised, but dubious.

OK.

At 1:27 AM +0200 2001-08-31, Peter T. Breuer wrote:
>// standard good 'ol faithful version
>#define __MIN(x,y) ({\
>    typeof(x) _x = x; \
>    typeof(y) _y = y; \
>    _x < _y ? _x : _y ; \
>  })

How about typeof(__MIN(u, s)), given unsigned u, int s?
-- 
/Jonathan Lundell.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alex Bligh - linux-kernel]

>># define __MIN(x,y) ({\
>>    typeof(x) _x = x; \
>>    typeof(y) _y = y; \
>>    _x < _y ? _x : _y ; \
>>  })
>
> How about typeof(__MIN(u, s)), given unsigned u, int s?

As this would expand to typeof({typeof(u) _u=u .... ? _u : _s;})
I am willing to bet it wouldn't even compile with either
version of min, so it doesn't matter.
--
Alex Bligh

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Alex Bligh - linux-kernel wrote:"
> I believe your patch is correct & better than the
> 3 arg perversion, (because the diff to 3 args
> can only be worse than the diff to strict
> casting); carry on plugging it.

Can't say that I have an agenda to carry. It just is a fact that
a 3-arg min/max sticks in the craw! And after seeing this discussion
fly past for a week I begin to understand what Linus' aim might be
(and what the technical problem might be, but I don't think that's
as important). And anyone wth access to gcc's info page can see
that gcc extensions are powerful enough to do the extra checking
that Linus wants ... even without working out the details.

Out of the same mild interest, I've counted the following defn's of min
in the kernel headers (2.4.8):

  include/linux/amigaffs.h:#define MIN(a, b) ({           \
  include/linux/cyclomx.h:#define min(a,b) (((a)<(b))?(a):(b))
  include/linux/cycx_drv.h:#define MIN(a,b)       ((a) < (b) ? (a) : (b))
  include/linux/efs_fs.h:#define MIN(a, b) (((a) < (b)) ? (a) : (b))
  include/linux/if_tun.h:#define MIN(a,b) ( (a)<(b) ? (a):(b) ) 
  include/linux/isdn.h:#define MIN(a,b) ((a<b)?a:b)
  include/linux/isicom.h:#define MIN(a, b) ( (a) < (b) ? (a) : (b) )
  include/linux/lvm.h:#define min(a,b) (((a)<(b))?(a):(b))
  include/linux/wanpipe.h:#define min(a,b) (((a)<(b))?(a):(b))

so apparently there wasn't a central definition before? Incidentally,
most of these are vulnerable to double evaluation.

I've modified these (used the macro with asm(."err");) and recompiled ...

 ..no messages from bzImage.

Groove.  Make modules hits paydirt:

  gcc -D__KERNEL__ -I/usr/local/src/linux-2.4.8/include  -Wall
  -Wstrict-prototypes -Wno-trigraphs -O2 -fno-strict-aliasing
  -fno-common -pipe -mpreferred-stack-boundary=2 -march=i686 -DMODULE
  -c -o tun.o tun.c
  {standard input}: Assembler messages:
  {standard input}:633: Error: Unknown pseudo-op:
  `.error_with_min_or_max'
  make[2]: *** [tun.o] Error 1

That's line 270:

          int len = count ...
          ...
          len = MIN(skb->len, len);
          copy_to_user(ptr, skb->data, len);

Well, I assume skb->len is unsigned. But len can be negative at times
..  it causes an error return. But this assignment is in the non-error
path where len >= 0, so it is safe to do unsigned comparisons
and replace this with

          if (len > skb->len)  // len >= 0 guaranteed
              len = skb->len;


There are other errors. I'll leave them to you ...

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

hi,

I just can't see why the new min/max macros are any better than the old
one.

given the following two variables: "signed int i" and "unsigned int j"
assume i=-1 and j=2. we all know that -1 < 2, unfortunately, C doesnt
know that, probably when these values are stored in machineword quantities.
(i.e. no problem wich char and short, but with int on 32bit platform).

now, the new macros come in. we could now either write:

    min(unsigned int,i,j)           ; case 1
or
    min(signed int,i,j)             ; case 2


when casting to unsigned int, -1 will become 0xffffffff and 
"min(unsigned int,-1,2)" will return 2. this is wrong.

case 2 will give the right result.

but what will happen if the unsigned variable is so large that it will
use the most significant bit?

assume i=-1 and j=0xfffffff0;
still, -1 < 0xfffffff0 is true.

	min(unsigned int,i,j)
will give 0xfffffff0 because its < 0xffffffff;
wrong result.

	min(signed int,i,j)
will give 0xfffffff0 because it will be cast to -16 which is < -1.
wrong result again.

I think this shows that the type-argument in the macros gain nothing.
Instead, whenever we encounter such a problem, the code should be
closely investigated.

I think that this really is some compiler-issue. signed/unsigned comparison
is okay for char and short, but not for int; looks like the compiler
forgets to set or evaluate the carry-flag, perhaps?

Re: [IDEA+RFC] Possible solution for min()/max() war

[mike_phillips]

> So I have this number, -200, which is stored in an int. I have this 
other
> number, 200, which is stored in an unsigned char. Everybody in his right
> mind will agree that -200 is smaller than 200, the compiler will do just
> that, yet you disagree???

Now try doing that with an int and an unsigned int, you'll get 200, not 
-200.

Mike

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ion Badulescu]

On Thu, 30 Aug 2001 mike_phillips@urscorp.com wrote:

> > So I have this number, -200, which is stored in an int. I have this 
> other
> > number, 200, which is stored in an unsigned char. Everybody in his right
> > mind will agree that -200 is smaller than 200, the compiler will do just
> > that, yet you disagree???
> 
> Now try doing that with an int and an unsigned int, you'll get 200, not 
> -200.

You'll get a warning with -Wsign-compare, which is what the argument was 
all about.

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

> Please guys. The issue of sign in comparisons are a LOT more complicated
> than most of you seem to think.

as a friend of mine put it on IRC:
  "Your little brains are not able to grasp the complicated issue of
  sign in comparisons."

If the problem is compiler-related, shouldn't it be forwared to the
gcc-group?

e.g.:

    : void foo() {
    : unsigned int i;
    : signed int j;
    : 
    : 	i=j;
    : }

does not generated a warning even with "-Wall". I think it should print
out something like "warning: sign will be lost in signed to unsigned
assignment".

The rules for signed/unsigned comparison should be:
 o the sign is always "sticky" (i.e. signed/unsigned -> signed)
 o data size is expanded to the size of the larger var.
 o (1) if the larger var is unsigned, data size is expanded
       even further.
   (2) if that's not possible, use carry flag.

However, I noticed that this "value-preserving arithmetic conversion"
is not always performed.

code follows:


    : #include        <stdio.h>
    : 
    : int main() {
    : 
    : unsigned char ca;
    : signed char cb;
    : 
    : unsigned short sa;
    : signed short sb;
    : 
    : unsigned int ia;
    : signed int ib;
    : 
    :         ca=0xff;
    :         cb=0xff;
    :         printf("ca=%u cb=%d\n",ca,cb);
    :         if (ca<cb) printf("ca<cb\n");
    :         else if (ca>cb) printf("ca>cb\n");
    :         else printf("ca==cb\n");
    : 
    :         sa=0xffff;
    :         sb=0xffff;
    :         printf("sa=%u sb=%hd\n",sa,sb);
    :         if (sa<sb) printf("sa<sb\n");
    :         else if (sa>sb) printf("sa>sb\n");
    :         else printf("sa==sb\n");
    : 
    :         ia=0xffffffff;
    :         ib=0xffffffff;
    :         printf("ia=%u ib=%d\n",ia,ib);
    :         if (ia<ib) printf("ia<ib\n");
    :         else if (ia>ib) printf("ia>ib\n");
    :         else printf("ia==ib\n");
    : 
    :         return 0;
    : }

will result to:

    : bash-2.03# cc -Wsign-compare -o cm cm.c
    : cm.c: In function `main':
    : cm.c:32: warning: comparison between signed and unsigned
    : cm.c:33: warning: comparison between signed and unsigned
    : bash-2.03# ./cm
    : ca=255 cb=-1
    : ca>cb
    : sa=65535 sb=-1
    : sa>sb
    : ia=4294967295 ib=-1
    : ia==ib


so, in the "int" case, the result of the comparison is wrong. (shouldnt
it be expanded to long long, or at least use the carry-flag?)
If the min/max macros explicitly cast a signed data-type to an
unsigned one, the meaning of the sign-bit will be lost in any
way, not only in the int-case.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Helge Hafting]

Herbert Rosmanith wrote:
> 
> > Please guys. The issue of sign in comparisons are a LOT more complicated
> > than most of you seem to think.
> 
> as a friend of mine put it on IRC:
>   "Your little brains are not able to grasp the complicated issue of
>   sign in comparisons."
> 
> If the problem is compiler-related, shouldn't it be forwared to the
> gcc-group?
> 
C has (complicated) rules for this.  The gcc people can't break them in
order to
make it easier for developers to understand.  But you may of course
try a discussion on comp.lang.c

Helge Hafting

Re: [IDEA+RFC] Possible solution for min()/max() war

[Herbert Rosmanith]

> From: Helge Hafting <helgehaf@idb.hist.no>
> X-Mailer: Mozilla 4.76 [no] (X11; U; Linux 2.4.10-pre1 i686)
> X-Accept-Language: no, en
> To: Brad Chapman <kakadu_croc@yahoo.com>, linux-kernel@vger.kernel.org
> Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
> References: <20010828204207.12623.qmail@web10908.mail.yahoo.com>
> Sender: linux-kernel-owner@vger.kernel.org
> Precedence: bulk
> X-Mailing-List: 	linux-kernel@vger.kernel.org
> 
> Brad Chapman wrote:
> > 
> > Then why, IIRC, are the kernel hackers so upset about how
> > the three-arg macros _prevent_ signed/unsigned comparisons?
> > Apparently the ability to compare
> > signed/unsigned variables must have _some_ significance....
> 
> You are misunderstanding this.
> It is possible to write something that _looks like_ a signed to unsigned
> comparison, i.e.:
> int a;
> unsigned int b;
> ...
> x = min(a,b);
> 
> Looks like a signed to unsigned comparison - but it isn't!
> 
> You see, the compiler always does an _implicit_ cast in cases like this.
                             ^^^^^^^^^^^^^^^^^^^^^

if that's your concern, we can easily fix this by still moving min/max
to its same form.


#define type_min(type,x,y) \
        ({ type __x = (x), __y = (y); __x < __y ? __x: __y; })
#define type_max(type,x,y) \
        ({ type __x = (x), __y = (y); __x > __y ? __x: __y; })

#define min(x,y) type_min(typeof(x),x,y)
#define max(x,y) type_max(typeof(x),x,y)

no _implicit_ cast and ...

> One of the arguments gets changed invisibly, and that is what kernel
> developers are so upset about.  You don't really know which one without
> thinking hard about it, and that is a source of many hard-to-find bugs.

... joy, we would even know which one.


-- 
mfg,
Dipl.-Ing. H.Rosmanith                    Karrer & Partner Gesellschaft mbH
Freistaedter Str. 236, A-4040 Linz,                   Tel. +43/732/75 71 30
                                                      Fax. +43/732/75 71 44

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ignacio Vazquez-Abrams]

What if:

1) We rename the new min() as type_min(),
2) We bring back the old min(), and
      HERE is the _important_ part:
3) Have min() bitch and moan real loud whenever the two arguments
       aren't of the same type.

That way we can have it both ways: a min() that is easy and convenient, and a
min() that can handle two arguments with different types.

-- 
Ignacio Vazquez-Abrams  <ignacio@openservices.net>

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

Everyone,

	From reading this thread, I believe I have come up with several reasons,
IMHO, why the old min()/max() macros were not usable:

	- They did not take into account non-typesafe comparisons
	- They were too generic
	- Some versions, IIRC, relied on typeof()
	- They did not take into account signed/unsigned conversions

	I have also discovered one problem with the new three-arg min()/max()
macro: it forces both arguments to be the same, thus preventing signed/unsigned
comparisons.

	Thus, I have a humble idea: add another type argument!

	Below is a coding example (just an example!):

	#define min(ta, a, tb, b)	((ta)(a) < (tb)(b) ? (a) : (b))
	#define max(ta, a, tb, b)	((ta)(a) > (tb)(b) ? (a) : (b))

	.....
	long int number[] = { 878593, 786831 };
	short int num[] = { 878, 786 };

	.....
	long_min = min(long int, num[0], long int, number[0]);
	short_min = min(short int, num[0], short int, number[0]);

	long_max = max(long int, num[1], long int, number[1]);
	short_max = max(short int, num[1], short int, number[1]);

	Thus, people can now cast different number types up or down, depending
on what they need, and properly compare their sizes. The new argument also makes
comparisons between signed/unsigned more flexible, because now you can cast
in both directions - get a signed minimum and an unsigned minimum.

	This is an RFC. Flames, anyone? Honestly, this was one of those
"lightbulb" ideas. If people object, I don't mind......

Brad

P.S: /me also wonders if I will need firehoses in my e-mail client ;)
	
> On 28 Aug 2001, Andreas Schwab wrote:
> 
> > Roman Zippel <zippel@linux-m68k.org> writes:
> > 
> > |> Hi,
> > |> 
> > |> Linus Torvalds wrote:
> > [...]
> > |> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> > |> > address the fact that most of the min/max bugs are due to the programmer
> > |> > _indending_ a unsigned compare because he didn't even think about the
> > |> > type.
> 
> 
> #if 0
> If the comparison was made unsigned, cast to the largest natural
> value on the target, while keeping the types and sizes of the
> input variables the same, this macro does about what 99.9999999 percent
> of what everybody wants:
> #endif
> 
> #include <stdio.h>
> 
> #define min(a,b) ((size_t)(a) < (size_t)(b) ? (a) : (b))
> 
> int main()
> {
>     printf("%d\n", min(1,2));
>     printf("%d\n", min(-1,2));
>     printf("%d\n", min(0xffffffff,3));
>     printf("%d\n", min(0x8000,4));
>     printf("%d\n", min(0x7fff,5));
>     printf("%d\n", min(0x80000000,6));
>     printf("%d\n", min(0x7fffffff,7));
> 
>     printf("%ld\n", min(1L,2L));
>     printf("%ld\n", min(-1L,2L));
>     printf("%ld\n", min(0xffffffff,3L));
>     printf("%ld\n", min(0x8000,4L));
>     printf("%ld\n", min(0x7fff,5L));
>     printf("%ld\n", min(0x80000000,6L));
>     printf("%ld\n", min(0x7fffffff,7L));
> 
>     printf("%lu\n", min(1L,2LU));
>     printf("%lu\n", min(-1L,2LU));
>     printf("%lu\n", min(0xffffffff,3LU));
>     printf("%lu\n", min(0x8000,4LU));
>     printf("%lu\n", min(0x7fff,5LU));
>     printf("%lu\n", min(0x80000000,6LU));
>     printf("%lu\n", min(0x7fffffff,7LU));
> 
>     printf("%d\n", min(-1, -2));
>     printf("%d\n", min(-1, 0));
>     printf("%p\n", min((void *)0x80000000, (void *)0x7fffffff));
>     return 0;
> }
> 
> 
> 
> Cheers,
> Dick Johnson
> 
> Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
> 
>     I was going to compile a list of innovations that could be
>     attributed to Microsoft. Once I realized that Ctrl-Alt-Del
>     was handled in the BIOS, I found that there aren't any.
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Lang]

Brad, the three arg min/max changes both values to the common type and
then compares them.

there isn't any need to have two different types becouse you then still
need to decide what the common type you are going to use for the
comparison is.

David Lang

 On Tue, 28 Aug 2001, Brad Chapman wrote:

> Date: Tue, 28 Aug 2001 12:33:17 -0700 (PDT)
> From: Brad Chapman <kakadu_croc@yahoo.com>
> To: linux-kernel@vger.kernel.org
> Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
>
> Everyone,
>
> 	From reading this thread, I believe I have come up with several reasons,
> IMHO, why the old min()/max() macros were not usable:
>
> 	- They did not take into account non-typesafe comparisons
> 	- They were too generic
> 	- Some versions, IIRC, relied on typeof()
> 	- They did not take into account signed/unsigned conversions
>
> 	I have also discovered one problem with the new three-arg min()/max()
> macro: it forces both arguments to be the same, thus preventing signed/unsigned
> comparisons.
>
> 	Thus, I have a humble idea: add another type argument!
>
> 	Below is a coding example (just an example!):
>
> 	#define min(ta, a, tb, b)	((ta)(a) < (tb)(b) ? (a) : (b))
> 	#define max(ta, a, tb, b)	((ta)(a) > (tb)(b) ? (a) : (b))
>
> 	.....
> 	long int number[] = { 878593, 786831 };
> 	short int num[] = { 878, 786 };
>
> 	.....
> 	long_min = min(long int, num[0], long int, number[0]);
> 	short_min = min(short int, num[0], short int, number[0]);
>
> 	long_max = max(long int, num[1], long int, number[1]);
> 	short_max = max(short int, num[1], short int, number[1]);
>
> 	Thus, people can now cast different number types up or down, depending
> on what they need, and properly compare their sizes. The new argument also makes
> comparisons between signed/unsigned more flexible, because now you can cast
> in both directions - get a signed minimum and an unsigned minimum.
>
> 	This is an RFC. Flames, anyone? Honestly, this was one of those
> "lightbulb" ideas. If people object, I don't mind......
>
> Brad
>
> P.S: /me also wonders if I will need firehoses in my e-mail client ;)
>
> > On 28 Aug 2001, Andreas Schwab wrote:
> >
> > > Roman Zippel <zippel@linux-m68k.org> writes:
> > >
> > > |> Hi,
> > > |>
> > > |> Linus Torvalds wrote:
> > > [...]
> > > |> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> > > |> > address the fact that most of the min/max bugs are due to the programmer
> > > |> > _indending_ a unsigned compare because he didn't even think about the
> > > |> > type.
> >
> >
> > #if 0
> > If the comparison was made unsigned, cast to the largest natural
> > value on the target, while keeping the types and sizes of the
> > input variables the same, this macro does about what 99.9999999 percent
> > of what everybody wants:
> > #endif
> >
> > #include <stdio.h>
> >
> > #define min(a,b) ((size_t)(a) < (size_t)(b) ? (a) : (b))
> >
> > int main()
> > {
> >     printf("%d\n", min(1,2));
> >     printf("%d\n", min(-1,2));
> >     printf("%d\n", min(0xffffffff,3));
> >     printf("%d\n", min(0x8000,4));
> >     printf("%d\n", min(0x7fff,5));
> >     printf("%d\n", min(0x80000000,6));
> >     printf("%d\n", min(0x7fffffff,7));
> >
> >     printf("%ld\n", min(1L,2L));
> >     printf("%ld\n", min(-1L,2L));
> >     printf("%ld\n", min(0xffffffff,3L));
> >     printf("%ld\n", min(0x8000,4L));
> >     printf("%ld\n", min(0x7fff,5L));
> >     printf("%ld\n", min(0x80000000,6L));
> >     printf("%ld\n", min(0x7fffffff,7L));
> >
> >     printf("%lu\n", min(1L,2LU));
> >     printf("%lu\n", min(-1L,2LU));
> >     printf("%lu\n", min(0xffffffff,3LU));
> >     printf("%lu\n", min(0x8000,4LU));
> >     printf("%lu\n", min(0x7fff,5LU));
> >     printf("%lu\n", min(0x80000000,6LU));
> >     printf("%lu\n", min(0x7fffffff,7LU));
> >
> >     printf("%d\n", min(-1, -2));
> >     printf("%d\n", min(-1, 0));
> >     printf("%p\n", min((void *)0x80000000, (void *)0x7fffffff));
> >     return 0;
> > }
> >
> >
> >
> > Cheers,
> > Dick Johnson
> >
> > Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
> >
> >     I was going to compile a list of innovations that could be
> >     attributed to Microsoft. Once I realized that Ctrl-Alt-Del
> >     was handled in the BIOS, I found that there aren't any.
> >
> >
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
>
>
> =====
> Brad Chapman
>
> Permanent e-mail: kakadu_croc@yahoo.com
> Current e-mail: kakadu@adelphia.net
> Alternate e-mail: kakadu@netscape.net
>
> __________________________________________________
> Do You Yahoo!?
> Make international calls for as low as $.04/minute with Yahoo! Messenger
> http://phonecard.yahoo.com/
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

Mr. Lang,

--- David Lang <dlang@diginsite.com> wrote:
> Brad, the three arg min/max changes both values to the common type and
> then compares them.

	That's the problem. It prevents you from doing varied conversions.
My new macros allow you to do all three basic conversions (AFAICT) :

	- signed with signed
	- unsigned with unsigned
	- signed with unsigned
	  ^^^^^^^^^^^^^^^^^^^^

	This is the one type of conversion that the three-arg macro DOES NOT
cover. When you provide the ability to do multiple signed/unsigned conversions
on different sizes (signed, unsigned, long, short), then stuff becomes
much easier.

> 
> there isn't any need to have two different types becouse you then still
> need to decide what the common type you are going to use for the
> comparison is.

	What if you don't _want_ a common type? See above. IIRC, this was a major
sticking point when Linus first changedthe macros to use a type argument; the
new macro prevented signed/unsigned conversions. My macro defs allow that.

> 
> David Lang

Brad

> 
>  On Tue, 28 Aug 2001, Brad Chapman wrote:
> 
> > Date: Tue, 28 Aug 2001 12:33:17 -0700 (PDT)
> > From: Brad Chapman <kakadu_croc@yahoo.com>
> > To: linux-kernel@vger.kernel.org
> > Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
> >
> > Everyone,
> >
> > 	From reading this thread, I believe I have come up with several reasons,
> > IMHO, why the old min()/max() macros were not usable:
> >
> > 	- They did not take into account non-typesafe comparisons
> > 	- They were too generic
> > 	- Some versions, IIRC, relied on typeof()
> > 	- They did not take into account signed/unsigned conversions
> >
> > 	I have also discovered one problem with the new three-arg min()/max()
> > macro: it forces both arguments to be the same, thus preventing signed/unsigned
> > comparisons.
> >
> > 	Thus, I have a humble idea: add another type argument!
> >
> > 	Below is a coding example (just an example!):
> >
> > 	#define min(ta, a, tb, b)	((ta)(a) < (tb)(b) ? (a) : (b))
> > 	#define max(ta, a, tb, b)	((ta)(a) > (tb)(b) ? (a) : (b))
> >
> > 	.....
> > 	long int number[] = { 878593, 786831 };
> > 	short int num[] = { 878, 786 };
> >
> > 	.....
> > 	long_min = min(long int, num[0], long int, number[0]);
> > 	short_min = min(short int, num[0], short int, number[0]);
> >
> > 	long_max = max(long int, num[1], long int, number[1]);
> > 	short_max = max(short int, num[1], short int, number[1]);
> >
> > 	Thus, people can now cast different number types up or down, depending
> > on what they need, and properly compare their sizes. The new argument also makes
> > comparisons between signed/unsigned more flexible, because now you can cast
> > in both directions - get a signed minimum and an unsigned minimum.
> >
> > 	This is an RFC. Flames, anyone? Honestly, this was one of those
> > "lightbulb" ideas. If people object, I don't mind......
> >
> > Brad
> >
> > P.S: /me also wonders if I will need firehoses in my e-mail client ;)
> >
> > > On 28 Aug 2001, Andreas Schwab wrote:
> > >
> > > > Roman Zippel <zippel@linux-m68k.org> writes:
> > > >
> > > > |> Hi,
> > > > |>
> > > > |> Linus Torvalds wrote:
> > > > [...]
> > > > |> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> > > > |> > address the fact that most of the min/max bugs are due to the
> programmer
> > > > |> > _indending_ a unsigned compare because he didn't even think about the
> > > > |> > type.
> > >
> > >
> > > #if 0
> > > If the comparison was made unsigned, cast to the largest natural
> > > value on the target, while keeping the types and sizes of the
> > > input variables the same, this macro does about what 99.9999999 percent
> > > of what everybody wants:
> > > #endif
> > >
> > > #include <stdio.h>
> > >
> > > #define min(a,b) ((size_t)(a) < (size_t)(b) ? (a) : (b))
> > >
> > > int main()
> > > {
> > >     printf("%d\n", min(1,2));
> > >     printf("%d\n", min(-1,2));
> > >     printf("%d\n", min(0xffffffff,3));
> > >     printf("%d\n", min(0x8000,4));
> > >     printf("%d\n", min(0x7fff,5));
> > >     printf("%d\n", min(0x80000000,6));
> > >     printf("%d\n", min(0x7fffffff,7));
> > >
> > >     printf("%ld\n", min(1L,2L));
> > >     printf("%ld\n", min(-1L,2L));
> > >     printf("%ld\n", min(0xffffffff,3L));
> > >     printf("%ld\n", min(0x8000,4L));
> > >     printf("%ld\n", min(0x7fff,5L));
> > >     printf("%ld\n", min(0x80000000,6L));
> > >     printf("%ld\n", min(0x7fffffff,7L));
> > >
> > >     printf("%lu\n", min(1L,2LU));
> > >     printf("%lu\n", min(-1L,2LU));
> > >     printf("%lu\n", min(0xffffffff,3LU));
> > >     printf("%lu\n", min(0x8000,4LU));
> > >     printf("%lu\n", min(0x7fff,5LU));
> > >     printf("%lu\n", min(0x80000000,6LU));
> > >     printf("%lu\n", min(0x7fffffff,7LU));
> > >
> > >     printf("%d\n", min(-1, -2));
> > >     printf("%d\n", min(-1, 0));
> > >     printf("%p\n", min((void *)0x80000000, (void *)0x7fffffff));
> > >     return 0;
> > > }
> > >
> > >
> > >
> > > Cheers,
> > > Dick Johnson
> > >
> > > Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
> > >
> > >     I was going to compile a list of innovations that could be
> > >     attributed to Microsoft. Once I realized that Ctrl-Alt-Del
> > >     was handled in the BIOS, I found that there aren't any.
> > >
> > >
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> >
> >
> > =====
> > Brad Chapman
> >
> > Permanent e-mail: kakadu_croc@yahoo.com
> > Current e-mail: kakadu@adelphia.net
> > Alternate e-mail: kakadu@netscape.net
> >
> > __________________________________________________
> > Do You Yahoo!?
> > Make international calls for as low as $.04/minute with Yahoo! Messenger
> > http://phonecard.yahoo.com/
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Lang]

On Tue, 28 Aug 2001, Brad Chapman wrote:

> Mr. Lang,
>
> --- David Lang <dlang@diginsite.com> wrote:
> > Brad, the three arg min/max changes both values to the common type and
> > then compares them.
>
> 	That's the problem. It prevents you from doing varied conversions.
> My new macros allow you to do all three basic conversions (AFAICT) :
>
> 	- signed with signed
> 	- unsigned with unsigned
> 	- signed with unsigned
> 	  ^^^^^^^^^^^^^^^^^^^^
>
> 	This is the one type of conversion that the three-arg macro DOES NOT
> cover. When you provide the ability to do multiple signed/unsigned conversions
> on different sizes (signed, unsigned, long, short), then stuff becomes
> much easier.

there isn't such a thing as a signed with unsigned comparison at the
machine level (as andreas points out).

if you are thinking that there is please try and give a low level (i.e.
binary) example of how the comparison can work.

David Lang



> >
> > there isn't any need to have two different types becouse you then still
> > need to decide what the common type you are going to use for the
> > comparison is.
>
> 	What if you don't _want_ a common type? See above. IIRC, this was a major
> sticking point when Linus first changedthe macros to use a type argument; the
> new macro prevented signed/unsigned conversions. My macro defs allow that.
>
> >
> > David Lang
>
> Brad
>
> >
> >  On Tue, 28 Aug 2001, Brad Chapman wrote:
> >
> > > Date: Tue, 28 Aug 2001 12:33:17 -0700 (PDT)
> > > From: Brad Chapman <kakadu_croc@yahoo.com>
> > > To: linux-kernel@vger.kernel.org
> > > Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
> > >
> > > Everyone,
> > >
> > > 	From reading this thread, I believe I have come up with several reasons,
> > > IMHO, why the old min()/max() macros were not usable:
> > >
> > > 	- They did not take into account non-typesafe comparisons
> > > 	- They were too generic
> > > 	- Some versions, IIRC, relied on typeof()
> > > 	- They did not take into account signed/unsigned conversions
> > >
> > > 	I have also discovered one problem with the new three-arg min()/max()
> > > macro: it forces both arguments to be the same, thus preventing signed/unsigned
> > > comparisons.
> > >
> > > 	Thus, I have a humble idea: add another type argument!
> > >
> > > 	Below is a coding example (just an example!):
> > >
> > > 	#define min(ta, a, tb, b)	((ta)(a) < (tb)(b) ? (a) : (b))
> > > 	#define max(ta, a, tb, b)	((ta)(a) > (tb)(b) ? (a) : (b))
> > >
> > > 	.....
> > > 	long int number[] = { 878593, 786831 };
> > > 	short int num[] = { 878, 786 };
> > >
> > > 	.....
> > > 	long_min = min(long int, num[0], long int, number[0]);
> > > 	short_min = min(short int, num[0], short int, number[0]);
> > >
> > > 	long_max = max(long int, num[1], long int, number[1]);
> > > 	short_max = max(short int, num[1], short int, number[1]);
> > >
> > > 	Thus, people can now cast different number types up or down, depending
> > > on what they need, and properly compare their sizes. The new argument also makes
> > > comparisons between signed/unsigned more flexible, because now you can cast
> > > in both directions - get a signed minimum and an unsigned minimum.
> > >
> > > 	This is an RFC. Flames, anyone? Honestly, this was one of those
> > > "lightbulb" ideas. If people object, I don't mind......
> > >
> > > Brad
> > >
> > > P.S: /me also wonders if I will need firehoses in my e-mail client ;)
> > >
> > > > On 28 Aug 2001, Andreas Schwab wrote:
> > > >
> > > > > Roman Zippel <zippel@linux-m68k.org> writes:
> > > > >
> > > > > |> Hi,
> > > > > |>
> > > > > |> Linus Torvalds wrote:
> > > > > [...]
> > > > > |> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> > > > > |> > address the fact that most of the min/max bugs are due to the
> > programmer
> > > > > |> > _indending_ a unsigned compare because he didn't even think about the
> > > > > |> > type.
> > > >
> > > >
> > > > #if 0
> > > > If the comparison was made unsigned, cast to the largest natural
> > > > value on the target, while keeping the types and sizes of the
> > > > input variables the same, this macro does about what 99.9999999 percent
> > > > of what everybody wants:
> > > > #endif
> > > >
> > > > #include <stdio.h>
> > > >
> > > > #define min(a,b) ((size_t)(a) < (size_t)(b) ? (a) : (b))
> > > >
> > > > int main()
> > > > {
> > > >     printf("%d\n", min(1,2));
> > > >     printf("%d\n", min(-1,2));
> > > >     printf("%d\n", min(0xffffffff,3));
> > > >     printf("%d\n", min(0x8000,4));
> > > >     printf("%d\n", min(0x7fff,5));
> > > >     printf("%d\n", min(0x80000000,6));
> > > >     printf("%d\n", min(0x7fffffff,7));
> > > >
> > > >     printf("%ld\n", min(1L,2L));
> > > >     printf("%ld\n", min(-1L,2L));
> > > >     printf("%ld\n", min(0xffffffff,3L));
> > > >     printf("%ld\n", min(0x8000,4L));
> > > >     printf("%ld\n", min(0x7fff,5L));
> > > >     printf("%ld\n", min(0x80000000,6L));
> > > >     printf("%ld\n", min(0x7fffffff,7L));
> > > >
> > > >     printf("%lu\n", min(1L,2LU));
> > > >     printf("%lu\n", min(-1L,2LU));
> > > >     printf("%lu\n", min(0xffffffff,3LU));
> > > >     printf("%lu\n", min(0x8000,4LU));
> > > >     printf("%lu\n", min(0x7fff,5LU));
> > > >     printf("%lu\n", min(0x80000000,6LU));
> > > >     printf("%lu\n", min(0x7fffffff,7LU));
> > > >
> > > >     printf("%d\n", min(-1, -2));
> > > >     printf("%d\n", min(-1, 0));
> > > >     printf("%p\n", min((void *)0x80000000, (void *)0x7fffffff));
> > > >     return 0;
> > > > }
> > > >
> > > >
> > > >
> > > > Cheers,
> > > > Dick Johnson
> > > >
> > > > Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
> > > >
> > > >     I was going to compile a list of innovations that could be
> > > >     attributed to Microsoft. Once I realized that Ctrl-Alt-Del
> > > >     was handled in the BIOS, I found that there aren't any.
> > > >
> > > >
> > > > -
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at  http://www.tux.org/lkml/
> > >
> > >
> > > =====
> > > Brad Chapman
> > >
> > > Permanent e-mail: kakadu_croc@yahoo.com
> > > Current e-mail: kakadu@adelphia.net
> > > Alternate e-mail: kakadu@netscape.net
> > >
> > > __________________________________________________
> > > Do You Yahoo!?
> > > Make international calls for as low as $.04/minute with Yahoo! Messenger
> > > http://phonecard.yahoo.com/
> > > -
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > >
>
>
> =====
> Brad Chapman
>
> Permanent e-mail: kakadu_croc@yahoo.com
> Current e-mail: kakadu@adelphia.net
> Alternate e-mail: kakadu@netscape.net
>
> __________________________________________________
> Do You Yahoo!?
> Make international calls for as low as $.04/minute with Yahoo! Messenger
> http://phonecard.yahoo.com/
>

Re: [IDEA+RFC] Possible solution for min()/max() war

[Andreas Schwab]

Brad Chapman <kakadu_croc@yahoo.com> writes:

|> Everyone,
|> 
|> 	From reading this thread, I believe I have come up with several reasons,
|> IMHO, why the old min()/max() macros were not usable:
|> 
|> 	- They did not take into account non-typesafe comparisons
|> 	- They were too generic
|> 	- Some versions, IIRC, relied on typeof()
|> 	- They did not take into account signed/unsigned conversions
|> 
|> 	I have also discovered one problem with the new three-arg min()/max()
|> macro: it forces both arguments to be the same, thus preventing signed/unsigned
|> comparisons.

There is no such thing as signed/unsigned comparision in C.  Any
comparison is either signed or unsigned, depending on whether the common
type of arguments after applying the usual arithmetic conversions is
signed or unsigned.

|> 	Thus, I have a humble idea: add another type argument!

This does not bye you anything because the there can only be one common
type anyway.

Andreas.

-- 
Andreas Schwab                                  "And now for something
SuSE Labs                                        completely different."
Andreas.Schwab@suse.de
SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=us-ascii, Size: 2041 bytes --]

Mr. Schwab,

--- Andreas Schwab <schwab@suse.de> wrote:
> Brad Chapman <kakadu_croc@yahoo.com> writes:
> 
> |> Everyone,
> |> 
> |> 	From reading this thread, I believe I have come up with several reasons,
> |> IMHO, why the old min()/max() macros were not usable:
> |> 
> |> 	- They did not take into account non-typesafe comparisons
> |> 	- They were too generic
> |> 	- Some versions, IIRC, relied on typeof()
> |> 	- They did not take into account signed/unsigned conversions
> |> 
> |> 	I have also discovered one problem with the new three-arg min()/max()
> |> macro: it forces both arguments to be the same, thus preventing signed/unsigned
> |> comparisons.
> 
> There is no such thing as signed/unsigned comparision in C.  Any
> comparison is either signed or unsigned, depending on whether the common
> type of arguments after applying the usual arithmetic conversions is
> signed or unsigned.

	Then why, IIRC, are the kernel hackers so upset about how the three-arg
macros _prevent_ signed/unsigned comparisons? Apparently the ability to compare
signed/unsigned variables must have _some_ significance....
	
> 
> |> 	Thus, I have a humble idea: add another type argument!
> 
> This does not bye you anything because the there can only be one common
> type anyway.

	IDGT. What if you don't want a common type? What if you explicitly
_want_ to cast signed "up" to unsigned or unsigned "down" to signed?

> 
> Andreas.

Brad

> 
> -- 
> Andreas Schwab                                  "And now for something
> SuSE Labs                                        completely different."
> Andreas.Schwab@suse.de
> SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
> Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Christopher Friesen]

Brad Chapman wrote:
> 
> Mr. Schwab,
> 
> --- Andreas Schwab <schwab@suse.de> wrote:
> > Brad Chapman <kakadu_croc@yahoo.com> writes:
> >
> > |> Everyone,
> > |>
> > |>    From reading this thread, I believe I have come up with several reasons,
> > |> IMHO, why the old min()/max() macros were not usable:
> > |>
> > |>    - They did not take into account non-typesafe comparisons
> > |>    - They were too generic
> > |>    - Some versions, IIRC, relied on typeof()
> > |>    - They did not take into account signed/unsigned conversions
> > |>
> > |>    I have also discovered one problem with the new three-arg min()/max()
> > |> macro: it forces both arguments to be the same, thus preventing signed/unsigned
> > |> comparisons.
> >
> > There is no such thing as signed/unsigned comparision in C.  Any
> > comparison is either signed or unsigned, depending on whether the common
> > type of arguments after applying the usual arithmetic conversions is
> > signed or unsigned.
> 
>         Then why, IIRC, are the kernel hackers so upset about how the three-arg
> macros _prevent_ signed/unsigned comparisons? Apparently the ability to compare
> signed/unsigned variables must have _some_ significance....

Are they?  The most salient objections I've seen are to do with compatibility
and code maintenance.

Since the underlying hardware has to compare either signed or unsigned numbers,
you will always have to eventually cast to some common type.  The three arg
macro simply makes it explicit *what* that common type is.

-- 
Chris Friesen                    | MailStop: 043/33/F10  
Nortel Networks                  | work: (613) 765-0557
3500 Carling Avenue              | fax:  (613) 765-2986
Nepean, ON K2H 8E9 Canada        | email: cfriesen@nortelnetworks.com

Re: [IDEA+RFC] Possible solution for min()/max() war

[Helge Hafting]

Brad Chapman wrote:
> 
> Mr. Schwab,
> 
[...]
> > There is no such thing as signed/unsigned comparision in C.  Any
> > comparison is either signed or unsigned, depending on whether the common
> > type of arguments after applying the usual arithmetic conversions is
> > signed or unsigned.
> 
>         Then why, IIRC, are the kernel hackers so upset about how the three-arg
> macros _prevent_ signed/unsigned comparisons? Apparently the ability to compare
> signed/unsigned variables must have _some_ significance....

You are misunderstanding this.
It is possible to write something that _looks like_ a signed to unsigned
comparison, i.e.:
int a;
unsigned int b;
...
x = min(a,b);

Looks like a signed to unsigned comparison - but it isn't!

You see, the compiler always does an _implicit_ cast in cases like this.
So the actual code generated by the compiler might become a signed to
signed
comparison, or a unsigned to unsigned comparison.  

One of the arguments gets changed invisibly, and that is what kernel
developers
are so upset about.  You don't really know which one without thinking
hard
about it, and that is a source of many hard-to-find bugs.

A min function with a single type argument force you to select a type,
instead of blindly let the compiler choose one or the other.

Your two-type min has exactly the same problem as the no-type min:
You can specify two different types, but the compiler will then
force it to be only one anyway.  And you may wonder which of the two
it will choose.  And there will be bugs as people assumed signed
when it turned out to be unsigned.

The single-type min let you specify exactly which type will be used
in the comparison.  You may use the type of one of the arguments,
or something different from both.  The type you select will be
the one used anyway, there is no room for doubt or false assumptions.

> 
> >
> > |>    Thus, I have a humble idea: add another type argument!
> >
> > This does not bye you anything because the there can only be one common
> > type anyway.
> 
>         IDGT. What if you don't want a common type? What if you explicitly
> _want_ to cast signed "up" to unsigned or unsigned "down" to signed?

What you ask for is impossible.  You can write code that looks like
it is comparing different types, but that won't actually happen.
The cpu always compares two quantities of the same type, no matter
what you write.  The compiler will force a single type anyway, so
it is nice to state explicit what that single type will be.

Stating two types buys you nothing, as the compiler will
force a single one again.  I don't even know about a cpu that
will do signed to unigned compares - this is why the compiler
works that way.  Common cpu's only do two
kindes of comparisons - signed to signed, and unigned to
unsigned.  Which one depends on which comparison instruction
the compiler inserts.  

A variable don't really have a type on common cpu's.  
That's just a programming language convention.
The machine instructions are typed though, and the single-typed min()
in the kernel mirrors this.


Helge Hafting

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Tobias Diedrich <ranma@gmx.at> wrote:
> Brad Chapman wrote:
> 
> > 	Yes, I'm aware of that. That's why we should try to centralize the #ifdef
> > somewhere, so that everybody sees a unified interface, but at the same time, 
> > do this correctly _and_ _without_ the use of typeof().
> > 
> > 	BTW what is your opinion on the things I suggested?
> 
> Well I'm not really a kernel hacker, but I don't see how it would be possible to
> do it 
> that way without always coding both versions, so I think it's a silly Idea (sorry
> ^^;)
> We would just have to agree on one Version. If I see it correctly the whole Idea
> of
> including it into kernel.h was to remove redundant code and appearently Linus
> decided
> to also force everyone to think about what they are doing when using these macros
> by requiring the additional type argument. I too think that #ifdef's in the code
> are ugly and should be avoided.
> How are you going to centralize the #ifdef and provide a unified interface if
> one version requires two arguments, the other three ?
> Currently the change broke a few things, but those are easy to convert to the new
> interface... They will just have to decide which one to use...
> I don't see any way around that.
> 
> -- 
> Tobias					     PGP-Key: 0x9AC7E0BC

Mr. Diedrich,

	See the e-mail I sent to Mr. Viro; in it I explain one way to provide
a compat interface via a config option, but still allow people to do it the
"right" way. See also the rant at the bottom ;)

Brad


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Tobias Diedrich <ranma@gmx.at> wrote:
> Brad Chapman wrote:
> 
> > 	This way, some hackers can use the two-arg min()/max() inside an #ifdef block,
> > other hackers can use the three-arg min()/max() inside an #ifdef block, 
> > and people who don't care can select either.
> 
> Umm, you know this means some drivers will work only with one setting, unless
> you always code both versions, which would be stupid ? 
> 
> -- 
> Tobias						   PGP-Key: 0x9AC7E0BC

Mr. Diedrich,

	Yes, I'm aware of that. That's why we should try to centralize the #ifdef
somewhere, so that everybody sees a unified interface, but at the same time, 
do this correctly _and_ _without_ the use of typeof().

	BTW what is your opinion on the things I suggested?

Brad


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

[IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

Everyone,

	At the risk of igniting another long-drawn-out flamewar over the
min()/max() macros, I have an idea.

	There is a section in the config dialogs labeled "Kernel hacking."
Under it there is the SysRQ option. Why don't we put an entry under that
dialog and label it "Use new min()/max() macros" and make it a y/n field.
Then we can add dozens of warnings to the help dialog about it, and allow
the user/hacker to select the macro they want.

	In any code which uses the macros, you can simply do this:

#include <linux/config.h>
....
#ifdef CONFIG_USE_NEW_MINMAX
	minimum = min(int, number[0], number[1]);
#else
	minimum = min(number[0], number[1]);
#endif

	This way, some hackers can use the two-arg min()/max() inside an #ifdef block,
other hackers can use the three-arg min()/max() inside an #ifdef block, 
and people who don't care can select either.

	Comments, flames, suggestions, anyone? If the output is good, I'll
publish a patch which will add the Config.in option and default it to
CONFIG_USE_NEW_MINMAX=y, since that was the decree of the Great Penguin Overlord ;)

Brad

=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ben LaHaise]

On Fri, 24 Aug 2001, Brad Chapman wrote:

> 	This way, some hackers can use the two-arg min()/max() inside an #ifdef block,
> other hackers can use the three-arg min()/max() inside an #ifdef block,
> and people who don't care can select either.

Have you no taste?  Use of #ifdef's should be minimised as much as
possible.  For this kind of construct, the spurious preprocessor usage
just makes me want to vomit.

		-ben

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Ben LaHaise <bcrl@redhat.com> wrote:
> On Fri, 24 Aug 2001, Brad Chapman wrote:
> 
> > 	This way, some hackers can use the two-arg min()/max() inside an #ifdef block,
> > other hackers can use the three-arg min()/max() inside an #ifdef block,
> > and people who don't care can select either.
> 
> Have you no taste?  Use of #ifdef's should be minimised as much as
> possible.  For this kind of construct, the spurious preprocessor usage
> just makes me want to vomit.
> 
> 		-ben

Mr. LaHaise,

	Eeek! Sorry. That was just a preliminary. As I see it, there are
several ways to wire this up:

	- make everyone use the new macros (some people are thinking :P)

	- make everyone use #ifdef blocks with a config option (you think it's :P)

	- make min()/max() typeof() wrappers for a switch-style stack of comparison 
	  functions which work on the various types, i.e:
	
	  __u8 minimum = min(one, two) -> __u8 minimum = __u8_min(one, two)

	  (this may be too complex and is probably :P)

	- make min()/max() inline functions, cast things to void, and use memcmp()
	  (this might almost be reasonable, but is probably also :P)

	- stay with the old-style macros (:P, :P, :P)

	What do you think, sir?

Brad
	


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alexander Viro]

On Fri, 24 Aug 2001, Brad Chapman wrote:

> 	- make everyone use the new macros (some people are thinking :P)
> 
> 	- make everyone use #ifdef blocks with a config option (you think it's :P)
> 
> 	- make min()/max() typeof() wrappers for a switch-style stack of comparison 
> 	  functions which work on the various types, i.e:
> 	
> 	  __u8 minimum = min(one, two) -> __u8 minimum = __u8_min(one, two)
> 
> 	  (this may be too complex and is probably :P)

> 
> 	- make min()/max() inline functions, cast things to void, and use memcmp()
> 	  (this might almost be reasonable, but is probably also :P)
> 
> 	- stay with the old-style macros (:P, :P, :P)
> 
> 	What do you think, sir?

	Use different inline functions for signed and unsigned.
Explicitly.

	Any scheme trying to imitate polymorphism with use of cpp/
GNU extensions/whatever is missing the point.  There is _no_ common
operation to extend on several types.  Choice between signed and
unsigned max should be explicit - they are different operations.

	Trying to hide that is C++itis of the worst kind - false
polymorphism that hides only one thing: subtle bugs.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Alexander Viro <viro@math.psu.edu> wrote: 
> On Fri, 24 Aug 2001, Brad Chapman wrote:
> 
> > 	- make everyone use the new macros (some people are thinking :P)
> > 
> > 	- make everyone use #ifdef blocks with a config option (you think it's :P)
> > 
> > 	- make min()/max() typeof() wrappers for a switch-style stack of comparison 
> > 	  functions which work on the various types, i.e:
> > 	
> > 	  __u8 minimum = min(one, two) -> __u8 minimum = __u8_min(one, two)
> > 
> > 	  (this may be too complex and is probably :P) 
> > 
> > 	- make min()/max() inline functions, cast things to void, and use memcmp()
> > 	  (this might almost be reasonable, but is probably also :P)
> > 
> > 	- stay with the old-style macros (:P, :P, :P)
> > 
> > 	What do you think, sir?
> 
> 	Use different inline functions for signed and unsigned.
> Explicitly.

	OK. That sounds reasonable, but do we want to do another forced
change, or do we want to hide it? That seems to be the root of the problem:
keeping the same API but making it work _right_.

> 
> 	Any scheme trying to imitate polymorphism with use of cpp/
> GNU extensions/whatever is missing the point.  There is _no_ common
> operation to extend on several types.  Choice between signed and
> unsigned max should be explicit - they are different operations.

	Well, IIRC I remember someone saying that it was legal to compare
signed/unsigned ints, if you did it in the right order. IMHO that might be
incorrect. And none of the ideas I have suggested use any fancy cpp/GNU rubbish.
The most evil-sounding thing I suggested was #4.

> 
> 	Trying to hide that is C++itis of the worst kind - false
> polymorphism that hides only one thing: subtle bugs.
>

	Ack. (IMHO) I don't like C++ either :P

Brad 


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alexander Viro]

On Fri, 24 Aug 2001, Brad Chapman wrote:

> > > 
> > > 	What do you think, sir?
> > 
> > 	Use different inline functions for signed and unsigned.
> > Explicitly.
> 
> 	OK. That sounds reasonable, but do we want to do another forced
> change, or do we want to hide it? That seems to be the root of the problem:
> keeping the same API but making it work _right_.

Existing API is wrong.  "Hiding" is precisely what's wrong here - we
use the same name for two subtly different functions.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Alexander Viro <viro@math.psu.edu> wrote:
> On Fri, 24 Aug 2001, Brad Chapman wrote:
> > > > 	What do you think, sir?
> > > 
> > > 	Use different inline functions for signed and unsigned.
> > > Explicitly.
> > 
> > 	OK. That sounds reasonable, but do we want to do another forced
> > change, or do we want to hide it? That seems to be the root of the problem:
> > keeping the same API but making it work _right_.
> 
> Existing API is wrong.  "Hiding" is precisely what's wrong here - we
> use the same name for two subtly different functions.

Mr. Viro,

	OK. The existing API is wrong and the new min()/max() macros are the
right way to properly compare values. However, we could always add a config 
option to enable a compatibility macro, which would use typeof() on one of the 
two variables and then call the real min()/max(). Something like this (just an
example):

#ifdef CONFIG_ALLOW_COMPAT_MINMAX
#define proper_min(t, a, b)	((t)(a) < (t)(b) ? (a) : (b))
#define proper_max(t, a, b)	((t)(a) > (t)(b) ? (a) : (b))
#define min(a, b)		proper_min(typeof(a), a, b)
#define max(a, b)		proper_max(typeof(a), a, b)
#else
#define min(t, a, b)		((t)(a) < (t)(b) ? (a) : (b))
#define max(t, a, b)		((t)(a) < (t)(b) ? (a) : (b))
#endif

	Unfortunately, this would violate your idea that "hiding" the real
code behind compatibility stuff is C++esque, and, IYO, an Evil Thing(tm). That
just leaves me with one question to everybody: 	

	<rant type="question" intensity="30%">
	Why did Linus change this out of /dev/null and suddenly run off!?
	</rant>

Brad


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alexander Viro]

On Fri, 24 Aug 2001, Brad Chapman wrote:

> 	OK. The existing API is wrong and the new min()/max() macros are the
> right way to properly compare values. However, we could always add a config 
> option to enable a compatibility macro, which would use typeof() on one of the 
> two variables and then call the real min()/max(). Something like this (just an
> example):
> 
> #ifdef CONFIG_ALLOW_COMPAT_MINMAX
> #define proper_min(t, a, b)	((t)(a) < (t)(b) ? (a) : (b))
> #define proper_max(t, a, b)	((t)(a) > (t)(b) ? (a) : (b))
> #define min(a, b)		proper_min(typeof(a), a, b)
> #define max(a, b)		proper_max(typeof(a), a, b)

_THAT_ _IS_ _WRONG_.  Who the fuck said that we always want type of _first_
argument?  Mind you, IMNSHO Dave had been on a seriously bad trip when he
had added that "type" argument - separate names would be cleaner.  And yes,
it'd be better in prepatch instead of 2.4.9-final.

However, no matter which variant you pick, old code with min/max
was broken.  Unless you are carefully giving right types (preferanly -
with casts) it works only by accidents (if it works at all).

"Compatibility option" is exactly the worst thing in such cases.
It's either changing the whole codebase or not bothering at all.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Alexander Viro <viro@math.psu.edu> wrote:
> 
> 
> On Fri, 24 Aug 2001, Brad Chapman wrote:
> 
> > 	OK. The existing API is wrong and the new min()/max() macros are the
> > right way to properly compare values. However, we could always add a config 
> > option to enable a compatibility macro, which would use typeof() on one of the 
> > two variables and then call the real min()/max(). Something like this (just an
> > example):
> > 
> > #ifdef CONFIG_ALLOW_COMPAT_MINMAX
> > #define proper_min(t, a, b)	((t)(a) < (t)(b) ? (a) : (b))
> > #define proper_max(t, a, b)	((t)(a) > (t)(b) ? (a) : (b))
> > #define min(a, b)		proper_min(typeof(a), a, b)
> > #define max(a, b)		proper_max(typeof(a), a, b)
> 
> _THAT_ _IS_ _WRONG_.  Who the fuck said that we always want type of _first_
> argument?  Mind you, IMNSHO Dave had been on a seriously bad trip when he
> had added that "type" argument - separate names would be cleaner.  And yes,
> it'd be better in prepatch instead of 2.4.9-final.

	I never said we had to use the _first_ argument. We could always do
the _second_. Or we could scrap the whole idea of compatibility with any of the old,
"broken" type-unsafe code and make everybody use the new macros. Period.

> 
> However, no matter which variant you pick, old code with min/max
> was broken.  Unless you are carefully giving right types (preferanly -
> with casts) it works only by accidents (if it works at all).
> 
> "Compatibility option" is exactly the worst thing in such cases.
> It's either changing the whole codebase or not bothering at all.
>

	Ack. Seems like the best way to fix this is to either make everybody
use the new macros, or wait for Linus to show up ;-)

Brad 


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[Rusty Russell]

In message <Pine.GSO.4.21.0108242055410.19796-100000@weyl.math.psu.edu> you wri
te:
> _THAT_ _IS_ _WRONG_.  Who the fuck said that we always want type of _first_
> argument?  Mind you, IMNSHO Dave had been on a seriously bad trip when he
> had added that "type" argument - separate names would be cleaner.  And yes,
> it'd be better in prepatch instead of 2.4.9-final.

We're going in circles.  Linus requested that the explicit type arg be
added.

IIUC Dave's original patch (and I just want to say that Dave is my
hero for pushing this) looked vaguely like the existing code in
include/linux/netfilter.h.  He's mentioned it before but noone
bothered to read it, so I'll quote for you:

/* From arch/i386/kernel/smp.c:
 *
 *	Why isn't this somewhere standard ??
 *
 * Maybe because this procedure is horribly buggy, and does
 * not deserve to live.  Think about signedness issues for five
 * seconds to see why.		- Linus
 */

/* Two signed, return a signed. */
#define SMAX(a,b) ((ssize_t)(a)>(ssize_t)(b) ? (ssize_t)(a) : (ssize_t)(b))
#define SMIN(a,b) ((ssize_t)(a)<(ssize_t)(b) ? (ssize_t)(a) : (ssize_t)(b))

/* Two unsigned, return an unsigned. */
#define UMAX(a,b) ((size_t)(a)>(size_t)(b) ? (size_t)(a) : (size_t)(b))
#define UMIN(a,b) ((size_t)(a)<(size_t)(b) ? (size_t)(a) : (size_t)(b))

/* Two unsigned, return a signed. */
#define SUMAX(a,b) ((size_t)(a)>(size_t)(b) ? (ssize_t)(a) : (ssize_t)(b))
#define SUMIN(a,b) ((size_t)(a)<(size_t)(b) ? (ssize_t)(a) : (ssize_t)(b))

Cheers,
Rusty.
--
Premature optmztion is rt of all evl. --DK

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

In article <E15b9rU-0002vE-00@localhost>,
Rusty Russell  <rusty@rustcorp.com.au> wrote:
>In message <Pine.GSO.4.21.0108242055410.19796-100000@weyl.math.psu.edu> you wri
>te:
>> _THAT_ _IS_ _WRONG_.  Who the fuck said that we always want type of _first_
>> argument?  Mind you, IMNSHO Dave had been on a seriously bad trip when he
>> had added that "type" argument - separate names would be cleaner.  And yes,
>> it'd be better in prepatch instead of 2.4.9-final.
>
>We're going in circles.  Linus requested that the explicit type arg be
>added.

Yes. "requested" is a bit too politic.

It was not davem who added the type argument.  It was me who _required_
that min/max be very much explicitly type-aware, as I consider any other
alternative to be inherently buggy. 

Yes, the new Linux min/max macros are different from the ones people are
used to.  Yes, I expected a lot of flamage.  And no, I don't care one
whit.  Unlike EVERY SINGLE other C version of min/max I've ever seen,
the new Linux kernel versions at least have a fighting chance in hell of
generating correct code.  And that's to a large degree because they
force the user to specify what type he _thinks_ the comparison should be
done in. 

I know people don't understand about the difference between signed and
unsigned compares, and people may not even realize that just by doing
the patches David ended up fixing a few real bugs that were uncovered
simply by virtue of having to think about what kind of comparison it was
supposed to be. 

There were a few alternatives that we looked at (or rather, David
implemented, and I ended up rejecting due to various reasons), but they
all boiled down to "how do we sanely generate min/max functions while at
the same time forcing people to understand the types in question".  Some
of the intermediate patches had the type in the macro name, ie things
like "min_uint()" and "min_slong()".  The final version (ie the one in
2.4.9) was deemed to be the most flexible. 

The problem with putting the "type" in the name of the function/macro
is/was that

 - there's a _lot_ of types. David actually had a version for this, and
   there were separate versions for everything ranging from "int" to
   "uint", to "s32" to "u32", to "size_t" etc etc.

 - Some people would _still_ not understand what the advantage of
   explicit typing would be, so we'd have drivers doing their own
   min/max functions, not understanding the issues about signs and C
   type conversion. 

Thus the current "min(type,x,y)" approach, which fixes both problems.

I have one thing to say to people who do not like the new min/max
functions: most of them probably never even _understood_ the multitude
of bugs that are inherent in the classical

	#define min(x,y) ((x) < (y) ? (x) : (y))

but even so I'm somewhat surprised that this is still debated.  I hoped
it would be over by the time I came back ;)

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alexander Viro]

On Tue, 28 Aug 2001, Linus Torvalds wrote:

> It was not davem who added the type argument.  It was me who _required_
> that min/max be very much explicitly type-aware, as I consider any other
> alternative to be inherently buggy. 

Sure. Any attempt to keep the old min/max and make them automagically
do the right thing is doomed since there is no _single_ right thing
we want from them and choice should be deliberate and explicit. No
arguments here.

> generating correct code.  And that's to a large degree because they
> force the user to specify what type he _thinks_ the comparison should be
> done in. 

<nod>

> The problem with putting the "type" in the name of the function/macro
> is/was that
> 
>  - there's a _lot_ of types. David actually had a version for this, and
>    there were separate versions for everything ranging from "int" to
>    "uint", to "s32" to "u32", to "size_t" etc etc.

Ugh. I have a serious feeling that most of them would be redundant, but...
OK, I guess that somebody might want "unsigned max of x and y % 256" (x
being unsigned char and y - unsigned long).  Yes, with stuff like that
we'd really need a function for every type, but I would be very surprised
if somebody actually needed that (and I'd rather see it written explicitly,
regardless of the mechanism we use).

>  - Some people would _still_ not understand what the advantage of
>    explicit typing would be, so we'd have drivers doing their own
>    min/max functions, not understanding the issues about signs and C
>    type conversion. 

Well, that's what grep is for.  Bet that we'll see MAX, __max, m4x and
all sorts of similar "workarounds", so grep'n'LART will be needed anyway.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Tue, 28 Aug 2001, Alexander Viro wrote:
>
> > The problem with putting the "type" in the name of the function/macro
> > is/was that
> >
> >  - there's a _lot_ of types. David actually had a version for this, and
> >    there were separate versions for everything ranging from "int" to
> >    "uint", to "s32" to "u32", to "size_t" etc etc.
>
> Ugh. I have a serious feeling that most of them would be redundant, but...
> OK, I guess that somebody might want "unsigned max of x and y % 256" (x
> being unsigned char and y - unsigned long).

It doesn't matter if the two things are of a different type - there is
always just one type that the comparison is done in, anyway (and returning
any other type doesn't make any sense to me, at least).

And yes, they _are_ redundant in the sense that "size_t" is obviously the
same as "unsigned int" or "unsigned long" on just about all architectures:
but we obviously don't know that on a source level, so you do end up
needing various different versions.

You could, of course, just always do the type expansion, and really boil
it all down to just "large enough" types, the obvious ones being "unsigned
long" and "signed long". However, that implies either depending on a smart
compiler or accepting an extra type expansion in the resulting binary, and
it also wouldn't work on 64-bit quantities (which NTFS actually _does_
use, for example), so you'd still have to have at least

 - umin()/umax()/smin()/smax() - for "long" and shorter
 - u64min()/u64max()/s64min()s64max() - for "long long" and "u64/s64"

to make sure that the min/max operations do not truncate any bits. Even
so, I'd really want to make sure it gets truncated down too - I'd be
nervous about doing "umax()" on two "char" entities: it's MUCH more clear
what you get from

	x = max(unsigned char, char, char)

than if you have signed chars that are expanded to unsigned long's and
then compared (the compare gets the same result, but how is the final
result of "min()" then expanded, if at all?). It gets worse if one of the
sides is unsigned and the other signed, at which point the "extend to a
larger type" approach doesn't work at all.

In short, I really think you want to have the exact types. And a quick
grep of the 2.4.9 diff shows that at least for "min()" we have at least
the following types already being used: "int" "long" "s64" "u32" "unsigned
int" "unsigned long".

So that's at _least_ 6 different versions of "min()" if you want to make
them different functions.

Or, with the 2.4.9 approach, it's just a single macro (well, and another
one for "max()"). And when somebody needs a new type, he doesn't have to
worry about creating a new instantiation of the macro.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alan Cox]

> Or, with the 2.4.9 approach, it's just a single macro (well, and another
> one for "max()"). And when somebody needs a new type, he doesn't have to
> worry about creating a new instantiation of the macro.

The unfortunate thing is that its min and max as opposed to typed_min and
typed_max (with min/max set up to error), since its now a nightmare to 
maintain compatibility between two allegedly stable releases of the same
kernel, as well as with 2.2

Had it been typed_min(a,b,c) then 2.2 could have stayed compatible and the
glue would have been simple

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Tue, 28 Aug 2001, Alan Cox wrote:
>
> The unfortunate thing is that its min and max as opposed to typed_min and
> typed_max (with min/max set up to error), since its now a nightmare to
> maintain compatibility between two allegedly stable releases of the same
> kernel, as well as with 2.2

Note that 2.2.x does not HAVE a "min/max" function, so that cannot be an
issue.

Yes, some drivers and filesystems did their own private version, but if
they are maintained in both 2.2.x and 2.4.x, then it's obviously very easy
for them to change their private version to match the 2.4.x tree, so I
think this particular argument is rather bogus.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[John Alvord]

On Tue, 28 Aug 2001 12:10:12 +0100 (BST), Alan Cox
<alan@lxorguk.ukuu.org.uk> wrote:

>> Or, with the 2.4.9 approach, it's just a single macro (well, and another
>> one for "max()"). And when somebody needs a new type, he doesn't have to
>> worry about creating a new instantiation of the macro.
>
>The unfortunate thing is that its min and max as opposed to typed_min and
>typed_max (with min/max set up to error), since its now a nightmare to 
>maintain compatibility between two allegedly stable releases of the same
>kernel, as well as with 2.2
>
>Had it been typed_min(a,b,c) then 2.2 could have stayed compatible and the
>glue would have been simple

Does the new min/max definitions hurt portability to and from Linux?

john alvord

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Linus Torvalds wrote:

> I know people don't understand about the difference between signed and
> unsigned compares, and people may not even realize that just by doing
> the patches David ended up fixing a few real bugs that were uncovered
> simply by virtue of having to think about what kind of comparison it was
> supposed to be.

What's wrong with "-Wsign-compare"? You just fixed only a minor amount
of compares.

An explicit type is maintenance problem and cause of other subtle bugs.
When types are changing, the type in the macro is too easily missed and
can cause these funny "but it works on ia32..." errors.

What's wrong with this version?

#define min(a, b) ({            \
        typeof(a) _a = (a);     \
        typeof(b) _b = (b);     \
        _a < _b ? _a : _b;      \
})      

With "-Wsign-compare" the compiler will warn you about wrong usage and
will otherwise automatically do the right thing.

Explicit casts are not good, but such half hidden casts are worse. You
are hiding the problem instead of really fixing it. Why won't you let
the compiler help you?

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Tue, 28 Aug 2001, Roman Zippel wrote:
>
> Linus Torvalds wrote:
>
> > I know people don't understand about the difference between signed and
> > unsigned compares, and people may not even realize that just by doing
> > the patches David ended up fixing a few real bugs that were uncovered
> > simply by virtue of having to think about what kind of comparison it was
> > supposed to be.
>
> What's wrong with "-Wsign-compare"? You just fixed only a minor amount
> of compares.

-Wsign-compare is TOTALLY useless.

The problem with signed compares is not just comparing a signed entity
against a unsigned one. It's quite common to have signed quantities on
both sides, but _intending_ a unsigned comparison or vice versa.

This is simply an area where it's better to make people think about the
types, than to magically try to do the "right" thing.

> What's wrong with this version?

[ Standard stupid min() removed ]

You just fixed the "re-use arguments" bug - which is a bug, but doesn't
address the fact that most of the min/max bugs are due to the programmer
_indending_ a unsigned compare because he didn't even think about the
type.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Linus Torvalds wrote:

> The problem with signed compares is not just comparing a signed entity
> against a unsigned one. It's quite common to have signed quantities on
> both sides, but _intending_ a unsigned comparison or vice versa.

Then it's a bug that should _not_ be fixed in the min macro. Unsigned
values should be hold in unsigned variables. If it's that common, please
show me a sane and realistic example.

> This is simply an area where it's better to make people think about the
> types, than to magically try to do the "right" thing.
> 
> > What's wrong with this version?
> 
> [ Standard stupid min() removed ]

It's not stupid, it does the right thing for the majority of the cases.
A cast just hides the problem. If you need a broken min macro to get
people thinking, you have a much bigger problem.

> You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> address the fact that most of the min/max bugs are due to the programmer
> _indending_ a unsigned compare because he didn't even think about the
> type.

You maybe fixed a few bugs, but this new macro will only cause new
problems in the future. If we change only a single type, you have to
scan all min/max users if they possibly need to be changed too. Thanks
to the cast, the compiler won't even remotely help you finding them.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Tue, 28 Aug 2001, Roman Zippel wrote:
>
> Linus Torvalds wrote:
>
> > The problem with signed compares is not just comparing a signed entity
> > against a unsigned one. It's quite common to have signed quantities on
> > both sides, but _intending_ a unsigned comparison or vice versa.
>
> Then it's a bug that should _not_ be fixed in the min macro. Unsigned
> values should be hold in unsigned variables. If it's that common, please
> show me a sane and realistic example.

I'll show you a real example from drivers/acorn/scsi/acornscsi.c:

	min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);

this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
an integer constant. So the above is actually a signed comparison, and
I'll bet you that was not what the author intended.

Now, this_residual is hopefully never negative, so it doesn't matter.

But how many security bugs have you seen where people just didn't even
_think_ of the user giving invalid values?

Think of code like

	#define BUFFER_SIZE (10)

	char buf[BUFFER_SIZE];

	len = min(user_len, BUFFER_SIZE);
	if (copy_from_user(buf, user, len))
		return -EFAULT;

which is not all that broken. Sure, maybe you _should_ have marked
BUFFER_SIZE explicitly unsigned, but face it, how many people actually do
that?

So "BUFFER_SIZE" is actually a signed value (never mind that it is
obviously positive), and if "user_len" is signed too the above doesn't
actually do what the programmer obviously _meant_ to do.

Does code like the above happen? Sure it does. The input can be signed
because often the user interfaces are defined outside the kernel (think of
the arguments to "bind()" or "setsockopt()", and realize that "socklen_t"
is "int" - the programmer may not even SEE the signedness of the arguments
exactly because they are typedefs).

And did you realize, for example, that of the existing "min()"
implementations, some were inline functions that implicitly cast their
arguments to "int" (and one to "unsigned int")? This way we could also
maintain those kinds of local conventions - where the programmer had
originally (at least in the case of the unsigned int) on purpose made all
"min()" functions unsigned.

That, together with different pieces of code really wanting different
semantics. Let me quote something that got removed:

<include/net/sock.h>:

-static __inline__ int min(unsigned int a, unsigned int b)
-{
-       if (a > b)
-               a = b;
-       return a;
-}
-

<net/khttpd/prototypes.h>:

-/* the TCP/IP stack defines a __BROKEN__ set of min/max functions !! */
-/* So we better define our own.                                      */
-
-/* Broken means: working on unsigned data only, which is not acceptable
-                for kHTTPd and probably a lot of other functions. */
-
-#undef min
-#undef max
-#define min(a,b)  ( (a) < (b) ? (a) : (b) )
-#define max(a,b)  ( (a) > (b) ? (a) : (b) )

Now, just _imagine_ the set of bugs implicit in the fact that part of
networking wants "min()" to be unsigned, while another part (that _uses_
the code that depends on the unsigned "min()") re-defines min() to be
"whatever the arguments are".

Now isn't it better to have a nice macro that _can_ be used for all of
this, and that _does_ explicitly handle the issue of signedness that
different parts of the code has very different opinions on, and that
clearly encodes the assumptions that you have at the exact point of use?

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Henning P. Schmiedehausen]

Linus Torvalds <torvalds@transmeta.com> writes:

>I'll show you a real example from drivers/acorn/scsi/acornscsi.c:

>	min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);

>this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
>an integer constant. So the above is actually a signed comparison, and
>I'll bet you that was not what the author intended.

And the mistake of the author was not to write "unsigned int this_residual".
That's the bug. Not the min() function.

	Regards
		Henning

-- 
Dipl.-Inf. (Univ.) Henning P. Schmiedehausen       -- Geschaeftsfuehrer
INTERMETA - Gesellschaft fuer Mehrwertdienste mbH     hps@intermeta.de

Am Schwabachgrund 22  Fon.: 09131 / 50654-0   info@intermeta.de
D-91054 Buckenhof     Fax.: 09131 / 50654-20   

Re: [IDEA+RFC] Possible solution for min()/max() war

[Russell King]

On Tue, Aug 28, 2001 at 03:44:53PM +0000, Henning P. Schmiedehausen wrote:
> >I'll show you a real example from drivers/acorn/scsi/acornscsi.c:
> >	min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);
> 
> >this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
> >an integer constant. So the above is actually a signed comparison, and
> >I'll bet you that was not what the author intended.
> 
> And the mistake of the author was not to write "unsigned int this_residual".
> That's the bug. Not the min() function.

Hmm, everyone's talking about my code.  And I agree with Henning that
"this_residual" should be unsigned.  Unfortunately, it's defined in the
generic SCSI layer.  I'd really like it to be fixed in the SCSI layer
no matter what the outcome of the min/max debarcle^wdebate is.

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [IDEA+RFC] Possible solution for min()/max() war

[Alan Cox]

> >	min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);
> 
> >this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
> >an integer constant. So the above is actually a signed comparison, and
> >I'll bet you that was not what the author intended.
> 
> And the mistake of the author was not to write "unsigned int this_residual".
> That's the bug. Not the min() function.

Or more likely the author knew that this_residual was going to be positive
in all cases anyway. Its just sloppy typing by the scsi layer.

The typing of min() is something I do agree with Linus about (for once 8)),
and making people think about them is a good idea. When DaveM proposed the
original his patches showed up a bug in the older ixj driver immediately.

Alan

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Alan Cox wrote:

> The typing of min() is something I do agree with Linus about (for once 8)),
> and making people think about them is a good idea. When DaveM proposed the
> original his patches showed up a bug in the older ixj driver immediately.

No doubt, that it fixed bugs now. What I'm worried about is whether it
really avoids bugs in the future and isn't a new source of bugs.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Linus Torvalds wrote:

> I'll show you a real example from drivers/acorn/scsi/acornscsi.c:
> 
>         min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);
> 
> this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
> an integer constant. So the above is actually a signed comparison, and
> I'll bet you that was not what the author intended.
> 
> Now, this_residual is hopefully never negative, so it doesn't matter.

Let's assume it does. Joe Hacker uses the new min macro and uses int
because this_residual is an int. Later he realizes that this_residual
must be an unsigned int. Will he now also automatically change the type
in the min macro?

> But how many security bugs have you seen where people just didn't even
> _think_ of the user giving invalid values?
> 
> Think of code like
> 
>         #define BUFFER_SIZE (10)
> 
>         char buf[BUFFER_SIZE];
> 
>         len = min(user_len, BUFFER_SIZE);
>         if (copy_from_user(buf, user, len))
>                 return -EFAULT;
> 
> which is not all that broken. Sure, maybe you _should_ have marked
> BUFFER_SIZE explicitly unsigned, but face it, how many people actually do
> that?

How does the new macro help here? A negative value is invalid and so
should be tested _explicitly_.

What about code like this:

	if (len > BUFFER_SIZE)
		return -EINVAL;

This is not automagically fixed by this macro. So your new macro fixes a
few problems and hides other problems. I can't call this a solution.

> And did you realize, for example, that of the existing "min()"
> implementations, some were inline functions that implicitly cast their
> arguments to "int" (and one to "unsigned int")? This way we could also
> maintain those kinds of local conventions - where the programmer had
> originally (at least in the case of the unsigned int) on purpose made all
> "min()" functions unsigned.

I'm not arguing against a single (and correct) definition of min/max.
This had to be fixed of course.

> Now isn't it better to have a nice macro that _can_ be used for all of
> this, and that _does_ explicitly handle the issue of signedness that
> different parts of the code has very different opinions on, and that
> clearly encodes the assumptions that you have at the exact point of use?

But this assumption is only encoded once. Are you going to change the
min interface every second release, just to keep people thinking about
the types they're using? Breaking interfaces is fine, but it only works
_once_ and is simply forgotten afterwards. To quote my other argument,
that is still unanswered:

You maybe fixed a few bugs, but this new macro will only cause new
problems in the future. If we change only a single type, you have to
scan all min/max users if they possibly need to be changed too. Thanks
to the cast, the compiler won't even remotely help you finding them.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Mike Castle]

On Tue, Aug 28, 2001 at 06:39:43PM +0200, Roman Zippel wrote:
> Let's assume it does. Joe Hacker uses the new min macro and uses int
> because this_residual is an int. Later he realizes that this_residual
> must be an unsigned int. Will he now also automatically change the type
> in the min macro?


min (typeof(this_residual), this_residual, foo);

Though I'm not sure this would be accepted into the kernel.  :->

mrc
-- 
     Mike Castle      dalgoda@ix.netcom.com      www.netcom.com/~dalgoda/
    We are all of us living in the shadow of Manhattan.  -- Watchmen
fatal ("You are in a maze of twisty compiler features, all different"); -- gcc

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 28, 2001 05:14 pm, Linus Torvalds wrote:
> I'll show you a real example from drivers/acorn/scsi/acornscsi.c:
> 
> 	min(host->scsi.SCp.this_residual, DMAC_BUFFER_SIZE / 2);
> 
> this_residual is "int", and "DMAC_BUFFER_SIZE" is just a #define for
> an integer constant. So the above is actually a signed comparison, and
> I'll bet you that was not what the author intended.
> 
> Now, this_residual is hopefully never negative, so it doesn't matter.

    min(host->scsi.SCp.this_residual, (unsigned) DMAC_BUFFER_SIZE / 2);

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Wed, 29 Aug 2001, Daniel Phillips wrote:
>
>     min(host->scsi.SCp.this_residual, (unsigned) DMAC_BUFFER_SIZE / 2);

Sure.

If you put the type information explicitly, you can get it right.

Which is, btw, _exactly_ why the min() function takes the type explicitly.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 29, 2001 03:13 am, Linus Torvalds wrote:
> On Wed, 29 Aug 2001, Daniel Phillips wrote:
> >
> >     min(host->scsi.SCp.this_residual, (unsigned) DMAC_BUFFER_SIZE / 2);
> 
> Sure.
> 
> If you put the type information explicitly, you can get it right.
> 
> Which is, btw, _exactly_ why the min() function takes the type explicitly.

My point is that proper programming discipline would have prevented the 
problem from arising in the first place.  It would be far more appropriate 
for kernel programmers to exercise such discpline than to treat them like 
babies, breaking well-known syntax in the process.

It seems trivial to pick up all potential min/max problems with the Stanford 
Checker in the case some programmer has been too clueless to think about 
their code as they write it.  A simple policy statement for users of min/max 
would have avoided this entire mess.

Not that I you're going to back down, it just made me feel better to get this 
off my chest ;-)

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Lang]

one question that I thought of in context with the other e-mails in this
thread.

when you write a signed/unsigned comparison is it defined in any standard
which type the compiler should generate or is it somethign that could be
different in different compilers (and versions)

(also when comparing different size items same question)

if there are cases that are not defined in a standard and could vary by
compiler/version then we definantly need to have the current version with
the type argument.

David Lang


 On Wed, 29 Aug 2001,
Daniel Phillips wrote:

> Date: Wed, 29 Aug 2001 17:42:39 +0200
> From: Daniel Phillips <phillips@bonn-fries.net>
> To: Linus Torvalds <torvalds@transmeta.com>
> Cc: Roman Zippel <zippel@linux-m68k.org>, linux-kernel@vger.kernel.org
> Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
>
> On August 29, 2001 03:13 am, Linus Torvalds wrote:
> > On Wed, 29 Aug 2001, Daniel Phillips wrote:
> > >
> > >     min(host->scsi.SCp.this_residual, (unsigned) DMAC_BUFFER_SIZE / 2);
> >
> > Sure.
> >
> > If you put the type information explicitly, you can get it right.
> >
> > Which is, btw, _exactly_ why the min() function takes the type explicitly.
>
> My point is that proper programming discipline would have prevented the
> problem from arising in the first place.  It would be far more appropriate
> for kernel programmers to exercise such discpline than to treat them like
> babies, breaking well-known syntax in the process.
>
> It seems trivial to pick up all potential min/max problems with the Stanford
> Checker in the case some programmer has been too clueless to think about
> their code as they write it.  A simple policy statement for users of min/max
> would have avoided this entire mess.
>
> Not that I you're going to back down, it just made me feel better to get this
> off my chest ;-)
>
> --
> Daniel
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 29, 2001 06:02 pm, David Lang wrote:
> one question that I thought of in context with the other e-mails in this
> thread.
> 
> when you write a signed/unsigned comparison is it defined in any standard
> which type the compiler should generate or is it somethign that could be
> different in different compilers (and versions)

Yes, in the signed/unsigned case the comparison generated is always
unsigned.  This is something that all c programmers are supposed to have 
tattoed on the insides of their eyelids, because if you don't know it
there are all kinds of situations that can bite you, not just min and
max.

> (also when comparing different size items same question)

The narrower is expanded to the size of the wider before being compared.

> if there are cases that are not defined in a standard and could vary by
> compiler/version then we definantly need to have the current version with
> the type argument.

No, these cases are defined perfectly clearly and have been at least
since K&R.

> David Lang
> 
> 
>  On Wed, 29 Aug 2001,
> Daniel Phillips wrote:
> 
> > Date: Wed, 29 Aug 2001 17:42:39 +0200
> > From: Daniel Phillips <phillips@bonn-fries.net>
> > To: Linus Torvalds <torvalds@transmeta.com>
> > Cc: Roman Zippel <zippel@linux-m68k.org>, linux-kernel@vger.kernel.org
> > Subject: Re: [IDEA+RFC] Possible solution for min()/max() war
> >
> > On August 29, 2001 03:13 am, Linus Torvalds wrote:
> > > On Wed, 29 Aug 2001, Daniel Phillips wrote:
> > > >
> > > >     min(host->scsi.SCp.this_residual, (unsigned) DMAC_BUFFER_SIZE / 
2);
> > >
> > > Sure.
> > >
> > > If you put the type information explicitly, you can get it right.
> > >
> > > Which is, btw, _exactly_ why the min() function takes the type 
explicitly.
> >
> > My point is that proper programming discipline would have prevented the
> > problem from arising in the first place.  It would be far more appropriate
> > for kernel programmers to exercise such discpline than to treat them like
> > babies, breaking well-known syntax in the process.
> >
> > It seems trivial to pick up all potential min/max problems with the 
Stanford
> > Checker in the case some programmer has been too clueless to think about
> > their code as they write it.  A simple policy statement for users of 
min/max
> > would have avoided this entire mess.
> >
> > Not that I you're going to back down, it just made me feel better to get 
this
> > off my chest ;-)
> >
> > --
> > Daniel
> > -
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bill Rugolsky Jr.]

On Thu, Aug 30, 2001 at 01:49:54AM +0200, Daniel Phillips wrote:
> On August 29, 2001 06:02 pm, David Lang wrote:
> > when you write a signed/unsigned comparison is it defined in any standard
> > which type the compiler should generate or is it somethign that could be
> > different in different compilers (and versions)
> 
> Yes, in the signed/unsigned case the comparison generated is always
> unsigned.  This is something that all c programmers are supposed to have 
> tattoed on the insides of their eyelids, because if you don't know it
> there are all kinds of situations that can bite you, not just min and
> max.
> 
> > (also when comparing different size items same question)
> 
> The narrower is expanded to the size of the wider before being compared.

Careful.  Operands of relational operators undergo "usual arithmetic
conversions," which are value-preserving, so unsigned char and unsigned
short are promoted to int on platforms where int is value-preserving.
Similarly, unsigned int is promoted to long where long is value-preserving,
as I pointed out earlier in this thread.  (This wasn't always the case,
see below.)

The problem here, of course, is that if you don't know the widths of the
operands, you can't determine the type of the compare.  Change the width
of the operand (to steal some bits in a structure, say) and the signedness
of the comparison changes.

> 
> > if there are cases that are not defined in a standard and could vary by
> > compiler/version then we definantly need to have the current version with
> > the type argument.
> 
> No, these cases are defined perfectly clearly and have been at least
> since K&R.

K&R 1st Edition and the UNIX 7th Edition C reference manual specify that
unsigned dominates.  The "value-preserving" language was crafted for
C89, and appears in K&R, 2nd Edition.  C99 introduces the notion of
integer type "rank" to generalize the rules to extended precision types.

Regards,

   Bill Rugolsky

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Thu, 30 Aug 2001, Daniel Phillips wrote:
>
> Yes, in the signed/unsigned case the comparison generated is always
> unsigned.

Well... No.

If you compare a signed integer with a unsigned char, the char gets
promoted to a _signed_ integer, and the comparison is signed. It is NOT
a unsigned comparison.

And THIS is one example of why it gets complicated.

The C logic for type expansion is just a tad too easy to get wrong, and
the strict type-checking you normally have with well-written ANSI C simply
does not exist for integer types. The compiler will silently just do the
promotion..

Somebody mentioned -Wsign-compare. Try it with the example above. It won't
warn at all, exactly because under C both sides of such a compare have the
_same_ sign, even if one is a "unsigned char", and the other is a "signed
int".

Try it yourself if you don't believe me.

Please guys. The issue of sign in comparisons are a LOT more complicated
than most of you seem to think.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ion Badulescu]

On Wed, 29 Aug 2001 20:28:20 -0700 (PDT), Linus Torvalds <torvalds@transmeta.com> wrote:

> If you compare a signed integer with a unsigned char, the char gets
> promoted to a _signed_ integer, and the comparison is signed. It is NOT
> a unsigned comparison.
> 
[...]
> 
> Somebody mentioned -Wsign-compare. Try it with the example above. It won't
> warn at all, exactly because under C both sides of such a compare have the
> _same_ sign, even if one is a "unsigned char", and the other is a "signed
> int".

And why should the compiler warn at all? The range of "int" completely 
covers the range of "unsigned char", so the result of the comparison will 
always be correct if the types were chosen correctly.

... unless of course the programmer used an unsigned char when what he 
really wanted was a signed char. But in that case even your typed min 
macro won't save him, because what should the forced type be anyway? If 
it's "int", nothing changes; if it's "signed char", you risk truncating 
the int. So you end up with something like

	min(int, a, (char)b)

and I fail to see how this is any better than

	min(a, (char)b)

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Woodhouse]

ionut@cs.columbia.edu said:
> ... unless of course the programmer used an unsigned char when what he
>  really wanted was a signed char. But in that case even your typed min
>  macro won't save him, because what should the forced type be anyway?
> If  it's "int", nothing changes; if it's "signed char", you risk
> truncating  the int. So you end up with something like

> 	min(int, a, (char)b) 

If the programmer wrote that when he really wanted a signed char, he has 
more fundamental brokenness to worry about than the min/max fun.

--
dwmw2

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ion Badulescu]

On Thu, 30 Aug 2001, David Woodhouse wrote:

> 
> 
> ionut@cs.columbia.edu said:
> > ... unless of course the programmer used an unsigned char when what he
> >  really wanted was a signed char. But in that case even your typed min
> >  macro won't save him, because what should the forced type be anyway?
> > If  it's "int", nothing changes; if it's "signed char", you risk
> > truncating  the int. So you end up with something like
> 
> > 	min(int, a, (char)b) 
> 
> If the programmer wrote that when he really wanted a signed char, he has 
> more fundamental brokenness to worry about than the min/max fun.

Which is precisely my point, we're in violent agreement here...

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Thu, 30 Aug 2001, Ion Badulescu wrote:
> >
> > Somebody mentioned -Wsign-compare. Try it with the example above. It won't
> > warn at all, exactly because under C both sides of such a compare have the
> > _same_ sign, even if one is a "unsigned char", and the other is a "signed
> > int".
>
> And why should the compiler warn at all? The range of "int" completely
> covers the range of "unsigned char", so the result of the comparison will
> always be correct if the types were chosen correctly.

You obviously do not understand the issue at all.

It doesn't _matter_ that the int covers the whole space of "unsigned
char".

What if the "int" happens to be negative?

Right. You get _totally_ different results for a unsigned and a signed
compare. For the signed compare, you do obviously get the "logical"
result, that the negative int is smaller than the unsigned char. But if
you're not _aware_ of the signedness, you may not think about the issues.

Basically, when you compare a "long" to a "xxx", and the xxxx is unsigned,
WHAT ARE THE RESULTS?

And the answer is that you simply DO NOT KNOW! If the xxxx is "unsigned
int", the sign of the compare actually depends on whether "int" is smaller
than "long" on the particular architecture you're compiling for. So the
sign of the compare actually ends up being different on an alpha than on a
x86.

Stating the type you compare in explicitly means that you do not get
surprised.

And not getting surprisied is a good thing.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ion Badulescu]

On Thu, 30 Aug 2001, Linus Torvalds wrote:

> You obviously do not understand the issue at all.

No offence, but you obviously assume too much.

> It doesn't _matter_ that the int covers the whole space of "unsigned
> char".
> 
> What if the "int" happens to be negative?

If the "int" happens to be negative, the comparison will say that the
"int" is smaller than the "unsigned char". This is the expected result.
What exactly is wrong with this?

So I have this number, -200, which is stored in an int. I have this other
number, 200, which is stored in an unsigned char. Everybody in his right
mind will agree that -200 is smaller than 200, the compiler will do just
that, yet you disagree???

> Basically, when you compare a "long" to a "xxx", and the xxxx is unsigned,
> WHAT ARE THE RESULTS?

So now you're turning it around. If you compare a long with unsigned char,
there is no issue. If you compare long with unsigned int, you get the
right result on the alpha and a warning on x86. If you compare long with
unsigned long, you get a warning period.

> And the answer is that you simply DO NOT KNOW! If the xxxx is "unsigned
> int", the sign of the compare actually depends on whether "int" is smaller
> than "long" on the particular architecture you're compiling for. So the
> sign of the compare actually ends up being different on an alpha than on a
> x86.

And read above. You get the right result on the alpha and a warning on the 
x86. I don't see the problem with that.

If you want to be 100% paranoid, change the sign-compare warning into an 
error, so people don't ignore it. That's a much better alternative, and 
gcc supports it.

> Stating the type you compare in explicitly means that you do not get
> surprised.

No. It means I suppress the gcc warning before I get a chance to see it. 
It does NOT necessarily mean I (and by "I" I mean your average programmer) 
think about the range issues, I just think Linux min/max are broken.

> And not getting surprisied is a good thing.

Duplicating types all over the place is a bad thing, however.

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Jamie Lokier]

Ion Badulescu wrote:
> So now you're turning it around. If you compare a long with unsigned char,
> there is no issue.

This is _not_ true.  What Linus said about long vs. unsigned int
depending on the architecture also applies to unsigned char, on those
old 32-bit Crays.  It is legitimate for unsigned char to have the same
size as signed long.

> If you compare long with unsigned int, you get the right result on the
> alpha and a warning on x86. If you compare long with unsigned long,
> you get a warning period.

Precisely.  Buggy code (read: architecture-specific bug) is written, and
the bug isn't noticed until somebody compiles that code on a different
architecture.  Far better to have GCC warn even on the alpha.

> And read above. You get the right result on the alpha and a warning on the 
> x86. I don't see the problem with that.
> 
> If you want to be 100% paranoid, change the sign-compare warning into an 
> error, so people don't ignore it. That's a much better alternative, and 
> gcc supports it.

See example with sizeof().  Something similar applies with PAGE_SIZE and
other unsigned constants.

> > Stating the type you compare in explicitly means that you do not get
> > surprised.
> 
> No. It means I suppress the gcc warning before I get a chance to see it. 

I have to agree with Ion here.  The explicit type is very helpful, but
if it reduces argument range, it masks a real bug.  Not that the old
macros were any better.  Ideally, there should be some way to get a
warning out of GCC when a bug is masked in this way.

This is what I see as the closest to ideal:

   1. min/max with type argument, as in new kernels.

   2. Warning added to GCC for casts which reduce argument range, but
      only when explicitly requested by an attribute on the cast...

   3. Warning added to GCC for signed vs. unsigned comparisons
      _regardless_ of type size.  This would also catch erroneous
      unsigned char vs. EOF checks in misuses of stdio.

      However, a type attribute must be provided to make the result of
      sizeof() not return a warning.  (size_t _should_ return a warning,
      though, so it is not that simple).

As you can see, at least the kernel part is done :)

-- Jamie

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Jamie Lokier wrote:

>    2. Warning added to GCC for casts which reduce argument range, but
>       only when explicitly requested by an attribute on the cast...

How many false positives will this produce? I think, Linus will hate
this warning even more than -Wsign-compare.

>    3. Warning added to GCC for signed vs. unsigned comparisons
>       _regardless_ of type size.  This would also catch erroneous
>       unsigned char vs. EOF checks in misuses of stdio.

Do you know of such bug in the context of min()?

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Jamie Lokier]

Roman Zippel wrote:
> >    3. Warning added to GCC for signed vs. unsigned comparisons
> >       _regardless_ of type size.  This would also catch erroneous
> >       unsigned char vs. EOF checks in misuses of stdio.
> 
> Do you know of such bug in the context of min()?

I don't know of an actual example.  This one is made up:

       min (int, len, big_size)

Now if big_size has unsigned type, and does not fit in the range of int,
this expression will return the value of big_size cast to int, i.e. a
negative value.  The suggested warning would catch this potential bug.

I don't know if it would warn for too many other things.  Certainly, a
sizeof() exception (don't warn about signed comparison with sizeof()
result) is essential; perhaps too many other exceptions are required
too.

-- Jamie

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ben LaHaise]

On Thu, 30 Aug 2001, Linus Torvalds wrote:

> And not getting surprisied is a good thing.

Most of us don't really care about the details of the whole min/max thing
(my only real care is that it should've been called anything other than
min or max, typed_min perhaps, but that's a secondary issue, more relevant
to forwards compatibility of 2.4/2.2/2.0 hybrid drivers).  What we do care
about is the fact that a release was done without a prepatch to look over
to give people a chance to fix any resulting breakage.

		-ben

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Linus Torvalds wrote:"
> What if the "int" happens to be negative?

   if sizeof(typeof(a)) != sizeof(typeof(b)) 
       BUG() // sizes differ
   const (typeof(a)) _a = ~(typeof(a))0   
   const (typeof(b)) _b = ~(typeof(b))0   
   if _a < 0 && _b > 0 || _a > 0 && b < 0
       BUG() // one signed, the other unsigned
   standard_max(a,b)


Peter 

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Weinehall]

On Thu, Aug 30, 2001 at 06:38:40PM +0200, Peter T. Breuer wrote:
> "Linus Torvalds wrote:"
> > What if the "int" happens to be negative?
> 
>    if sizeof(typeof(a)) != sizeof(typeof(b)) 
>        BUG() // sizes differ
>    const (typeof(a)) _a = ~(typeof(a))0   
>    const (typeof(b)) _b = ~(typeof(b))0   
>    if _a < 0 && _b > 0 || _a > 0 && b < 0
>        BUG() // one signed, the other unsigned
>    standard_max(a,b)

<disgusting vomit-sound>Do you really want code like that in the
kernel (yes, I know that there are code that can compete with it in
ugliness</disgusting vomit-sound>


/David Weinehall
  _                                                                 _
 // David Weinehall <tao@acc.umu.se> /> Northern lights wander      \\
//  Project MCA Linux hacker        //  Dance across the winter sky //
\>  http://www.acc.umu.se/~tao/    </   Full colour fire           </

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago David Weinehall wrote:"
> On Thu, Aug 30, 2001 at 06:38:40PM +0200, Peter T. Breuer wrote:
> > "Linus Torvalds wrote:"
> > > What if the "int" happens to be negative?
> > 
> >    if sizeof(typeof(a)) != sizeof(typeof(b)) 
> >        BUG() // sizes differ
> >    const (typeof(a)) _a = ~(typeof(a))0   
> >    const (typeof(b)) _b = ~(typeof(b))0   
> >    if _a < 0 && _b > 0 || _a > 0 && b < 0
> >        BUG() // one signed, the other unsigned
> >    standard_max(a,b)
> 
> <disgusting vomit-sound>Do you really want code like that in the
> kernel (yes, I know that there are code that can compete with it in

Have you read it? Do you not realize That it optimizes down to BUG() or
the standard min/max? By BUG(), I was hoping that somebody could 
produce some magic that doesn't even compile when it reduces to that,
but maybe that's too much to hope for from gcc.

> ugliness</disgusting vomit-sound>

If that's the limit of the audiences intelligence, I don't think
I'll bother bothering to think next time! I rather hoped instead
for people to pick up the idea and tell me about the problem
left to cover (void* ? what if the arch does not have 2's complement
arithmetic?)

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 30, 2001 09:51 pm, David Weinehall wrote:
> On Thu, Aug 30, 2001 at 06:38:40PM +0200, Peter T. Breuer wrote:
> > "Linus Torvalds wrote:"
> > > What if the "int" happens to be negative?
> > 
> >    if sizeof(typeof(a)) != sizeof(typeof(b)) 
> >        BUG() // sizes differ
> >    const (typeof(a)) _a = ~(typeof(a))0   
> >    const (typeof(b)) _b = ~(typeof(b))0   
> >    if _a < 0 && _b > 0 || _a > 0 && b < 0
> >        BUG() // one signed, the other unsigned
> >    standard_max(a,b)
> 
> <disgusting vomit-sound>Do you really want code like that in the
> kernel (yes, I know that there are code that can compete with it in
> ugliness</disgusting vomit-sound>

Well, loook, if the signedness and widths match all the tests get optimized 
out as constant expressions, if they don't it barfs with BUG and the author 
has to insert specify saner types for the input variables.  It's nice.

This accomplishes what 3arg_min/max set out to do without messing up the 
syntax.

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[Patrick J. LoPresti]

"Peter T. Breuer" <ptb@it.uc3m.es> writes:

> "Linus Torvalds wrote:"
> > What if the "int" happens to be negative?
> 
>    if sizeof(typeof(a)) != sizeof(typeof(b)) 
>        BUG() // sizes differ
>    const (typeof(a)) _a = ~(typeof(a))0   
>    const (typeof(b)) _b = ~(typeof(b))0   
>    if _a < 0 && _b > 0 || _a > 0 && b < 0
>        BUG() // one signed, the other unsigned
>    standard_max(a,b)

This is a MUCH nicer solution.  max() is a well-defined mathematical
concept; it is simply the larger of its two arguments, period.  It is
C's *promotion* rules that kill you, especially signed->unsigned
promotion.  So just forbid them, at least when they implicit.

You can argue about whether the "differing sizes" case should be a
BUG(), since the output will still be mathematically correct.  It
depends on how often it is useful to compare (say) unsigned chars
against ints, and on whether the compiler warns about cases where you
try to stuff the return value into a too-small container.  I bet that
just forbidding signed->unsigned promotion would be enough.

In general, types should work for the programmer, not the other way
around.  Force people to "think hard" about their types when they are
making a likely mistake, not *every* time they call max().

Littering all those calls with types is just gross.

 - Pat

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Patrick J. LoPresti wrote:"
> This is a MUCH nicer solution.  max() is a well-defined mathematical
> concept; it is simply the larger of its two arguments, period.  It is
> C's *promotion* rules that kill you, especially signed->unsigned
> promotion.  So just forbid them, at least when they implicit.
> 
> You can argue about whether the "differing sizes" case should be a
> BUG(), since the output will still be mathematically correct.  It

To give you all something definite to look at, here's some test code:

// standard good 'ol faithful version
#define __MIN(x,y) ({\
   typeof(x) _x = x; \
   typeof(y) _y = y; \
   _x < _y ? _x : _y ; \
 })

// possible implemetation with type sanity checks - alter to taste
#define MIN(x,y) ({\
   const typeof(x) _x = ~(typeof(x))0; \
   const typeof(y) _y = ~(typeof(y))0; \
   void MIN_BUG(); \
   if (sizeof(_x) != sizeof(_y)) \
     MIN_BUG(); \
   if ((_x > 0 && _y < 0) || (_x < 0 && _y > 0)) \
     MIN_BUG(); \
   __MIN(x,y); \
 })

// test code that compiles with no complaints with -O1 -Wall
int main() {
  unsigned i = 1;
  unsigned j = -2;
  return MIN(i,j);
}

// test code that complains at link time (in this version) with ..
//  gcc -o test -O1 -Wall test.c
// /tmp/cczJbwv5.o: In function `main':
// /tmp/cczJbwv5.o(.text+0x7): undefined reference to `MIN_BUG'
// collect2: ld returned 1 exit status
//
int main() {
  unsigned i = 1;
  signed j = -2;
  return MIN(i,j);
}

> depends on how often it is useful to compare (say) unsigned chars
> against ints, and on whether the compiler warns about cases where you
> try to stuff the return value into a too-small container.  I bet that
> just forbidding signed->unsigned promotion would be enough.

Possibly. I have little clue as to the real extent of the problem.

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Fri, 31 Aug 2001, Peter T. Breuer wrote:
>
> To give you all something definite to look at, here's some test code:

Hmm.. This might be a good idea, actually. Have you tried whether it finds
something in the existing tree (you could just take the existing macro and
ignore the first argument)?

This would definitely be acceptable to me, and should (assuming no gcc
optimization bugs) work with no run-time overhead.

Thanks,

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Linus Torvalds wrote:"
> 
> On Fri, 31 Aug 2001, Peter T. Breuer wrote:
> >
> > To give you all something definite to look at, here's some test code:
> 
> Hmm.. This might be a good idea, actually. Have you tried whether it finds
> something in the existing tree (you could just take the existing macro and

Yes. I just tried it. The first warning thrown up for 2.4.8 was in
tun.c, when I did a make modules. Obviously it all depends on my
  .config as to what it finds!  I put in a asm(".error_here") instead of
BUG() so the compilation stops at every problem instead of warning and
continuing. Hence I don't kow the total. I can try and see ..

It's very late here (spain) so I don't trust myself to do much
hacking of code right now .. I corrected the tun.c code (it was
harmless) and posted the first bit of output a little while ago.
It should be down your mail client page a bit.

> ignore the first argument)?
> 
> This would definitely be acceptable to me, and should (assuming no gcc
> optimization bugs) work with no run-time overhead.


Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Linus Torvalds wrote:"
> 
> On Fri, 31 Aug 2001, Peter T. Breuer wrote:
> >
> > To give you all something definite to look at, here's some test code:
> 
> Hmm.. This might be a good idea, actually. Have you tried whether it finds
> something in the existing tree (you could just take the existing macro and
> ignore the first argument)?

I only have source for 2.4.8 plus xfs 1.0 patches here. 

I removed MIN/min/MAX/max definitions from about a dozen header files
in include/linux, and added an #include <linux/minmax.h> to them. That
file goes below. The test are fairly alarmist.

On doing a make bzImage no errors showed. On make modules, 4 errors
showed, one in tun.c and 3 in pagebuf_io.c (I presume that's from
xfs).

I probably covered about 25% of the kernel code in that trial.

The tun.c one was safe as it was .. the signed value was always
positive at the point where the macro was applied. I'm not
at all sure about the pagebuf_io.c ones because there was deep iovec
and iobuf and skb magic about,  but that's not of interest
to you. I made the comparisons signed and added an if (value out of
signed range) BUG(); just above.

The macros below seem to show up the line number perfectly in the
compiletime error. I guess one can add a __FILE__ to them someway.

One thing that worries me is that these should have triggered on
the signed/unsigned char comparisons that you were worried about in
-Wsigned-compare, and no, they didn't. They did find other
signed/unsigned mixes, but not char.

Peter



#ifndef MINMAX_H
#define MINMAX_H 1


#define __MINMAX_S(x) #x
#define MINMAX_S(x) __MINMAX_S(x)
#define MINMAX_BUG asm(".unsafe_min_or_max_at_line_" MINMAX_S(__LINE__))

#define __MIN(x,y) ({\
   typeof(x) _x = x; \
   typeof(y) _y = y; \
   _x < _y ? _x : _y ; \
 })
#define MIN(x,y) ({\
   const typeof(x) _x = ~(typeof(x))0; \
   const typeof(y) _y = ~(typeof(y))0; \
   if (sizeof(_x) != sizeof(_y)) \
     MINMAX_BUG; \
   if ((_x > (typeof(x))0 && _y < (typeof(y))0) \
   ||  (_x < (typeof(x))0 && _y > (typeof(y))0)) \
     MINMAX_BUG; \
   __MIN(x,y); \
 })
#define __MAX(x,y) ({\
   typeof(x) _x = x; \
   typeof(y) _y = y; \
   _x > _y ? _x : _y ; \
 })
#define MAX(x,y) ({\
   const typeof(x) _x = ~(typeof(x))0; \
   const typeof(y) _y = ~(typeof(y))0; \
   if (sizeof(_x) != sizeof(_y)) \
     MINMAX_BUG; \
   if ((_x > (typeof(x))0 && _y < (typeof(y))0) \
   ||  (_x < (typeof(x))0 && _y > (typeof(y))0)) \
     MINMAX_BUG; \
   __MAX(x,y); \
 })


#ifndef min
#define min(x,y) MIN(x,y)
#endif
#ifndef max
#define max(x,y) MAX(x,y)
#endif


#endif

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Fri, 31 Aug 2001, Peter T. Breuer wrote:
>
> On doing a make bzImage no errors showed. On make modules, 4 errors
> showed, one in tun.c and 3 in pagebuf_io.c (I presume that's from
> xfs).
>
> I probably covered about 25% of the kernel code in that trial.

Good. This implies that the test itself is "reasonable and safe" - it
doesn't get nasty false positives, and assuming people en dup being happy
about it never causing surprising sign changes, I think this is a good
idea.

I _would_ suggest that you change the MIN_BUG() thing to the traditional

	#define compile_time_assert(x) \
		do { switch (0) { case 0: case (x) != 0: ; } } while (0)

which gives the error from the compiler, and does not depend on the
assembler.

(Admittedly the error message isn't the nicest in the world, but it gives
file and line number and include depth etc.. )

> The tun.c one was safe as it was .. the signed value was always
> positive at the point where the macro was applied.

Note that a _few_ false positives are fine - we can fix them up, and
people will be happy. The problems happen if the false positives are so
numerous that it makes common code uglier and harder to read because of
work-arounds for checkers.

So adding a cast or using an unsigned constant or whatever is fine.

> One thing that worries me is that these should have triggered on
> the signed/unsigned char comparisons that you were worried about in
> -Wsigned-compare, and no, they didn't. They did find other
> signed/unsigned mixes, but not char.

What I _really_ think might be interesting is a

	-Wsign-promote

warnign that hits outside compares (at any implicit promotion that
changes the sign - ie exactly the cases where K&R v1 and v2 differ), but I
suspect that it will have even _more_ problems than -Wsign-compare in the
sense that it probably really needs the compiler to do range analysis for
any amount of sane output..

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Linus Torvalds wrote:"
> I _would_ suggest that you change the MIN_BUG() thing to the traditional
> 
> 	#define compile_time_assert(x) \
> 		do { switch (0) { case 0: case (x) != 0: ; } } while (0)
> 
> which gives the error from the compiler, and does not depend on the
> assembler.

OK. I did that here (inverting the logic from the if blah then bug form).
It goes just as well. Do you want this made up as a patch for 2.4.9?
Leaving code that thereby becomes non-compiling to its owner's tender
care?

What assertions on the types do you want, btw? People have been
saying that unequal size types are ok, and that it's just signed vs
unsigned that's the problem, and even then only when it's the longer
type that's unsigned and the shorter that's signed. I'd be inclined
to be paranoid and leave the alarms ringing when sizes or signs don't
match exactly.

> What I _really_ think might be interesting is a
> 
> 	-Wsign-promote
> 
> warnign that hits outside compares (at any implicit promotion that
> changes the sign - ie exactly the cases where K&R v1 and v2 differ), but I
> suspect that it will have even _more_ problems than -Wsign-compare in the

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

On Fri, 31 Aug 2001, Peter T. Breuer wrote:

>    if (sizeof(_x) != sizeof(_y)) \
>      MIN_BUG(); \

What bug are you trying to fix here?

> int main() {
>   unsigned i = 1;
>   signed j = -2;
>   return MIN(i,j);
> }

Try -Wsign-compare.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Roman Zippel wrote:"
> On Fri, 31 Aug 2001, Peter T. Breuer wrote:
> 
> >    if (sizeof(_x) != sizeof(_y)) \
> >      MIN_BUG(); \
> 
> What bug are you trying to fix here?

Wake up!

> > int main() {
> >   unsigned i = 1;
> >   signed j = -2;
> >   return MIN(i,j);
> > }
> 
> Try -Wsign-compare.

Wake up harder!

Try reading the last 10 days kernel messages. The last 48 hours are
particularly rewarding.

To be honest, nobody has precisely formulated the problem. I'll attempt
a quick summary of the most salient:

  C silently transforms signed int to unsigned int in cross-signed
  comparisons. This results in 1U < -2, and gives rise to all kinds
  of error paths from min/max codes (in particular, but they're not
  all) of the form

     min(unsigned_positive_constant, signed_ok_or_error_value)

  whose authors were expecting to get the error value out when the
  error value went in!

There are apparently more problems too, but nobody has explained them
to me in a manner that I can comprehend. I suspect that nobody knows
the full range of possible faults. I put in the size comparison
that you remarked upon so that people could tell me about it. Tell me
about it.

Linus wants possible mistakes flagged. He specifically does not want
-Wsign-compare because it apparently gives false positives.
  

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

On Fri, 31 Aug 2001, Peter T. Breuer wrote:

> > What bug are you trying to fix here?
>
> Wake up!

I'm trying.

> Try reading the last 10 days kernel messages. The last 48 hours are
> particularly rewarding.

I have, but I only get the feeling, we're hunting here for imaginary bugs.
Real bugs could be found with -Wsign-compare, but nobody wants to use it
because our master doesn't want it...
Please define the bugs first, you're trying to fix! If you don't like
-Wsign-compare, consider defining rules for the Stanford checker. This way
you can check all compares and not just the few uses of min.

>   C silently transforms signed int to unsigned int in cross-signed
>   comparisons. This results in 1U < -2, and gives rise to all kinds
>   of error paths from min/max codes (in particular, but they're not
>   all) of the form

Care to give an example? For the cases I tried gcc gave a warning.

> Linus wants possible mistakes flagged. He specifically does not want
> -Wsign-compare because it apparently gives false positives.

diff -u -r1.1.1.23 Makefile
--- Makefile	16 Aug 2001 20:50:22 -0000	1.1.1.23
+++ Makefile	31 Aug 2001 11:15:21 -0000
@@ -87,7 +87,11 @@

 CPPFLAGS := -D__KERNEL__ -I$(HPATH)

-CFLAGS := $(CPPFLAGS) -Wall -Wstrict-prototypes -Wno-trigraphs -O2 \
+WFLAGS := -Wall -Wstrict-prototypes -Wno-trigraphs
+ifdef CONFIG_EXTRA_WARNINGS
+WFLAGS := $(WFLAGS) -Wsign-compare
+endif
+CFLAGS := $(CPPFLAGS) $(WFLAGS) -O2 \
 	  -fomit-frame-pointer -fno-strict-aliasing -fno-common
 AFLAGS := -D__ASSEMBLY__ $(CPPFLAGS)

diff -u -r1.1.1.15 config.in
--- arch/i386/config.in	21 Jul 2001 12:48:20 -0000	1.1.1.15
+++ arch/i386/config.in	31 Aug 2001 11:12:56 -0000
@@ -390,4 +390,5 @@

 #bool 'Debug kmalloc/kfree' CONFIG_DEBUG_MALLOC
 bool 'Magic SysRq key' CONFIG_MAGIC_SYSRQ
+bool 'Extra compile warnings' CONFIG_EXTRA_WARNINGS
 endmenu
diff -u -r1.1.1.23 Configure.help
--- Documentation/Configure.help	16 Aug 2001 20:55:53 -0000	1.1.1.23
+++ Documentation/Configure.help	31 Aug 2001 11:21:10 -0000
@@ -15766,6 +15766,15 @@
   keys are documented in Documentation/sysrq.txt. Don't say Y unless
   you really know what this hack does.

+Extra compile warnings
+CONFIG_EXTRA_WARNINGS
+  If you say Y here, the compilation will generate lots of extra
+  warnings. Some of them warn about constructions that users generally
+  do not consider questionable, but which occasionally you might wish
+  to check for; others warn about constructions that are necessary or
+  hard to avoid in some cases, and there is no simple way to modify the
+  code to suppress the warning. Unless you look for bugs, say N.
+
 ISDN subsystem
 CONFIG_ISDN
   ISDN ("Integrated Services Digital Networks", called RNIS in France)

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Roman Zippel wrote:"
> On Fri, 31 Aug 2001, Peter T. Breuer wrote:
> > Try reading the last 10 days kernel messages. The last 48 hours are
> > particularly rewarding.
> 
> I have, but I only get the feeling, we're hunting here for imaginary bugs.

As I said, nobody has been too precise about the bugs to me either! I
just want to provide an alternative to the 3arg min/max that would
otherwise have been imposed. Whether we really need either of these
alternatives is another argument.

> Real bugs could be found with -Wsign-compare, but nobody wants to use it
> because our master doesn't want it...
> Please define the bugs first, you're trying to fix! If you don't like
> -Wsign-compare, consider defining rules for the Stanford checker. This way

Stanford checker? Is that a programmable C type checker? If so, lemmee
at it. Have you a URL, btw?

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

"Peter T. Breuer" wrote:

> As I said, nobody has been too precise about the bugs to me either!

So let's be precise first! Please?
I've asked Linus about examples, so far I've seen only buggy
implementations of min() or an example, where min() shouldn't be used in
first place. Maybe I missed something, then I'm deeply sorry for this,
but all I want to know is, what are we fixing here?

> Stanford checker? Is that a programmable C type checker? If so, lemmee
> at it. Have you a URL, btw?

http://verify.stanford.edu/SVC/
You should search the archive to look for some good examples, how it can
help.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Roman Zippel wrote:"
> [ptb wrote]
> > Stanford checker? Is that a programmable C type checker? If so, lemmee
> > at it. Have you a URL, btw?
> 
> http://verify.stanford.edu/SVC/
> You should search the archive to look for some good examples, how it can
> help.

Hmm .. it looks like a model checker, and only for 1st order logic
(i.e. not CTL). It seems very primitive. What's the point of using this
instead of the many sphisticated model checkers and theorem provers out
there?

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

"Peter T. Breuer" wrote:

> Hmm .. it looks like a model checker, and only for 1st order logic
> (i.e. not CTL). It seems very primitive. What's the point of using this
> instead of the many sphisticated model checkers and theorem provers out
> there?

We already have good experience with it. Feel free to try something
else, the more the better.
Nevertheless you have to define the bug and its context first. The
problem with defining some fancy min macro is you have too little
information about the context, so you can mostly only restrict the use
of min, possibly preventing legal uses of it (signed/unsigned compare is
AFAIK the only exception).

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bill Pringlemeir]

>>>>> Peter T Breuer <ptb@it.uc3m.es> writes:

  > To give you all something definite to look at, here's some test
  > code:


  > // standard good 'ol faithful version
  > #define __MIN(x,y) ({\
  >    typeof(x) _x = x; \
  >    typeof(y) _y = y; \
  >    _x < _y ? _x : _y ; \
  >  })

  > // possible implemetation with type sanity checks - alter to taste
  > #define MIN(x,y) ({\
  >    const typeof(x) _x = ~(typeof(x))0; \
  >    const typeof(y) _y = ~(typeof(y))0; \
  >    void MIN_BUG(); \
  >    if (sizeof(_x) != sizeof(_y)) \
  >      MIN_BUG(); \
  >    if ((_x > 0 && _y < 0) || (_x < 0 && _y > 0)) \
  >      MIN_BUG(); \
  >    __MIN(x,y); \
  >  })

[snip]

Sorry, I have been away for a while.  I think that I was talking about
a similar approach when Linus was away and David introduced the 3 arg
min/max.  I just went through 1800 LKML emails and I am not quite
clear what happened.  I have put my code that I posted earlier at the
end of this email.  This was the result of someone's `fanciful comment'
`what if gcc could handle "if(strcmp(typeof(a),typeof(b))!=0)"'.

So here is the important part.  The code Peter has introduced a cast
in the lines "const typeof(x) _x = ~(typeof(x))0;" etc.  I think that
the approach of declaring a variable of the type and initializing to
zero and then comparing "x - 1" _might_ be better.

The version of min() at the end calls the following an error, were
Peter's does not [after removing the sizeof() restriction].

 unsigned char Cx = 1; unsigned int Ix = 1; unsigned long  Lx = 1;
 Ix = min(Cx,Ix); Lx = min(Cx,Lx); 

Here the unsigned char is being promoted to int as discussed before.
I think that this is `more faithful' as what we are try to achieve is
to check the sign that will be used in the min comparison; almost
like applying a proper warned signed selectively to the min/max
macros.

Otherwise, I think we have independently came up with the same thing
and the `error' throwing can of course be changed to whatever.

fwiw,
Bill Pringlemeir.

[start code]

#define bug_paste_chain(a,b) a##b
#define bug_paste(a) bug_paste_chain(BUG_AT_LINE_,a)
#define min(x,y)                                       \
                                                       \
  ({extern void bug_paste(__LINE__) (void);            \
    typeof(x) _x = 0; typeof(y) _y = 0;                \
    if ((_x-1>0 && _y-1<0) || (_x-1<0 && _y-1>0))      \
            bug_paste(__LINE__)();                     \
        _x = (x), _y = (y); (_x>_y)?_y:_x;             \
   }) 

#include <stdio.h>
int main(int argc, char *argv[])
{
      signed char  cx = 1, cy = 2;
      signed short sx = 1, sy = 2;
      signed int   ix = 1, iy = 2;
      signed long  lx = 1, ly = 2;
    unsigned char  Cx = 1, Cy = 2;
    unsigned short Sx = 1, Sy = 2;
    unsigned int   Ix = 1, Iy = 2;
    unsigned long  Lx = 1, Ly = 2;

    cx = min(cx,cy);    Cx = min(Cy,Cx);    sx = min(sx,sy);
    Sx = min(Sx,Sy);    ix = min(iy,ix);    Ix = min(Iy,Ix);
    lx = min(ly,ly);    Lx = min(Lx,Ly);

    /* No warning. */
/*  Lx = (typeof(Lx))min(Lx,&Ly); */

    printf("%d %d %hd %hd %d %d %ld %ld\n", cx, Cx,
           sx,Sx,ix,Ix,lx,Lx);

    cx = -1;    Cx = 0;
    cx = min(cx,cy);    sx = min(cx,sy);
    ix = min(cx,ix);    lx = min(cx,ly);
    Cx = min(Cx,Cx);    Sx = min(Cx,Sy);
    Ix = min(Ly,Ix);    Lx = min(Ix,Ly);

    /* correctly gives warning! */
/*  Ix = min(Cy,Ix);    Lx = min(Cx,Ly); */

    printf("%d %d %hd %hd %d %d %ld %ld\n", cx, Cx,
           sx,Sx,ix,Ix,lx,Lx);

    min((unsigned)ix,sizeof(ix));
/* BUG? printf ("%d\n", min(Iy, 10)); */
    printf ("%d\n", min(cx, 20));

    printf("min(3,4,5) %d\n", min(min(5,4),3));
    
    return 0;
}
/* gcc -Wall -O2 -o test test.c */

[end code]

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Bill Pringlemeir wrote:"
>   > // possible implemetation with type sanity checks - alter to taste
>   > #define MIN(x,y) ({\
>   >    const typeof(x) _x = ~(typeof(x))0; \
>   >    const typeof(y) _y = ~(typeof(y))0; \
>   >    void MIN_BUG(); \
>   >    if (sizeof(_x) != sizeof(_y)) \
>   >      MIN_BUG(); \
>   >    if ((_x > 0 && _y < 0) || (_x < 0 && _y > 0)) \
>   >      MIN_BUG(); \
>   >    __MIN(x,y); \
>   >  })
> 
> min/max.  I just went through 1800 LKML emails and I am not quite
> clear what happened.  I have put my code that I posted earlier at the

The final version evoked a compile-time error instead of BUG(), and
also was more careful about the kind of zero it compares with! Linus
okayed it in principle and "real life" tests showed that it works
and catches what it's supposed to catch. For me it turned up one
kernel report (in tun.c) and three more in the xfs patches. The
trial covered about a quarter of the kernel.

Some people spotted that the error case can probably be reduced to
large signed versus small unsigned, both of size at least int.

> end of this email.  This was the result of someone's `fanciful comment'
> `what if gcc could handle "if(strcmp(typeof(a),typeof(b))!=0)"'.
> 
> So here is the important part.  The code Peter has introduced a cast
> in the lines "const typeof(x) _x = ~(typeof(x))0;" etc.  I think that
> the approach of declaring a variable of the type and initializing to
> zero and then comparing "x - 1" _might_ be better.

I don't know. I intended only to turn the sign bit on. I'm not sure how
1's complement arithmetic works, so I don't know if it works for 1's
complement arithmetic.

> The version of min() at the end calls the following an error, were
> Peter's does not [after removing the sizeof() restriction].
> 
>  unsigned char Cx = 1; unsigned int Ix = 1; unsigned long  Lx = 1;
>  Ix = min(Cx,Ix); Lx = min(Cx,Lx); 
> 
> Here the unsigned char is being promoted to int as discussed before.
> I think that this is `more faithful' as what we are try to achieve is

When someone states what the problem is exactly, and what portion
of it they want to test for, then the test can be modified to suit.
I intended to show that the test can be performed and a two
arg min thereby retained.

> to check the sign that will be used in the min comparison; almost
> like applying a proper warned signed selectively to the min/max
> macros.
> 
> Otherwise, I think we have independently came up with the same thing
> and the `error' throwing can of course be changed to whatever.

Probably!

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Horst von Brand]

"Peter T. Breuer" <ptb@it.uc3m.es> said:

[...]

> I don't know. I intended only to turn the sign bit on. I'm not sure how
> 1's complement arithmetic works, so I don't know if it works for 1's
> complement arithmetic.

C's unsigned arithmetic is 2-complement. Never even seen a 1-complement
machine.
-- 
Dr. Horst H. von Brand                Usuario #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: [IDEA+RFC] Possible solution for min()/max() war

[Mark H. Wood]

On Fri, 7 Sep 2001, Horst von Brand wrote:
[quote snipped]
> C's unsigned arithmetic is 2-complement. Never even seen a 1-complement
> machine.

CDC big iron.  Seymour Cray found he could get a little more speed out of
his design that way.  IIRC SPSS on CDC gear used the -0 value to indicate
a missing case.

-- 
Mark H. Wood, Lead System Programmer   mwood@IUPUI.Edu
Make a good day.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"A month of sundays ago Bill Pringlemeir wrote:"
> Otherwise, I think we have independently came up with the same thing
> and the `error' throwing can of course be changed to whatever.

If you are interested, the last version I had is the following. The
compile_time_assert is linus' idea. We had an illegal assembler
statement there originally, containing the line number and the
error statement. One or the other will do until gcc gets the
__builtin_ct_assert() function.

#define compile_time_assert(x) \
                do { switch (0) { case 0: case (x) != 0: ; } } while (0)

#define __MIN(x,y) ({\
   typeof(x) _x = x; \
   typeof(y) _y = y; \
   _x < _y ? _x : _y ; \
 })
#define MIN(x,y) ({\
   const typeof(x) _x = ~(typeof(x))0; \
   const typeof(y) _y = ~(typeof(y))0; \
   compile_time_assert(sizeof(_x) == sizeof(_y));\
   compile_time_assert( (_x > (typeof(x))0 && _y > (typeof(y))0) \
                    ||  (_x < (typeof(x))0 && _y < (typeof(y))0)); \
   __MIN(x,y); \
 })

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bill Pringlemeir]

>>>>> Peter T Breuer <ptb@it.uc3m.es> writes:

 > If you are interested, the last version I had is the
 > following. The compile_time_assert is linus' idea. We had an
 > illegal assembler statement there originally, containing the line
 > number and the error statement. One or the other will do until gcc
 > gets the __builtin_ct_assert() function.

   #define compile_time_assert(x) \
                   do { switch (0) { case 0: case (x) != 0: ; } } while (0)

   #define __MIN(x,y) ({\
      typeof(x) _x = x; \
      typeof(y) _y = y; \
      _x < _y ? _x : _y ; \
    })
   #define MIN(x,y) ({\
      const typeof(x) _x = ~(typeof(x))0; \
      const typeof(y) _y = ~(typeof(y))0; \
      compile_time_assert(sizeof(_x) == sizeof(_y));\
      compile_time_assert( (_x > (typeof(x))0 && _y > (typeof(y))0) \
                       ||  (_x < (typeof(x))0 && _y < (typeof(y))0)); \
      __MIN(x,y); \
    })

As Horst von Brand noted, most modern machines use 2's complement
arithmetic.  AFAIK, all the Linux targets use 2's complement.  The use
of ones complement, probably pre-dates MMUs.  At any rate, I think
that this might be a little clearer as sign, not complement is what is
being checked.

   #define MIN(x,y) ({\
      const typeof(x) _x = 0; \
      const typeof(y) _y = 0; \
      compile_time_assert(sizeof(_x) == sizeof(_y));\
      compile_time_assert( (_x - 1 > 0 && _y - 1 > 0) \
                       ||  (_x - 1 < 0 && _y - 1 < 0)); \
      __MIN(x,y); \
    })

This should prevent all cases where the __MIN(x,y) macro can screw up
due to sign issues (on that machine).  If you do this, the `sizeof'
check isn't needed.  A MIN(int, long) etc should probably be ok.  The
only caveats are the promotion in the __MIN itself create a sign
mismatch.

However, if the `sizeof' check remains, then you don't have to worry
about this and both versions are equivalent.  Some other things to
worry about is what if the type is already const?  Maybe that works...
What if you try `MIN(x,_x);'.  I think that this is something that
David took care of in the "3 arg min".

regards,
Bill Pringlemeir.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Bill Pringlemeir wrote:"
>       compile_time_assert( (_x - 1 > 0 && _y - 1 > 0) \
>                        ||  (_x - 1 < 0 && _y - 1 < 0)); \
> 
> This should prevent all cases where the __MIN(x,y) macro can screw up
> due to sign issues (on that machine).  If you do this, the `sizeof'

Eh? "0" is a signed integer constant. So your comparisons force promotion
to int when x is smaller size. I'm not sure what type 

     "unsigned foo - signed int"

would have either!  It seems to me that you are making things very
murky, which is precisely what you want to avoid.

> check isn't needed.  A MIN(int, long) etc should probably be ok.  The
> only caveats are the promotion in the __MIN itself create a sign
> mismatch.
> 
> However, if the `sizeof' check remains, then you don't have to worry
> about this and both versions are equivalent.  Some other things to
> worry about is what if the type is already const?  Maybe that works...

No parsum.

> What if you try `MIN(x,_x);'.  I think that this is something that
> David took care of in the "3 arg min".

You can't take care of it. A macro is always vulnerable to name
clashes.


Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

Hi,

> Please guys. The issue of sign in comparisons are a LOT more complicated
> than most of you seem to think.

So why won't you let the compiler help you, even if it's not perfect in
every case?

This is my last attempt to get things IMO right. Most of the arguments
for the new macro were about bugs of the past. No question about this,
this had to be fixed, I'm not arguing about this at all.

I'm trying to get your attention to the bugs of the future. I still
don't understand why you think the new macro will be any better. Why do
you think the average kernel hacker will think about the type of the
compare and will come to the correct conclusion? It's far to easy to use
an int as type argument and forget about it. gcc won't warn about this,
so someone first has to hit this bug, before it gets fixed.

Maybe I'm too dumb, but I still fail to see, what sense it should make
to turn a signed compare into an unsigned one. Either one knows both
signed values are only positive, then the sign of the compare and the
destination doesn't matter at all. If the value could be negative,
better test it explicit:

	if (len < 0 || len > MAX)
		len = MAX;

First, it's far more readable and makes the intention so clear, that
even your average kernel hacker understands it. Second, gcc produces
exactly the same code with this, so there is no need to play type tricks
with the min macro.

Just for fun I enabled -Wsign-compare, enabled most of the drivers and I
got this:

$ grep 'comparison between signed and unsigned' make.log | sort | uniq |
wc -l
   3211

So if you want to get people thinking about the types they are using,
here you have at least 3211 chances.

Please reconsider your decision. IMVHO the new macros are a mistake and
will cause only more trouble than they are worth. If you want to keep
the new macros, please apply the attached patch, as I prefer this
version in the source I'm responsible for.

bye, Roman

[-- Attachment #2: affs.diff --]
[-- Type: text/plain, Size: 4223 bytes --]

Index: include/linux/amigaffs.h
===================================================================
RCS file: /cvsroot/linux-apus/2.3/include/linux/amigaffs.h,v
retrieving revision 1.1.1.3
diff -u -r1.1.1.3 amigaffs.h
--- include/linux/amigaffs.h	2001/07/22 23:22:24	1.1.1.3
+++ include/linux/amigaffs.h	2001/08/30 13:38:24
@@ -122,7 +122,7 @@
 }
 
 
-#define MIN(a, b) ({		\
+#define affs_min(a, b) ({	\
 	typeof(a) _a = (a);	\
 	typeof(b) _b = (b);	\
 	_a < _b ? _a : _b;	\
Index: fs/affs/amigaffs.c
===================================================================
RCS file: /cvsroot/linux-apus/2.3/fs/affs/amigaffs.c,v
retrieving revision 1.6
diff -u -r1.6 amigaffs.c
--- fs/affs/amigaffs.c	2001/08/20 13:23:07	1.6
+++ fs/affs/amigaffs.c	2001/08/30 13:38:24
@@ -507,7 +507,7 @@
 int
 affs_copy_name(unsigned char *bstr, struct dentry *dentry)
 {
-	int len = MIN(dentry->d_name.len, 30);
+	int len = affs_min(dentry->d_name.len, 30);
 
 	*bstr++ = len;
 	memcpy(bstr, dentry->d_name.name, len);
Index: fs/affs/dir.c
===================================================================
RCS file: /cvsroot/linux-apus/2.3/fs/affs/dir.c,v
retrieving revision 1.1.1.6
diff -u -r1.1.1.6 dir.c
--- fs/affs/dir.c	2001/07/22 23:18:04	1.1.1.6
+++ fs/affs/dir.c	2001/08/30 13:38:24
@@ -135,7 +135,7 @@
 				goto readdir_done;
 			}
 
-			namelen = MIN(AFFS_TAIL(sb, fh_bh)->name[0], 30);
+			namelen = affs_min(AFFS_TAIL(sb, fh_bh)->name[0], 30);
 			name = AFFS_TAIL(sb, fh_bh)->name + 1;
 			pr_debug("AFFS: readdir(): filldir(\"%.*s\", ino=%u), hash=%d, f_pos=%x\n",
 				 namelen, name, ino, hash_pos, f_pos);
Index: fs/affs/file.c
===================================================================
RCS file: /cvsroot/linux-apus/2.3/fs/affs/file.c,v
retrieving revision 1.1.1.8
diff -u -r1.1.1.8 file.c
--- fs/affs/file.c	2001/08/20 23:44:37	1.1.1.8
+++ fs/affs/file.c	2001/08/30 13:38:25
@@ -528,7 +528,7 @@
 		bh = affs_bread_ino(inode, bidx, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
-		tmp = MIN(bsize - boff, from - to);
+		tmp = affs_min(bsize - boff, from - to);
 		memcpy(data + from, AFFS_DATA(bh) + boff, tmp);
 		affs_brelse(bh);
 		bidx++;
@@ -558,7 +558,7 @@
 		bh = affs_bread_ino(inode, bidx, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
-		tmp = MIN(bsize - boff, newsize - size);
+		tmp = affs_min(bsize - boff, newsize - size);
 		memset(AFFS_DATA(bh) + boff, 0, tmp);
 		AFFS_DATA_HEAD(bh)->size = cpu_to_be32(be32_to_cpu(AFFS_DATA_HEAD(bh)->size) + tmp);
 		affs_fix_checksum(sb, bh);
@@ -576,7 +576,7 @@
 		bh = affs_getzeroblk_ino(inode, bidx);
 		if (IS_ERR(bh))
 			goto out;
-		tmp = MIN(bsize, newsize - size);
+		tmp = affs_min(bsize, newsize - size);
 		AFFS_DATA_HEAD(bh)->ptype = cpu_to_be32(T_DATA);
 		AFFS_DATA_HEAD(bh)->key = cpu_to_be32(inode->i_ino);
 		AFFS_DATA_HEAD(bh)->sequence = cpu_to_be32(bidx);
@@ -685,7 +685,7 @@
 		bh = affs_bread_ino(inode, bidx, 0);
 		if (IS_ERR(bh))
 			return PTR_ERR(bh);
-		tmp = MIN(bsize - boff, to - from);
+		tmp = affs_min(bsize - boff, to - from);
 		memcpy(AFFS_DATA(bh) + boff, data + from, tmp);
 		AFFS_DATA_HEAD(bh)->size = cpu_to_be32(be32_to_cpu(AFFS_DATA_HEAD(bh)->size) + tmp);
 		affs_fix_checksum(sb, bh);
@@ -731,7 +731,7 @@
 		bh = affs_bread_ino(inode, bidx, 1);
 		if (IS_ERR(bh))
 			goto out;
-		tmp = MIN(bsize, to - from);
+		tmp = affs_min(bsize, to - from);
 		memcpy(AFFS_DATA(bh), data + from, tmp);
 		if (bh->b_state & (1UL << BH_New)) {
 			AFFS_DATA_HEAD(bh)->ptype = cpu_to_be32(T_DATA);
Index: fs/affs/namei.c
===================================================================
RCS file: /cvsroot/linux-apus/2.3/fs/affs/namei.c,v
retrieving revision 1.9
diff -u -r1.9 namei.c
--- fs/affs/namei.c	2001/08/21 17:57:27	1.9
+++ fs/affs/namei.c	2001/08/30 13:38:25
@@ -81,7 +81,7 @@
 		return i;
 
 	hash = init_name_hash();
-	i = MIN(qstr->len, 30);
+	i = affs_min(qstr->len, 30);
 	for (; i > 0; name++, i--)
 		hash = partial_name_hash(toupper(*name), hash);
 	qstr->hash = end_name_hash(hash);
@@ -172,7 +172,7 @@
 	toupper_t toupper = affs_get_toupper(sb);
 	int hash;
 
-	hash = len = MIN(len, 30);
+	hash = len = affs_min(len, 30);
 	for (; len > 0; len--)
 		hash = (hash * 13 + toupper(*name++)) & 0x7ff;
 

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Thu, 30 Aug 2001, Roman Zippel wrote:
>
> So why won't you let the compiler help you, even if it's not perfect in
> every case?

I considered enabling -Wsign-compare a long time ago, and it's not a
question of being "perfect", but a question of being _so_ broken that it's
not funny.

For example, let's look at this perfectly natural code:

	static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
	{
		if (len <= sizeof(short) || len > sizeof(*sunaddr))
			return -EINVAL;
		...

Would you agree that the above is _good_ code, and code that makes
perfect sense, and code that does exactly the right thing in testing its
arguments?

Try to compile it with -Wsign-compare.

You'll get not one, but TWO warnings for code that is totally correct, and
that it would make _no_ sense in writing any other way.

In short, -Wsign-compare (at least with a _lot_ of gcc versions) warns for
totally sane and reasonable code - for code that exists all over the
kernel. The above snippet is in fact directly from the kernel, go look and
see for yourself.

In short, -Wsign-compare is totally useless. The warnings are mostly _so_
bogus that nobody has the energy to even try to figure out which of them
might actually be worthwhile.

Face it, you don't know what you're talking about.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Christopher Friesen]

Linus Torvalds wrote:

> For example, let's look at this perfectly natural code:
> 
>         static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
>         {
>                 if (len <= sizeof(short) || len > sizeof(*sunaddr))
>                         return -EINVAL;
>                 ...
> 
> Would you agree that the above is _good_ code, and code that makes
> perfect sense, and code that does exactly the right thing in testing its
> arguments?

Wouldn't it have made more sense to make the 'len' parameter an unsigned int? 
Presumably we can't have a negative length for a name.  In this case the
warnings should just go away, no?


-- 
Chris Friesen                    | MailStop: 043/33/F10  
Nortel Networks                  | work: (613) 765-0557
3500 Carling Avenue              | fax:  (613) 765-2986
Nepean, ON K2H 8E9 Canada        | email: cfriesen@nortelnetworks.com

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Thu, 30 Aug 2001, Christopher Friesen wrote:
>
> Wouldn't it have made more sense to make the 'len' parameter an unsigned int?

Oh yes.

And wouldn't it be nicer if the sky was pink, and God came personally down
to earth and stopped all wrans and made you king?

You do realize that many things are signed, whether you want them to be or
not?

Like "off_t", which on the face of it sounds like it should be unsigned
("offset within a file - oh, that can obviously never be negative").

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Linus Torvalds wrote:

>         static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
>         {
>                 if (len <= sizeof(short) || len > sizeof(*sunaddr))
>                         return -EINVAL;
>                 ...
> 
> Would you agree that the above is _good_ code, and code that makes
> perfect sense, and code that does exactly the right thing in testing its
> arguments?

Where is the problem to change the type into "unsigned int"?

> Face it, you don't know what you're talking about.

Sorry, but what makes you so sure about it?
Anyway, forget -Wsign-compare, but could you _please_ give me an answer
to my other arguments? Maybe I'm a stupid idiot, but then please
enlighten me and show me the right way. So here is the important part of
my mail again:

This is my last attempt to get things IMO right. Most of the arguments
for the new macro were about bugs of the past. No question about this,
this had to be fixed, I'm not arguing about this at all.

I'm trying to get your attention to the bugs of the future. I still
don't understand why you think the new macro will be any better. Why do
you think the average kernel hacker will think about the type of the
compare and will come to the correct conclusion? It's far too easy to
use an int as type argument and forget about it. gcc won't warn about
this, so someone first has to hit this bug, before it gets fixed.

Maybe I'm too dumb, but I still fail to see, what sense it should make
to turn a signed compare into an unsigned one. Either one knows both
signed values are only positive, then the sign of the compare and the
destination doesn't matter at all. If the value could be negative,
better test it explicit:

        if (len < 0 || len > MAX)
                len = MAX;

First, it's far more readable and makes the intention so clear, that
even your average kernel hacker understands it. Second, gcc produces
exactly the same code with this, so there is no need to play type tricks
with the min macro.

Please reconsider your decision. IMVHO the new macros are a mistake and
will cause only more trouble than they are worth.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Ion Badulescu]

On Thu, 30 Aug 2001 09:21:12 -0700 (PDT), Linus Torvalds <torvalds@transmeta.com> wrote:

> For example, let's look at this perfectly natural code:
> 
>        static int unix_mkname(struct sockaddr_un * sunaddr, int len, unsigned *hashp)
>        {
>                if (len <= sizeof(short) || len > sizeof(*sunaddr))
>                        return -EINVAL;
>                ...
> 
> Would you agree that the above is _good_ code, and code that makes
> perfect sense, and code that does exactly the right thing in testing its
> arguments?

Ugh. I must confess I am disappointed, Linus. I thought you had better taste.

Yes, the above code is correct. And yes, gcc should be more aggressive to
recognize that the above code is unambiguous. But that doesn't change the fact
that the code is UGLY AS HELL. Nor does it change the fact that each comparison
is broken if taken separately.

You make two UNSIGNED comparisons when you clearly mean to make two SIGNED
comparisons, and you call that _good_ code? Hmm.

> Try to compile it with -Wsign-compare.
> 
> You'll get not one, but TWO warnings for code that is totally correct, and
> that it would make _no_ sense in writing any other way.

Really? How so? We _know_ that the result of sizeof() fits confortably within
"int"'s range. So the natural way to write that comparison would be

	if (len <= (int) sizeof(short) || len > (int) sizeof(*sunaddr))

which is 100% correct, 100% obvious, and does not break horribly if you
remove one of the comparisons.

The compiler also _knows_ that. But the compiler can't change the implicit 
cast, because it would make it incompatible with every other compiler on
the planet. Yes, standards suck sometimes. So compiler warns us instead.
I consider that a feature.

Ion

-- 
  It is better to keep your mouth shut and be thought a fool,
            than to open it and remove all doubt.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Thu, 30 Aug 2001, Ion Badulescu wrote:
>
> Really? How so? We _know_ that the result of sizeof() fits confortably within
> "int"'s range. So the natural way to write that comparison would be
>
> 	if (len <= (int) sizeof(short) || len > (int) sizeof(*sunaddr))

You're so full of shit that it's incredible.

I'mnot going to argue this, when people call stuff like the above the
"natural way". This is not worth it.

The fact is, the way gcc currently does things, -Wsign-compare is useless.
Anybody who is honest would admit that. In order for the warning to become
useful, gcc would need to do value range analysis - which people have been
talking about, but which is not there yet.

There have been some constructive comments here (the automatic detection
of bad comparisons etc), but yours is just stupid.

		Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Jamie Lokier]

Linus Torvalds wrote:
> > 	if (len <= (int) sizeof(short) || len > (int) sizeof(*sunaddr))
> 
> You're so full of shit that it's incredible.
> 
> I'mnot going to argue this, when people call stuff like the above the
> "natural way". This is not worth it.

While I agree with Linus that the above line is ugly, there is a problem
with the original line:

    if (len <= sizeof(short) || len > sizeof(*sunaddr))

The problem?  Thinking this is natural, suppose you decide you only need
to check len against sizeof(short), perhaps here, perhaps copying this
idea to another part of the program:

    if (len <= sizeof(short))

_This_ code has a bug.  The comparison is unsigned; i.e. len is
converted to unsigned int first, so if the user passes a negative value,
the comparison returns false and the code will break.  It only works in
the original example because the second comparison checks the bogus
results of the first one.

Now, any later use of len should not lead to a buffer overflow or
anything, because you always check against a maximum size for copying,
yes?

No.  Consider this hypothetical code to copy things up to a page at a
time.  len is a signed type s.t. sizeof(len) <= sizeof(size_t):

   if (len <= sizeof(struct thing) || len > MAX_BUFFER_SIZE) {
           printk(KERN_ERR "Sanity check failed!\n");
           goto out;
   }
   memcpy (to, from, len);

Inexperienced C programmers, and sleepy ones, may not see the fault.

cheers,
-- Jamie

Re: [IDEA+RFC] Possible solution for min()/max() war

[Linus Torvalds]

On Fri, 31 Aug 2001, Jamie Lokier wrote:
>
> While I agree with Linus that the above line is ugly, there is a problem
> with the original line:
>
>     if (len <= sizeof(short) || len > sizeof(*sunaddr))
>
> The problem?  Thinking this is natural, suppose you decide you only need
> to check len against sizeof(short), perhaps here, perhaps copying this
> idea to another part of the program:
>
>     if (len <= sizeof(short))
>
> _This_ code has a bug.

I agree.

The difference is subtle, and maybe too subtle.

The fact is, that _ranges_ are "stable" in different types. If a value "x"
is in the range [a,b] in type A, then it will also be in range [a,b] in
type B (assuming, of course, that 'a' and 'b' are valid values in both
types).

This is why range comparisons are different from normal comparisons.

But C doesn't have the notion of a range, which is why you can't write
(while lots of people have _tried_ to write, including in the kernel)

	a < x < b
or
	x in [a,b]

or similar. So you end up having to use a more "complex" setup, where the
individual parts might not be safe even though the totality is safe.

In fact, if gcc never becomes good enough to do that kind of range
checking, I won't be _too_ unhappy. I selected a special example on bad
grounds - it's not actually the most common case of range checking, and
the most common case by far tends to be something like

	if (len < 0)
		return -EINVAL;

	if (len > sizeof(buffer))
		len = sizeof(buffer);

	copy_from_user(buffer, ..., len);

where the operation is safe for _another_ range check reason, namely the
fact that we check that "len" is within the domain of the second check.

Right now gcc will complain about the second comparison, exactly because
gcc does not do range analysis.

Now, the good news is that gcc people have already worked on range
analysis, because it is very useful for other things too (not the least of
which is optimization). So I bet gcc _will_ be able to do a much better
job of this in the future, and if we have to help it by hand in only a few
places, then that will be a good thing.

However, right now gcc complains _way_ too much about perfectly valid and
good code. If it was a few small cases, I'd be happy to fix them in the
kernel. But last time I tried -Wsign-compare, the false positives were
just too damn numerous.

(Now, we may have fixed some of them anyway, and I haven't tried it in a
_loong_ time. If somebody decides to try to see what happens if you try to
clean them up, that would probably not be a bad idea per se. Proving me
wrong is always a good sport ;)

			Linus

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 30, 2001 05:28 am, Linus Torvalds wrote:
> On Thu, 30 Aug 2001, Daniel Phillips wrote:
> >
> > Yes, in the signed/unsigned case the comparison generated is always
> > unsigned.
> 
> Well... No.
> 
> If you compare a signed integer with a unsigned char, the char gets
> promoted to a _signed_ integer, and the comparison is signed. It is NOT
> a unsigned comparison.

Lets not go into how stupid that is.  Yes, things changed between K&R 
editions 1 and 2, in a misguided attempt to make things less "surprising" the 
drafters just introduced additional confusion.

> And THIS is one example of why it gets complicated.
>
> The C logic for type expansion is just a tad too easy to get wrong, and
> the strict type-checking you normally have with well-written ANSI C simply
> does not exist for integer types. The compiler will silently just do the
> promotion..
> 
> Somebody mentioned -Wsign-compare. Try it with the example above. It won't
> warn at all, exactly because under C both sides of such a compare have the
> _same_ sign, even if one is a "unsigned char", and the other is a "signed
> int".
> 
> Try it yourself if you don't believe me.
> 
> Please guys. The issue of sign in comparisons are a LOT more complicated
> than most of you seem to think.

More than anything, it shows that education is needed, not macro patch-ups.
We have exactly the same issues with < and >, should we introduce 
three-argument macros to replace them?

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[Peter T. Breuer]

"Daniel Phillips wrote:"
> More than anything, it shows that education is needed, not macro patch-ups.
> We have exactly the same issues with < and >, should we introduce 
> three-argument macros to replace them?

# define le(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _a ; })
# define ge(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _b ; })

;-)

Peter

Re: [IDEA+RFC] Possible solution for min()/max() war

[Daniel Phillips]

On August 30, 2001 07:03 pm, Peter T. Breuer wrote:
> "Daniel Phillips wrote:"
> > More than anything, it shows that education is needed, not macro 
patch-ups.
> > We have exactly the same issues with < and >, should we introduce 
> > three-argument macros to replace them?
> 
> # define le(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _a ; })
> # define ge(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _b ; })

Oh, you are one sick puppy.

For completeness:

# define lt(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) != _b ; })
# define gt(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) != _a ; })

--
Daniel

Re: [IDEA+RFC] Possible solution for min()/max() war

[Kai Henningsen]

phillips@bonn-fries.net (Daniel Phillips)  wrote on 30.08.01 in <20010830171934Z16012-32384+1116@humbolt.nl.linux.org>:

> On August 30, 2001 07:03 pm, Peter T. Breuer wrote:
> > "Daniel Phillips wrote:"
> > > More than anything, it shows that education is needed, not macro
> patch-ups.
> > > We have exactly the same issues with < and >, should we introduce
> > > three-argument macros to replace them?
> >
> > # define le(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _a ; })
> > # define ge(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) == _b ; })
>
> Oh, you are one sick puppy.
>
> For completeness:
>
> # define lt(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) != _b ; })
> # define gt(t,a,b) ({ t _a = a; t _b = b;  min(t,_a,_b) != _a ; })

Does it give the right answers for floating point?


MfG Kai

Re: [IDEA+RFC] Possible solution for min()/max() war

[Graham Murray]

Daniel Phillips <phillips@bonn-fries.net> writes:

> More than anything, it shows that education is needed, not macro patch-ups.
> We have exactly the same issues with < and >, should we introduce 
> three-argument macros to replace them?

Would it not have been much more "obvious" if the rules for
unsigned/signed integer comparisons (irrespective of the widths
involved) were

1) If the signed element is negative then it is always less than the
   unsigned element.

2) If the unsigned element is greater than then maximum positive value
   expressible by the signed one then it is always greater.

3) Only if both values are positive and within the range of the
   smaller element are the actual values compared. 

Re: [IDEA+RFC] Possible solution for min()/max() war

[David Weinehall]

On Thu, Aug 30, 2001 at 09:16:47PM +0000, Graham Murray wrote:
> Daniel Phillips <phillips@bonn-fries.net> writes:
> 
> > More than anything, it shows that education is needed, not macro patch-ups.
> > We have exactly the same issues with < and >, should we introduce 
> > three-argument macros to replace them?
> 
> Would it not have been much more "obvious" if the rules for
> unsigned/signed integer comparisons (irrespective of the widths
> involved) were
> 
> 1) If the signed element is negative then it is always less than the
>    unsigned element.
> 
> 2) If the unsigned element is greater than then maximum positive value
>    expressible by the signed one then it is always greater.
> 
> 3) Only if both values are positive and within the range of the
>    smaller element are the actual values compared. 

Possibly, but changing the C specification is not really an option here...


/David Weinehall
  _                                                                 _
 // David Weinehall <tao@acc.umu.se> /> Northern lights wander      \\
//  Project MCA Linux hacker        //  Dance across the winter sky //
\>  http://www.acc.umu.se/~tao/    </   Full colour fire           </

Re: [IDEA+RFC] Possible solution for min()/max() war

[Helge Hafting]

David Weinehall wrote:
> 
> On Thu, Aug 30, 2001 at 09:16:47PM +0000, Graham Murray wrote:
> > Daniel Phillips <phillips@bonn-fries.net> writes:
> >
> > > More than anything, it shows that education is needed, not macro patch-ups.
> > > We have exactly the same issues with < and >, should we introduce
> > > three-argument macros to replace them?
> >
> > Would it not have been much more "obvious" if the rules for
> > unsigned/signed integer comparisons (irrespective of the widths
> > involved) were
> >
> > 1) If the signed element is negative then it is always less than the
> >    unsigned element.
> >
> > 2) If the unsigned element is greater than then maximum positive value
> >    expressible by the signed one then it is always greater.
> >
> > 3) Only if both values are positive and within the range of the
> >    smaller element are the actual values compared.
> 
> Possibly, but changing the C specification is not really an option here...

Even worse: most microprosessors don't do comparisons that way.
They compare either two signed or two unsigned 
items and do that reasonably fast.  This is why C also works this way.

A compiler can be made to use the above standard, but it would generate
slow code for all signed/unsigned compares because now there is
3 tests instead of one.  This is why language designers don't do that.

You can of course do this explicitly in code if you need that
sort of comparison, e.g.

signed a;
unsigned b;
if (a<0) case1() 
   else if (b>MAX_SIGNED) case2()
   else if (a<b) case1() else case2();


A nice feature of C is that ugly time-consuming stuff tends to
look ugly and time-consuming in code too.  So it is easier
to avoid. :-)

Helge Hafting

Re: [IDEA+RFC] Possible solution for min()/max() war

[Patrick J. LoPresti]

Graham Murray <graham@barnowl.demon.co.uk> writes:

> Daniel Phillips <phillips@bonn-fries.net> writes:
> 
> > More than anything, it shows that education is needed, not macro patch-ups.
> > We have exactly the same issues with < and >, should we introduce 
> > three-argument macros to replace them?
> 
> Would it not have been much more "obvious" if the rules for
> unsigned/signed integer comparisons (irrespective of the widths
> involved) were
> 
> 1) If the signed element is negative then it is always less than the
>    unsigned element.
> 
> 2) If the unsigned element is greater than then maximum positive value
>    expressible by the signed one then it is always greater.
> 
> 3) Only if both values are positive and within the range of the
>    smaller element are the actual values compared. 

With infinite-precision arithmetic, yes; of course min() should just
return the smaller value.

But what would the C type of "min" be for comparing, say, signed and
unsigned longs?  The range of possible results does not fit in any
integral type.  (Repeat the question for signed/unsigned "long long"
if "long long" is your answer.)

 - Pat

Re: [IDEA+RFC] Possible solution for min()/max() war

[Andreas Schwab]

Roman Zippel <zippel@linux-m68k.org> writes:

|> Hi,
|> 
|> Linus Torvalds wrote:
[...]
|> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
|> > address the fact that most of the min/max bugs are due to the programmer
|> > _indending_ a unsigned compare because he didn't even think about the
|> > type.
|> 
|> You maybe fixed a few bugs, but this new macro will only cause new
|> problems in the future. If we change only a single type, you have to
|> scan all min/max users if they possibly need to be changed too. Thanks
|> to the cast, the compiler won't even remotely help you finding them.

There is no cast in the min/max macros.

Andreas.

-- 
Andreas Schwab                                  "And now for something
SuSE Labs                                        completely different."
Andreas.Schwab@suse.de
SuSE GmbH, Schanzäckerstr. 10, D-90443 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

Andreas Schwab wrote:

> There is no cast in the min/max macros.

Ok, it uses an assignment, but it has almost the same effect (except for
pointer/integer values).

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bill Rugolsky Jr.]

On Tue, Aug 28, 2001 at 06:47:23PM +0200, Roman Zippel wrote:
> Ok, it uses an assignment, but it has almost the same effect (except for
> pointer/integer values).
 
Wrong.  A cast throws away information, making meaningful warnings impossible.
The assignment allows the compiler to apply the usual C integral
promotions and catch narrowing and non-value-preserving conversions,
like unsigned int to int, or an even more common bug, unsigned int to
long, which behaves differently depending on whether long is 32 or 64
bits.

Regards,

   Bill Rugolsky

Re: [IDEA+RFC] Possible solution for min()/max() war

[Roman Zippel]

Hi,

"Bill Rugolsky Jr." wrote:

> > Ok, it uses an assignment, but it has almost the same effect (except for
> > pointer/integer values).
> 
> Wrong.  A cast throws away information, making meaningful warnings impossible.
> The assignment allows the compiler to apply the usual C integral
> promotions and catch narrowing and non-value-preserving conversions,
> like unsigned int to int, or an even more common bug, unsigned int to
> long, which behaves differently depending on whether long is 32 or 64
> bits.

Do you have an example? AFAIK this can only be done for constants and
mosts constants used with min are within usual integer limits.

bye, Roman

Re: [IDEA+RFC] Possible solution for min()/max() war

[Richard B. Johnson]

On 28 Aug 2001, Andreas Schwab wrote:

> Roman Zippel <zippel@linux-m68k.org> writes:
> 
> |> Hi,
> |> 
> |> Linus Torvalds wrote:
> [...]
> |> > You just fixed the "re-use arguments" bug - which is a bug, but doesn't
> |> > address the fact that most of the min/max bugs are due to the programmer
> |> > _indending_ a unsigned compare because he didn't even think about the
> |> > type.


#if 0
If the comparison was made unsigned, cast to the largest natural
value on the target, while keeping the types and sizes of the
input variables the same, this macro does about what 99.9999999 percent
of what everybody wants:
#endif

#include <stdio.h>

#define min(a,b) ((size_t)(a) < (size_t)(b) ? (a) : (b))

int main()
{
    printf("%d\n", min(1,2));
    printf("%d\n", min(-1,2));
    printf("%d\n", min(0xffffffff,3));
    printf("%d\n", min(0x8000,4));
    printf("%d\n", min(0x7fff,5));
    printf("%d\n", min(0x80000000,6));
    printf("%d\n", min(0x7fffffff,7));

    printf("%ld\n", min(1L,2L));
    printf("%ld\n", min(-1L,2L));
    printf("%ld\n", min(0xffffffff,3L));
    printf("%ld\n", min(0x8000,4L));
    printf("%ld\n", min(0x7fff,5L));
    printf("%ld\n", min(0x80000000,6L));
    printf("%ld\n", min(0x7fffffff,7L));

    printf("%lu\n", min(1L,2LU));
    printf("%lu\n", min(-1L,2LU));
    printf("%lu\n", min(0xffffffff,3LU));
    printf("%lu\n", min(0x8000,4LU));
    printf("%lu\n", min(0x7fff,5LU));
    printf("%lu\n", min(0x80000000,6LU));
    printf("%lu\n", min(0x7fffffff,7LU));

    printf("%d\n", min(-1, -2));
    printf("%d\n", min(-1, 0));
    printf("%p\n", min((void *)0x80000000, (void *)0x7fffffff));
    return 0;
}



Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

    I was going to compile a list of innovations that could be
    attributed to Microsoft. Once I realized that Ctrl-Alt-Del
    was handled in the BIOS, I found that there aren't any.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Bill Pringlemeir]

>>>>> "Brad" == Brad Chapman <kakadu_croc@yahoo.com> writes:

 Brad> 	OK. The existing API is wrong and the new min()/max() macros
 Brad> are the right way to properly compare values. However, we could
 Brad> always add a config option to enable a compatibility macro,
 Brad> which would use typeof() on one of the two variables and then
 Brad> call the real min()/max(). Something like this (just an
 Brad> example):

       #ifdef CONFIG_ALLOW_COMPAT_MINMAX
       #define proper_min(t, a, b)	((t)(a) < (t)(b) ? (a) : (b))
       #define proper_max(t, a, b)	((t)(a) > (t)(b) ? (a) : (b))
       #define min(a, b)		proper_min(typeof(a), a, b)
       #define max(a, b)		proper_max(typeof(a), a, b)
       #else
       #define min(t, a, b)		((t)(a) < (t)(b) ? (a) : (b))
       #define max(t, a, b)		((t)(a) < (t)(b) ? (a) : (b))
       #endif

Conditionals are bad.  Casting is bad.  In my opinion the only
semantically correct version would be something like this,

#define min(x,y)                                       \
                                                       \
  ({typeof(x) _x = 0; typeof(y) _y = 0;                \
    if ((_x-1>0 && _y-1<0) || (_x-1<0 && _y-1>0)) {    \
         if((x) < 0 ||  (y) < 0)                       \
                exit(SIGNED_UNSIGNED_CONFLICT);        \
    }                                                  \
        _x = (x), _y = (y); (_x>_y)?_y:_x;             \
   }) 

I think that if you are mixing signed and unsigned you should have had
a check before hand to gaurentee that any casting away of the sign is
ok.  It is ok if the variable is in range, it is not if they are out
of range.  However, this incurs a runtime check which is not
appropriate for kernel level code.

As several other people have noted, the casting is bad from a
maintainability prospective.  This is slapping your best friend in the
face; the compiler.  Also, the three arg macro breaks compatibility
with a long standing `C' min/max macro convention.

Probably we should just shutup for now until Linus can clarify what it
is that he wants to achieve and avoid.  Since there is probably no way
to make a universally correct min/max macro in `C', you need to have
some sort of goal to achieve.

regards,
Bill Pringlemeir.

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- Ben LaHaise <bcrl@redhat.com> wrote:
> On Fri, 24 Aug 2001, Brad Chapman wrote:
> 
> > 	This way, some hackers can use the two-arg min()/max() inside an #ifdef block,
> > other hackers can use the three-arg min()/max() inside an #ifdef block,
> > and people who don't care can select either.
> 
> Have you no taste?  Use of #ifdef's should be minimised as much as
> possible.  For this kind of construct, the spurious preprocessor usage
> just makes me want to vomit.
> 
> 		-ben

Mr. LaHaise,

	Eeek! Sorry. That was just a preliminary. As I see it, there are
several ways to wire this up:

	- make everyone use the new macros (some people are thinking :P)

	- make everyone use #ifdef blocks with a config option (you think it's :P)

	- make min()/max() typeof() wrappers for a switch-style stack of comparison 
	  functions which work on the various types, i.e:
	
	  __u8 minimum = min(one, two) -> __u8 minimum = __u8_min(one, two)

	  (this may be too complex and is probably :P)

	- make min()/max() inline functions, cast things to void, and use memcmp()
	  (this sounds reasonable, but is probably also :P)

	- stay with the old-style macros (:P, :P, :P)

	What do you think, sir?

Brad
	


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[David S. Miller]

   From: Brad Chapman <kakadu_croc@yahoo.com>
   Date: Fri, 24 Aug 2001 16:59:27 -0700 (PDT)
   
   	- stay with the old-style macros (:P, :P, :P)

I've been trying to stay out of this until Linus returns
and has his word... but I can say with a level of certainty
that this won't sit well with Linus at all.

Later,
David S. Miller
davem@redhat.com

Re: [IDEA+RFC] Possible solution for min()/max() war

[Brad Chapman]

--- "David S. Miller" <davem@redhat.com> wrote:
>    From: Brad Chapman <kakadu_croc@yahoo.com>
>    Date: Fri, 24 Aug 2001 16:59:27 -0700 (PDT)
>    
>    	- stay with the old-style macros (:P, :P, :P)
> 
> I've been trying to stay out of this until Linus returns
> and has his word... but I can say with a level of certainty
> that this won't sit well with Linus at all.
> 
> Later,
> David S. Miller
> davem@redhat.com

Mr. Miller,

	Ack. When Linus returns, I'm sure he'll give us all very good reasons 
why he thought it needed to be changed.

	BTW, what is your opinion on the various ways I suggested to satisfy 
people over the min()/max() issue? Mr. LaHaise nearly threw up when I suggested 
using #ifdef, so we can safely scratch that one ;-). What do you think, sir?

Brad 


=====
Brad Chapman

Permanent e-mail: kakadu_croc@yahoo.com
Current e-mail: kakadu@adelphia.net
Alternate e-mail: kakadu@netscape.net

__________________________________________________
Do You Yahoo!?
Make international calls for as low as $.04/minute with Yahoo! Messenger
http://phonecard.yahoo.com/

Re: [IDEA+RFC] Possible solution for min()/max() war

[David S. Miller]

   From: Brad Chapman <kakadu_croc@yahoo.com>
   Date: Fri, 24 Aug 2001 17:18:44 -0700 (PDT)
   
   What do you think, sir?

Like I said, I'd like to defer to Linus until he has his word.
I should not have said anything at all... 

Later,
David S. Miller
davem@redhat.com