macro conflict

macro conflict

[J. Imlay]

IN getting the AFS kernel modules to compile under linux I dicovered that
the were useing the standard min(x,y) macro that whould evaluate which one
is smaller. However sometime between 2.4.6 and 2.4.9 a new macro was added
to linux/kernel.h

this one:

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

the old one is

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

has been around a lot longer and is in lots of header files.

The problem here with AFS is that it needs the old definition but the old
definition is being over written by the new one... you guys should know
all this. But I am just saying that I really think the new macro
min(type,x,y) should get a new name. like type_min or something.

Thanks,

Josie Imlay

Re: macro conflict

[Alan Cox]

> The problem here with AFS is that it needs the old definition but the old
> definition is being over written by the new one... you guys should know
> all this. But I am just saying that I really think the new macro
> min(type,x,y) should get a new name. like type_min or something.

Yep. Thats one of the reasons I've not yet moved to 2.4.9 derived trees
for -ac. Its going to require I schedule some time to fix up every single
bogus min/max change in Linus tree

Re: macro conflict

[Tim Walberg]

[-- Attachment #1: Type: text/plain, Size: 2441 bytes --]

There has already been **much** discussion about this, but I think
that the bottom line is that the new version is safer and more
robust than the old version, and thus is not likely to be changed
back.

Consider what happens if someone writes min(++x,y) - the old
version expands to (without some of the extra parens):

	++x < y ? ++x : y

which will increment x twice if the condition ++x < y is true.
There's all kinds of nasty side effects possible with the old
version, including having x > y at the end of the statement, which
definitely violates the semantics of min().

The new version avoids these side effects by only evaluating
the given arguments once (assigning them to temp variables,
which will be optimized away in almost all cases anyway), but
in order to do that, the macro needs to know the variable type,
hence the additional argument. In C++, this can be done using
typename or templates, but the kernel's not written in C++
for a number of very good reasons.

Bottom line, I think the new version of min() and friends is
here to stay and is definitely a positive move. One of the down
sides to that is that a lot of people have a lot of cleaning
up to do.

			tw

On 08/23/2001 12:03 -0700, J. Imlay wrote:
>>	IN getting the AFS kernel modules to compile under linux I dicovered that
>>	the were useing the standard min(x,y) macro that whould evaluate which one
>>	is smaller. However sometime between 2.4.6 and 2.4.9 a new macro was added
>>	to linux/kernel.h
>>	
>>	this one:
>>	
>>	#define min(type,x,y) \
>>	        ({ type __x = (x), __y = (y); __x < __y ? __x: __y; })
>>	
>>	the old one is
>>	
>>	#define min(x,y) ( (x)<(y)?(x):(y) )
>>	
>>	has been around a lot longer and is in lots of header files.
>>	
>>	The problem here with AFS is that it needs the old definition but the old
>>	definition is being over written by the new one... you guys should know
>>	all this. But I am just saying that I really think the new macro
>>	min(type,x,y) should get a new name. like type_min or something.
>>	
>>	Thanks,
>>	
>>	Josie Imlay
>>	
>>	-
>>	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/
End of included message



-- 
twalberg@mindspring.com

[-- Attachment #2: Type: application/pgp-signature, Size: 175 bytes --]

Re: macro conflict

[Alan Cox]

> Consider what happens if someone writes min(++x,y) - the old
> version expands to (without some of the extra parens):
> 
> 	++x < y ? ++x : y

That has nothing to do with it

a) nobody does it and its a known property
b) that was perfectly fixable anyway

Re: macro conflict

[raybry]

Without digging through the archives to see if this has already
been suggested (if so, I apologize), why can't the following be done:

min(x,y) = ({typeof((x)) __x=(x), __y=(y); (__x < __y) ? __x : __y})

That gets you the correct "evaluate the args once" semantics and gives
you control over typing (the comparison is done in the type of the 
first argument) and we don't have to change a zillion drivers.

(typeof() is a gcc extension.)

Tim Walberg wrote:
> 
> There has already been **much** discussion about this, but I think
> that the bottom line is that the new version is safer and more
> robust than the old version, and thus is not likely to be changed
> back.
> 
> Consider what happens if someone writes min(++x,y) - the old
> version expands to (without some of the extra parens):
> 
>         ++x < y ? ++x : y
> 
> which will increment x twice if the condition ++x < y is true.
> There's all kinds of nasty side effects possible with the old
> version, including having x > y at the end of the statement, which
> definitely violates the semantics of min().
> 
> The new version avoids these side effects by only evaluating
> the given arguments once (assigning them to temp variables,
> which will be optimized away in almost all cases anyway), but
> in order to do that, the macro needs to know the variable type,
> hence the additional argument. In C++, this can be done using
> typename or templates, but the kernel's not written in C++
> for a number of very good reasons.
> 
> Bottom line, I think the new version of min() and friends is
> here to stay and is definitely a positive move. One of the down
> sides to that is that a lot of people have a lot of cleaning
> up to do.
> 
>                         tw
> 
> On 08/23/2001 12:03 -0700, J. Imlay wrote:
> >>      IN getting the AFS kernel modules to compile under linux I
> dicovered that
> >>      the were useing the standard min(x,y) macro that whould evaluate
> which one
> >>      is smaller. However sometime between 2.4.6 and 2.4.9 a new macro
> was added
> >>      to linux/kernel.h
> >>
> >>      this one:
> >>
> >>      #define min(type,x,y) \
> >>              ({ type __x = (x), __y = (y); __x < __y ? __x: __y; })
> >>
> >>      the old one is
> >>
> >>      #define min(x,y) ( (x)<(y)?(x):(y) )
> >>
> >>      has been around a lot longer and is in lots of header files.
> >>
> >>      The problem here with AFS is that it needs the old definition
> but the old
> >>      definition is being over written by the new one... you guys
> should know
> >>      all this. But I am just saying that I really think the new macro
> >>      min(type,x,y) should get a new name. like type_min or something.
> >>
> >>      Thanks,
> >>
> >>      Josie Imlay
> >>
> >>      -
> >>      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/
> End of included message
> 
> --
> twalberg@mindspring.com

-- 
----------------------------------------------------------- 
  Ray Bryant,  Linux Performance Analyst, Times N Systems
   1908 Kramer Lane, Bldg. B, Suite P, Austin, TX 78758
              512-977-5366, raybry@timesn.com
-----------------------------------------------------------

Re: macro conflict

[Magnus Naeslund(f)]

From: <raybry@timesn.com>
> Without digging through the archives to see if this has already
> been suggested (if so, I apologize), why can't the following be done:
>
> min(x,y) = ({typeof((x)) __x=(x), __y=(y); (__x < __y) ? __x : __y})
>
> That gets you the correct "evaluate the args once" semantics and gives
> you control over typing (the comparison is done in the type of the
> first argument) and we don't have to change a zillion drivers.
>
> (typeof() is a gcc extension.)
>

But then again, how do you know it's the type of x we want, maybe we want
type of y, that is and signed char (not an int like x).
Talk about hidden buffer overflow stuff :)

Magnus

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 Programmer/Networker [|] Magnus Naeslund
 PGP Key: http://www.genline.nu/mag_pgp.txt
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Re: macro conflict

[Alan Cox]

> But then again, how do you know it's the type of x we want, maybe we want
> type of y, that is and signed char (not an int like x).
> Talk about hidden buffer overflow stuff :)

This is one of the big problems with min and max, you get deeply suprising
and unpleasant results if you don't consider sign propogation, sizes and
overruns carefully. In C there are some great evils in that area, and its
one reason a lot of people prefer to write such code explicitly.

Alan

Re: macro conflict

[Magnus Naeslund(f)]

From: "Alan Cox" <alan@lxorguk.ukuu.org.uk>
> This is one of the big problems with min and max, you get deeply suprising
> and unpleasant results if you don't consider sign propogation, sizes and
> overruns carefully. In C there are some great evils in that area, and its
> one reason a lot of people prefer to write such code explicitly.
>


Yeah, been there, bitten by that. :)

Magnus

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 Programmer/Networker [|] Magnus Naeslund
 PGP Key: http://www.genline.nu/mag_pgp.txt
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Re: macro conflict

[Andrew Cannon]

"Magnus Naeslund(f)" wrote:
> 
> From: <raybry@timesn.com>
> > Without digging through the archives to see if this has already
> > been suggested (if so, I apologize), why can't the following be done:
> >
> > min(x,y) = ({typeof((x)) __x=(x), __y=(y); (__x < __y) ? __x : __y})
> >
> > That gets you the correct "evaluate the args once" semantics and gives
> > you control over typing (the comparison is done in the type of the
> > first argument) and we don't have to change a zillion drivers.
> >
> > (typeof() is a gcc extension.)
> >
> 
> But then again, how do you know it's the type of x we want, maybe we want
> type of y, that is and signed char (not an int like x).
> Talk about hidden buffer overflow stuff :)


What about this then:

#define min(x,y) ({typeof(x) __x=(x); typeof(y) __y=(y); (__x < __y) ?
__x : __y})

This is guaranteed to work the same as the old min/max in all cases but
without side effects. You can still force the comparison to be done with
a certain type by casting the arguments first:

#define typed_min(type, x, y) (min((type)(x), (type)(y)))

...although, if the type used for the comparison is so critical you
maybe shouldn't be hiding it in a macro anyway.

Andrew

Re: macro conflict

[Magnus Naeslund(f)]

From: "Andrew Cannon" <ajc@gmx.net>
>
> What about this then:
>
> #define min(x,y) ({typeof(x) __x=(x); typeof(y) __y=(y); (__x < __y) ?
> __x : __y})
>
> This is guaranteed to work the same as the old min/max in all cases but
> without side effects. You can still force the comparison to be done with
> a certain type by casting the arguments first:
>
[snip]

Well it's closer but not really what i want.
The min/max_type is maybe the way to go, but the above can still bit you if
the types differ. Consider max().

char lut[256];
int   c1 = 256+rand()%256;
char  c2 = rand()%256;
char dest = lut[max(c2,c1)];

Won't c1 still be returned untruncated?

Ofcourse one will use another construct for these kinds of checks, but maybe
your brain collapses just for a second and think that this will return a
char.

OK ok bad example, but maybe you see that i have a point here somewhere
(under my chair? :) ).

Magnus

-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
 Programmer/Networker [|] Magnus Naeslund
 PGP Key: http://www.genline.nu/mag_pgp.txt
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

Re: macro conflict

[Roman Zippel]

Hi,

"Magnus Naeslund(f)" wrote:

> > min(x,y) = ({typeof((x)) __x=(x), __y=(y); (__x < __y) ? __x : __y})
> >
> > That gets you the correct "evaluate the args once" semantics and gives
> > you control over typing (the comparison is done in the type of the
> > first argument) and we don't have to change a zillion drivers.
> >
> > (typeof() is a gcc extension.)

({...}) is also gcc extension.

> But then again, how do you know it's the type of x we want, maybe we want
> type of y, that is and signed char (not an int like x).
> Talk about hidden buffer overflow stuff :)

That's the reason I'm using this macro for affs:

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

I need a very good reason to use something else, so far I'm unconvinced.

bye, Roman

RE: macro conflict

[Camiel Vanderhoeven]

A small detail; you min(x,y) fails if x and y are pointers.

Example: min(char * a, char * b) would evaluate to
char * __x = (a), __y = (b); etc...

Here __x is a pointer and __y is a char. A better solution (at least the
one that has my vote so far) would besomething like this:

typeof(x) __x=(x); typeof(y) __y=(y); (__x < __y) ? __x : __y

Camiel.

> Without digging through the archives to see if this has already
> been suggested (if so, I apologize), why can't the following be done:
> 
> min(x,y) = ({typeof((x)) __x=(x), __y=(y); (__x < __y) ? __x : __y})
> 
> That gets you the correct "evaluate the args once" semantics and gives
> you control over typing (the comparison is done in the type of the
> first argument) and we don't have to change a zillion drivers.
> 
> (typeof() is a gcc extension.)

Re: macro conflict

[David Woodhouse]

twalberg@mindspring.com said:
> There has already been **much** discussion about this, but I think
> that the bottom line is that the new version is safer and more robust
> than the old version, and thus is not likely to be changed back. 

That's a completely separate issue. You can fix it while keeping sane 
semantics for min() and max().

#define real_min(x,y) ({ typeof((x)) _x = (x); typeof((y)) _y = (y); (_x>_y)?_y:_x; })

#define min(x,y) ({ if strcmp(STRINGIFY(typeof(x)), STRINGIFY(typeof(y))) BUG(); realmin(x,y) }) 

/me wonders if gcc would manage to optimise that.
--
dwmw2

Re: macro conflict

[Keith Owens]

On Fri, 24 Aug 2001 14:03:34 +0100, 
David Woodhouse <dwmw2@infradead.org> wrote:
>#define min(x,y) ({ if strcmp(STRINGIFY(typeof(x)), STRINGIFY(typeof(y))) BUG(); realmin(x,y) }) 

Did you try that?  Firstly typeof() is only defined in declaration
context, it gets an error when used in an expression.  Secondly
typeof() is not expanded by cpp so the stringify tricks do not work.
typeof(x) is handled by cc, not cpp.

Re: macro conflict

[David Woodhouse]

kaos@ocs.com.au said:
>  Did you try that?  Firstly typeof() is only defined in declaration
> context, it gets an error when used in an expression.  Secondly
> typeof() is not expanded by cpp so the stringify tricks do not work.
> typeof(x) is handled by cc, not cpp. 

No. It's far too silly for me to have actually tried it :)

--
dwmw2

Re: macro conflict

[Bill Pringlemeir]

>>>> kaos@ocs.com.au said:

 >> Did you try that?  Firstly typeof() is only defined in declaration
 >> context, it gets an error when used in an expression.  Secondly
 >> typeof() is not expanded by cpp so the stringify tricks do not
 >> work.  typeof(x) is handled by cc, not cpp.

>>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:

 David> No. It's far too silly for me to have actually tried it :)

David, you wild and crazy guy!  How crazy is this,

#define real_min(x,y) ({ typeof((x)) _x = (x); typeof((y)) _y = (y); (_x>_y)?_y:_x; })
#define min(x,y) ({extern void BUG(void); typeof(x) _x = 0; typeof(y) _y = 0;  \
        if (sizeof(typeof(x)) != sizeof(typeof(y)) ||   \
            (_x-1 > 0 && _y-1 < 0) || (_x-1<0 && _y-1>0)) \
                            BUG();                      \
        real_min(x,y); }) 

int main(int argc, char *argv[])
{
    int ix,iy;
    unsigned int ux,uy;
    long lx,ly;
    unsigned long Ux,Uy;

    ux = min(ix,uy);
    return 0;
}

sh> gcc -Wall test.c -O2 test

This is a little slimmy as the sizeof will evaluate the to same thing
on some architectures for long/int or short/int.  Even worse, it won't
handle pointers.  The macro is `physically correct' if a short/int are
the same.  Actually, you would probably want to cast up to which ever
is the bigger of the types to fully handle the precision.  Maybe the
sizeof() test isn't even needed due to promotion.  Just the signs are
important (afaik) and a test for pointer and integral mixing which I
cann't think of.  Maybe some clever use of arrays or "+ *x" or
something.

fwiw,
Bill Pringlemeir.

Re: macro conflict

[Roman Zippel]

Hi,

> sizeof() test isn't even needed due to promotion.  Just the signs are
> important (afaik) and a test for pointer and integral mixing which I
> cann't think of.  Maybe some clever use of arrays or "+ *x" or
> something.

Try '-W' or '-Wsign-compare'. pointer/integer compare already results in
a warning without any option.

bye, Roman

Re: macro conflict

[Richard B. Johnson]

On Fri, 24 Aug 2001, David Woodhouse wrote:

> 
> twalberg@mindspring.com said:
> > There has already been **much** discussion about this, but I think
> > that the bottom line is that the new version is safer and more robust
> > than the old version, and thus is not likely to be changed back. 
> 
> That's a completely separate issue. You can fix it while keeping sane 
> semantics for min() and max().
> 
> #define real_min(x,y) ({ typeof((x)) _x = (x); typeof((y)) _y = (y); (_x>_y)?_y:_x; })
> 
> #define min(x,y) ({ if strcmp(STRINGIFY(typeof(x)), STRINGIFY(typeof(y))) BUG(); realmin(x,y) }) 
> 
> /me wonders if gcc would manage to optimise that.
> --
> dwmw2
> 

Looking through the code, min() is most always used to find some value
that will not overflow some buffer, i.e.,

    len = min(user_request_len, sizeof(buffer));

The problem is that sizeof() returns an unsigned int (size_t), and the
user request length may be an integer. Everything works fine until
you get to lengths with the high bit set. Then, you are in trouble.

In this case, you could have a 'min()' that does:

#define min(a,b) (unsigned long)(a) < (unsigned long)(b) ? (a) : (b)

... where the comparison (only) is made unsigned, and you keep the
original values. This should work, perhaps in all the current uses.
However, min() is not the mathematical min() anymore. That's fine,
but it probably should not be called min() unless it is truly a min()
function.

So, I suggest that instead of using some macro in a header, if you
need min() you make one in your code called MIN(). Problem solved.
Your macro will do exactly what you want it to do. If It's broken,
you get to fix it (or keep the pieces).


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: macro conflict

[David Wagner]

Richard B. Johnson wrote:
>Looking through the code, min() is most always used to find some value
>that will not overflow some buffer, i.e.,
>
>    len = min(user_request_len, sizeof(buffer));
>
>The problem is that sizeof() returns an unsigned int (size_t), and the
>user request length may be an integer. Everything works fine until
>you get to lengths with the high bit set. Then, you are in trouble.
>
>In this case, you could have a 'min()' that does:
>
>#define min(a,b) (unsigned long)(a) < (unsigned long)(b) ? (a) : (b)
>
>... where the comparison (only) is made unsigned, and you keep the
>original values. This should work, perhaps in all the current uses.

Just a small warning:  If anyone writes something like
    int len = min(user_request_len, sizeof(buffer));
    if (user_request_len > len)
        goto fail;
    memcpy(dst, user_src, len);
they can get into trouble even with your min() macro.
Ok, maybe this is crazy code that noone in their right
mind would ever write.

This is not intended as a criticism -- your approach may be
sufficient for existing code -- but it is something to watch
out for.

Re: macro conflict

[Alex Bligh - linux-kernel]

># define real_min(x,y) ({ typeof((x)) _x = (x); typeof((y)) _y = (y);
># (_x>_y)?_y:_x; })
>
># define min(x,y) ({ if strcmp(STRINGIFY(typeof(x)), STRINGIFY(typeof(y)))
># BUG(); realmin(x,y) })
>
> /me wonders if gcc would manage to optimise that.

Will this work with things like

void test(unsigned int foo, char bar)
{
	printf ("%d %d\n", min(foo, 10), min (bar, 20));
}

Surely one of those must BUG().

--
Alex Bligh

Re: macro conflict

[David Woodhouse]

linux-kernel@alex.org.uk said:
>  Will this work with things like
> void test(unsigned int foo, char bar) {
> 	printf ("%d %d\n", min(foo, 10), min (bar, 20)); }
> Surely one of those must BUG().

Well, ideally both of them would BUG() and the user would have to explicitly
cast one (or both) of the arguments so the types match. But as Keith 
pointed out, it won't work.

--
dwmw2

Re: macro conflict

[Bill Pringlemeir]

>>>>> "David" == David Woodhouse <dwmw2@infradead.org> writes:

> printf ("%d %d\n", min(foo, 10), min (bar, 20)); }

 David> Well, ideally both of them would BUG() and the user would have
 David> to explicitly cast one (or both) of the arguments so the types
 David> match. But as Keith pointed out, it won't work.

Why would both BUG? 20 is a signed integer and bar _could be_ a signed
char.  This is fine.  As a matter of fact, both constants are positive
and fit in the range so I don't really think this is a bug in either
case.  I guess the constants should be written as 10U and 20U.  There
are other problems as the code without a specifically type char will
have bugs on the ARM (and others with different sign default).  I
don't think that the casting handles this well either.

I did a little more beautification.  Gcc does warn if you compare a
pointer to an integral type.  Do you need more?  The bug_paste macro
would pollute the name-space, but it is nice to see where things go
wrong.

fwiw,
Bill Pringlemeir.

[test.c]
#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! Promotion to int before compare. */
/*  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);
    
/* BUG? printf ("%d\n", min(Iy, 10)); */
    printf ("%d\n", min(cx, 20));

    return 0;
}