2.4.9 does not compile

2.4.9 does not compile

[f5ibh]

Hi !

I've the following error messages :

gcc -D__KERNEL__ -I/usr/src/kernel-sources-2.4.9/include -Wall
-Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer
-fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2 -march=k6
-DMODULE -DMODVERSIONS -include
/usr/src/kernel-sources-2.4.9/include/linux/modversions.h
-DNTFS_VERSION=\"1.1.16\"   -c -o unistr.o unistr.c
unistr.c: In function `ntfs_collate_names':
unistr.c:99: warning: implicit declaration of function `min'
unistr.c:99: parse error before `unsigned'
unistr.c:99: parse error before `)'
unistr.c:97: warning: `c1' might be used uninitialized in this function
unistr.c: At top level:
unistr.c:118: parse error before `if'
unistr.c:123: warning: type defaults to `int' in declaration of `c1'
unistr.c:123: `name1' undeclared here (not in a function)
unistr.c:123: warning: data definition has no type or storage class
unistr.c:124: parse error before `if'
make[3]: *** [unistr.o] Erreur 1
make[3]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs/ntfs'
make[2]: *** [_modsubdir_ntfs] Erreur 2
make[2]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs'
make[1]: *** [_mod_fs] Erreur 2
make[1]: Leaving directory `/usr/src/kernel-sources-2.4.9'
make: *** [stamp-build] Erreur 2


gcc is :
Reading specs from /usr/lib/gcc-lib/i386-linux/2.95.4/specs
gcc version 2.95.4 20010319 (Debian prerelease)

------
Regards
		Jean-Luc

Re: 2.4.9 does not compile [PATCH]

[tpepper]

Confirmed here.  Looks like a pretty obvious goof to me.  Does the following
fix it for you?

--- linux-2.4.9/fs/ntfs/unistr.c.orig   Thu Aug 16 14:35:03 2001
+++ linux-2.4.9/fs/ntfs/unistr.c        Thu Aug 16 14:35:15 2001
@@ -96,7 +96,7 @@
        __u32 cnt;
        wchar_t c1, c2;
 
-       for (cnt = 0; cnt < min(unsigned int, name1_len, name2_len); ++cnt)
+       for (cnt = 0; cnt < min(name1_len, name2_len); ++cnt)
        {
                c1 = le16_to_cpu(*name1++);
                c2 = le16_to_cpu(*name2++);

> unistr.c: In function `ntfs_collate_names':
> unistr.c:99: warning: implicit declaration of function `min'
> unistr.c:99: parse error before `unsigned'
> unistr.c:99: parse error before `)'
> unistr.c:97: warning: `c1' might be used uninitialized in this function
> unistr.c: At top level:
> unistr.c:118: parse error before `if'
> unistr.c:123: warning: type defaults to `int' in declaration of `c1'
> unistr.c:123: `name1' undeclared here (not in a function)
> unistr.c:123: warning: data definition has no type or storage class
> unistr.c:124: parse error before `if'
> make[3]: *** [unistr.o] Erreur 1
> make[3]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs/ntfs'
> make[2]: *** [_modsubdir_ntfs] Erreur 2
> make[2]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs'
> make[1]: *** [_mod_fs] Erreur 2
> make[1]: Leaving directory `/usr/src/kernel-sources-2.4.9'
> make: *** [stamp-build] Erreur 2

Re: 2.4.9 does not compile [better PATCH]

[tpepper]

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

It looks like they are actually using a 3 argument min() function in the ntfs
code.  It seems happy with these three arg's and an include added so this patch is pr÷bably preferable:

diff -Naur linux-2.4.9/fs/ntfs/unistr.c.orig linux-2.4.9/fs/ntfs/unistr.c
--- linux-2.4.9/fs/ntfs/unistr.c.orig   Thu Aug 16 14:35:03 2001
+++ linux-2.4.9/fs/ntfs/unistr.c        Thu Aug 16 15:07:54 2001
@@ -24,6 +24,7 @@
 #include <linux/string.h>
 #include <asm/byteorder.h>
 
+#include "ntfstypes.h"
 #include "unistr.h"
 #include "macros.h"
 
@@ -96,7 +97,7 @@
        __u32 cnt;
        wchar_t c1, c2;
 
-       for (cnt = 0; cnt < min(unsigned int, name1_len, name2_len); ++cnt)
+       for (cnt = 0; cnt < min(__u32, name1_len, name2_len); ++cnt)
        {
                c1 = le16_to_cpu(*name1++);
                c2 = le16_to_cpu(*name2++);

Tim

--
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************


On Thu 16 Aug at 23:11:40 +0200 f5ibh@db0bm.ampr.org done said:

> unistr.c: In function `ntfs_collate_names':
> unistr.c:99: warning: implicit declaration of function `min'
> unistr.c:99: parse error before `unsigned'
> unistr.c:99: parse error before `)'
> unistr.c:97: warning: `c1' might be used uninitialized in this function
> unistr.c: At top level:
> unistr.c:118: parse error before `if'
> unistr.c:123: warning: type defaults to `int' in declaration of `c1'
> unistr.c:123: `name1' undeclared here (not in a function)
> unistr.c:123: warning: data definition has no type or storage class
> unistr.c:124: parse error before `if'
> make[3]: *** [unistr.o] Erreur 1
> make[3]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs/ntfs'
> make[2]: *** [_modsubdir_ntfs] Erreur 2
> make[2]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs'
> make[1]: *** [_mod_fs] Erreur 2
> make[1]: Leaving directory `/usr/src/kernel-sources-2.4.9'
> make: *** [stamp-build] Erreur 2

Re: 2.4.9 does not compile [PATCH]

[Anton Altaparmakov]

Wrong fix. Jules Colding already sent correct one (an hour ago or so).

At 22:41 16/08/2001, tpepper@vato.org wrote:
>Confirmed here.  Looks like a pretty obvious goof to me.

Yes. Someone forgot to add kernel.h include after they removed the ntfs 
specific macros... Grr...

>   Does the following fix it for you?
>
>--- linux-2.4.9/fs/ntfs/unistr.c.orig   Thu Aug 16 14:35:03 2001
>+++ linux-2.4.9/fs/ntfs/unistr.c        Thu Aug 16 14:35:15 2001
>@@ -96,7 +96,7 @@
>         __u32 cnt;
>         wchar_t c1, c2;
>
>-       for (cnt = 0; cnt < min(unsigned int, name1_len, name2_len); ++cnt)
>+       for (cnt = 0; cnt < min(name1_len, name2_len); ++cnt)

This won't fix it because the min macro still will not exist.

Anton

>         {
>                 c1 = le16_to_cpu(*name1++);
>                 c2 = le16_to_cpu(*name2++);
>
> > unistr.c: In function `ntfs_collate_names':
> > unistr.c:99: warning: implicit declaration of function `min'
> > unistr.c:99: parse error before `unsigned'
> > unistr.c:99: parse error before `)'
> > unistr.c:97: warning: `c1' might be used uninitialized in this function
> > unistr.c: At top level:
> > unistr.c:118: parse error before `if'
> > unistr.c:123: warning: type defaults to `int' in declaration of `c1'
> > unistr.c:123: `name1' undeclared here (not in a function)
> > unistr.c:123: warning: data definition has no type or storage class
> > unistr.c:124: parse error before `if'
> > make[3]: *** [unistr.o] Erreur 1
> > make[3]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs/ntfs'
> > make[2]: *** [_modsubdir_ntfs] Erreur 2
> > make[2]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs'
> > make[1]: *** [_mod_fs] Erreur 2
> > make[1]: Leaving directory `/usr/src/kernel-sources-2.4.9'
> > make: *** [stamp-build] Erreur 2
>-
>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/

-- 
   "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: 2.4.9 does not compile [better PATCH]

[Anton Altaparmakov]

At 23:10 16/08/2001, tpepper@vato.org wrote:
>It looks like they are actually using a 3 argument min() function in the ntfs
>code.

No, we are not using a 3 argument min() macro in ntfs code. At least we 
weren't until someone decided to replace all existing max/min macros from 
the entire kernel with this new 3 argument version and in the process 
forgot to add the kernel.h include thus breaking ntfs compilation...

Oh well, we will have to live with it until Linus comes back.

>It seems happy with these three arg's and an include added so this patch 
>is pr÷bably preferable:
>
>diff -Naur linux-2.4.9/fs/ntfs/unistr.c.orig linux-2.4.9/fs/ntfs/unistr.c
>--- linux-2.4.9/fs/ntfs/unistr.c.orig   Thu Aug 16 14:35:03 2001
>+++ linux-2.4.9/fs/ntfs/unistr.c        Thu Aug 16 15:07:54 2001
>@@ -24,6 +24,7 @@
>  #include <linux/string.h>
>  #include <asm/byteorder.h>
>
>+#include "ntfstypes.h"

that should be +#include <linux/kernel.h>

Best regards,

Anton

>  #include "unistr.h"
>  #include "macros.h"
>
>@@ -96,7 +97,7 @@
>         __u32 cnt;
>         wchar_t c1, c2;
>
>-       for (cnt = 0; cnt < min(unsigned int, name1_len, name2_len); ++cnt)
>+       for (cnt = 0; cnt < min(__u32, name1_len, name2_len); ++cnt)
>         {
>                 c1 = le16_to_cpu(*name1++);
>                 c2 = le16_to_cpu(*name2++);
>
>Tim
>
>--
>*********************************************************
>*  tpepper@vato dot org             * Venimus, Vidimus, *
>*  http://www.vato.org/~tpepper     * Dolavimus         *
>*********************************************************
>
>
>On Thu 16 Aug at 23:11:40 +0200 f5ibh@db0bm.ampr.org done said:
>
> > unistr.c: In function `ntfs_collate_names':
> > unistr.c:99: warning: implicit declaration of function `min'
> > unistr.c:99: parse error before `unsigned'
> > unistr.c:99: parse error before `)'
> > unistr.c:97: warning: `c1' might be used uninitialized in this function
> > unistr.c: At top level:
> > unistr.c:118: parse error before `if'
> > unistr.c:123: warning: type defaults to `int' in declaration of `c1'
> > unistr.c:123: `name1' undeclared here (not in a function)
> > unistr.c:123: warning: data definition has no type or storage class
> > unistr.c:124: parse error before `if'
> > make[3]: *** [unistr.o] Erreur 1
> > make[3]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs/ntfs'
> > make[2]: *** [_modsubdir_ntfs] Erreur 2
> > make[2]: Leaving directory `/usr/src/kernel-sources-2.4.9/fs'
> > make[1]: *** [_mod_fs] Erreur 2
> > make[1]: Leaving directory `/usr/src/kernel-sources-2.4.9'
> > make: *** [stamp-build] Erreur 2
>-
>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/

-- 
   "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: 2.4.9 does not compile [better PATCH]

[Jeff Chua]

lvm does not compile either.

Here's the complete patch for 2.4.9

Jeff

--- linux/drivers/md/lvm.c.org	Fri Aug 17 12:07:35 2001
+++ linux/drivers/md/lvm.c	Fri Aug 17 12:09:05 2001
@@ -2326,7 +2326,7 @@

 	/* save availiable i/o statistic data */
 	if (old_lv->lv_stripes < 2) {	/* linear logical volume */
-		end = min(old_lv->lv_current_le, new_lv->lv_current_le);
+		end = min(__u32, old_lv->lv_current_le, new_lv->lv_current_le);
 		for (l = 0; l < end; l++) {
 			new_lv->lv_current_pe[l].reads +=
 				old_lv->lv_current_pe[l].reads;
@@ -2340,7 +2340,7 @@

 		old_stripe_size = old_lv->lv_allocated_le / old_lv->lv_stripes;
 		new_stripe_size = new_lv->lv_allocated_le / new_lv->lv_stripes;
-		end = min(old_stripe_size, new_stripe_size);
+		end = min(__u32, old_stripe_size, new_stripe_size);

 		for (i = source = dest = 0; i < new_lv->lv_stripes; i++) {
 			for (j = 0; j < end; j++) {
--- linux/drivers/md/lvm-snap.c.org	Fri Aug 17 12:09:35 2001
+++ linux/drivers/md/lvm-snap.c	Fri Aug 17 12:10:30 2001
@@ -360,8 +360,8 @@

 	blksize_org = lvm_get_blksize(org_phys_dev);
 	blksize_snap = lvm_get_blksize(snap_phys_dev);
-	max_blksize = max(blksize_org, blksize_snap);
-	min_blksize = min(blksize_org, blksize_snap);
+	max_blksize = max(__u32, blksize_org, blksize_snap);
+	min_blksize = min(__u32, blksize_org, blksize_snap);
 	max_sectors = KIO_MAX_SECTORS * (min_blksize>>9);

 	if (chunk_size % (max_blksize>>9))
@@ -369,7 +369,7 @@

 	while (chunk_size)
 	{
-		nr_sectors = min(chunk_size, max_sectors);
+		nr_sectors = min(__u32, chunk_size, max_sectors);
 		chunk_size -= nr_sectors;

 		iobuf->length = nr_sectors << 9;
@@ -486,7 +486,7 @@

 	buckets = lv->lv_remap_end;
 	max_buckets = calc_max_buckets();
-	buckets = min(buckets, max_buckets);
+	buckets = min(__u32, buckets, max_buckets);
 	while (buckets & (buckets-1))
 		buckets &= (buckets-1);

--- linux/fs/ntfs/unistr.c.org	Fri Aug 17 12:18:13 2001
+++ linux/fs/ntfs/unistr.c	Fri Aug 17 12:18:25 2001
@@ -23,6 +23,7 @@

 #include <linux/string.h>
 #include <asm/byteorder.h>
+#include <linux/kernel.h>

 #include "unistr.h"
 #include "macros.h"
@@ -96,7 +97,7 @@
 	__u32 cnt;
 	wchar_t c1, c2;

-	for (cnt = 0; cnt < min(unsigned int, name1_len, name2_len); ++cnt)
+	for (cnt = 0; cnt < min(__u32, name1_len, name2_len); ++cnt)
 	{
 		c1 = le16_to_cpu(*name1++);
 		c2 = le16_to_cpu(*name2++);

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: tpepper@vato.org
   Date: Thu, 16 Aug 2001 14:41:09 -0700

   Confirmed here.  Looks like a pretty obvious goof to me.  Does the following
   fix it for you?

The args and semantics of min/max changed to take
a type first argument, the problem with this ntfs file is that it
fails to include linux/kernel.h

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Daniel Phillips <phillips@bonn-fries.net>
   Date: Fri, 17 Aug 2001 01:13:38 +0200

   What is wrong with using typeof?

Users don't think about the type that way, the new macros forces them
to at least consider it for a moment.

   If you must have a three argument min, 
   could it please be called "type_min" of similar.

We wanted people trying to use "min" and "max" with 2
args, or redefining their own, to get a compile error.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Daniel Phillips]

On August 17, 2001 12:31 am, David S. Miller wrote:
>    From: tpepper@vato.org
>    Date: Thu, 16 Aug 2001 14:41:09 -0700
> 
>    Confirmed here.  Looks like a pretty obvious goof to me.  Does the 
following
>    fix it for you?
> 
> The args and semantics of min/max changed to take
> a type first argument,

They did?  This three argument min is butt-ugly, not to mention a completely 
original way of expressing the idea that is very much in conflict with every 
other expression of min I have ever seen.

What is wrong with using typeof?  If you must have a three argument min, 
could it please be called "type_min" of similar.

> the problem with this ntfs file is that it fails to include linux/kernel.h

--
Daniel

Re: 2.4.9 does not compile [PATCH]

[Alan Cox]

> > The args and semantics of min/max changed to take
> > a type first argument,
> 
> They did?  This three argument min is butt-ugly, not to mention a completely 
> original way of expressing the idea that is very much in conflict with every 
> other expression of min I have ever seen.
> 
> What is wrong with using typeof?  If you must have a three argument min, 
> could it please be called "type_min" of similar.

It also doesnt solve all the cases. Basically its just ensuring everyone
doing portable code has to go and change all their macros to MIN and MAX
instead. Or use if statements, which with all the subtle suprises of
type casting is actually often a far far better idea and probably should
be encouraged a lot more in the kernel

Alan

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Anton Altaparmakov <aia21@cam.ac.uk>
   Date: Fri, 17 Aug 2001 00:35:02 +0100

   Otherwise I would submit a patch to switch NTFS. I don't like this 3 arg 
   construct...

Linus will reject every such patch, and I have grepping scripts that
will detect crap like this entering the tree in case he misses
something.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Rik van Riel <riel@conectiva.com.br>
   Date: Fri, 17 Aug 2001 18:44:16 -0300 (BRST)

   On Thu, 16 Aug 2001, David S. Miller wrote:
   
   > Linus will reject every such patch, and I have grepping scripts
   > that will detect crap like this entering the tree in case he
   > misses something.
   
   Well, you're not the NTFS maintainer.

Yes, but Linus is the kernel maintainer.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Anton Altaparmakov]

At 00:14 17/08/2001, Alan Cox wrote:
> > > The args and semantics of min/max changed to take
> > > a type first argument,
> >
> > They did?  This three argument min is butt-ugly, not to mention a 
> completely
> > original way of expressing the idea that is very much in conflict with 
> every
> > other expression of min I have ever seen.
> >
> > What is wrong with using typeof?  If you must have a three argument min,
> > could it please be called "type_min" of similar.
>
>It also doesnt solve all the cases.

Really? Could you point out an example where using ... typeof(x) __x; 
typeof(y) __y; ... in the macros wouldn't work? - I just tried a few 
examples I thought wouldn't work (side-effects ones) but I was pleasantly 
surprised to that gcc always produced the exact same assembler output for 
both the 3 arg and the 2 arg + typeof macros.

>Basically its just ensuring everyone doing portable code has to go and 
>change all their macros to MIN and MAX instead.

Considering the patch removed all occurences of MIN/MAX with the new 3 arg 
min/max macros it wouldn't seem that this would be accepted... )-: 
Otherwise I would submit a patch to switch NTFS. I don't like this 3 arg 
construct...

Best regards,

         Anton

>Or use if statements, which with all the subtle suprises of type casting 
>is actually often a far far better idea and probably should be encouraged 
>a lot more in the kernel



-- 
   "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: 2.4.9 does not compile [PATCH]

[Jeff Dike]

aia21@cam.ac.uk said:
> Really? Could you point out an example where using ... typeof(x) __x;
> typeof(y) __y; ... in the macros wouldn't work? - I just tried a few
> examples I thought wouldn't work (side-effects ones) but I was
> pleasantly  surprised to that gcc always produced the exact same
> assembler output for  both the 3 arg and the 2 arg + typeof macros. 

Try min(a, min(b, c)).  Look at the cpp expansion and notice all the variable
name clashes.

We went through this on #kernel one night, and Alan concocted some amazingly
gross unique identifier generators as a result.  To me, this looks like the 
best way to do this.

				Jeff

Re: 2.4.9 does not compile [PATCH]

[Anton Altaparmakov]

Hi,

Thanks for the reply, but...

At 02:46 17/08/2001, Jeff Dike wrote:
>aia21@cam.ac.uk said:
> > Really? Could you point out an example where using ... typeof(x) __x;
> > typeof(y) __y; ... in the macros wouldn't work? - I just tried a few
> > examples I thought wouldn't work (side-effects ones) but I was
> > pleasantly  surprised to that gcc always produced the exact same
> > assembler output for  both the 3 arg and the 2 arg + typeof macros.
>
>Try min(a, min(b, c)).  Look at the cpp expansion and notice all the variable
>name clashes.

There are no clashes; unless my knowledge of C has abandoned me at the 
moment, you can reuse variable names within local statement blocks and the 
compiler is intelligent enough to sort it out.

I tried egcs-1.1.2/gcc-2.91.66, gcc 2.95.3 (SuSE 7.1) and gcc 2.96 (RHL 7.1 
2.96-85) and the generated assembler is fully correct with all these 
compilers for follwing little test (I compiled it with the same gcc command 
line as the kernel uses for the ntfs source files):

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

int test(int a, int b, int c)
{
         return min(a, min(b, c));
}
--- cut ---

No warnings are emitted and the assembler is correct if I am not mistaken.

Is my test invalid? I guess I must be missing something, but I can't figure 
out what. )-: Perhaps sleeping over it might help...

>We went through this on #kernel one night, and Alan concocted some amazingly
>gross unique identifier generators as a result.  To me, this looks like the
>best way to do this.

Shame I missed that... </me really ignorant> which IRC network is this 
channel on? Doesn't seem to be on openprojects.net...

Best regards,

Anton


-- 
   "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: 2.4.9 does not compile [PATCH]

[Andrew Morton]

Anton Altaparmakov wrote:
> 
> #define min(x,y) \
>          ({ typeof(x) __x = (x); typeof(y) __y = (y); __x < __y ? __x : __y; })
> 
> int test(int a, int b, int c)
> {
>          return min(a, min(b, c));
> }

Problems occur if the caller happens to pass in variables whose
names confilct with the ones you chose in the above macro:

laptop:/home/akpm> cat t.c
#define min(x,y) \
         ({ typeof(x) __x = (x); typeof(y) __y = (y); __x < __y ? __x : __y; })

int test(int __x, int __y)
{
        return min(__x, __y);		/* sic */
}

main()
{
        printf("%d\n", test(1, 2));
        printf("%d\n", test(2, 1));
}
laptop:/home/akpm> gcc t.c
laptop:/home/akpm> ./a.out
-1073744344
1074305684


So if you replace "__x" with "__dont_use_this_identifier_its_reserved_for_min"
I don't expect anyone could sensibly complain...

-

Re: 2.4.9 does not compile [PATCH]

[Daniel Phillips]

On August 17, 2001 03:49 am, Andrew Morton wrote:
> Anton Altaparmakov wrote:
> > 
> > #define min(x,y) \
> >          ({ typeof(x) __x = (x); typeof(y) __y = (y); __x < __y ? __x : 
__y; })
> > 
> > int test(int a, int b, int c)
> > {
> >          return min(a, min(b, c));
> > }
> 
> Problems occur if the caller happens to pass in variables whose
> names confilct with the ones you chose in the above macro:

I remember the thread in detail.  We must have similar scoping problems in 
innumerable macros.  I'd a braindead flaw in C's scoping rules, and you know, 
I don't think it matters.  Don't use local variables with names like __foo.

--
Daniel

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Andrew Morton <akpm@zip.com.au>
   Date: Thu, 16 Aug 2001 18:49:58 -0700

   int test(int __x, int __y)
   {
           return min(__x, __y);		/* sic */
   }

People are expected not to use underscore prefixed
variables in normal C code, this is why macros
in the kernel make liberal use of them for locals.

I mean, with your rule half of the macro defines in the kernel using
local variables are broken :-)

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Jeff Dike]

aia21@cam.ac.uk said:
> There are no clashes; unless my knowledge of C has abandoned me at the
>  moment, you can reuse variable names within local statement blocks
> and the  compiler is intelligent enough to sort it out. 

Sorry, you're right.  I wasn't thinking this through enough.  I was thinking 
that
	min(a, min(b, c))
would cause cpp to do things like
	int __x = (__x)

It seems the only problematic case is calling it with __x type arguments.

				Jeff

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Anton Altaparmakov <aia21@cam.ac.uk>
   Date: Fri, 17 Aug 2001 00:22:43 +0100
   
   IMHO, it would have been more elegant to use the typeof construct provided 
   by gcc in the new macro instead of introducing a type parameter like this...

The whole point was to make users explicitly state the type so they
would have to think about it.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

>    IMHO, it would have been more elegant to use the typeof construct provided
>    by gcc in the new macro instead of introducing a type parameter like this...
> 
> The whole point was to make users explicitly state the type so they
> would have to think about it.

I have two problems with this:
1. They maybe think once about it, but are they doing it a second time?
If the type of the argument is changed somewhere in the header, the min
argument is easily missed, since...
2. This macro doesn't produce a warning like the typeof version does.
The typeof version warns you about signed/unsigned compares, while an
assignment gives no warning.

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 03:24:05 +0200

   2. This macro doesn't produce a warning like the typeof version does.
   The typeof version warns you about signed/unsigned compares, while an
   assignment gives no warning.

That is a legitimate operation, there is no reason to prevent people
from comparing signed and unsigned values.  These type argument
min/max values allow people to specify what the comparison type
degenerates into.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

> That is a legitimate operation, there is no reason to prevent people
> from comparing signed and unsigned values.  These type argument
> min/max values allow people to specify what the comparison type
> degenerates into.

Sure, but if people change something, they get a warning and can think
_again_, your current macro prevents this. That hardcoded type is just
to easy to miss and can introduce subtle bugs. The typeof version
automatically chooses that largest of the types and possibly warns you.
Why would you want something different?

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 04:29:42 +0200
   
   Why would you want something different?

Because comparing a signed value with an unsigned value is a perfectly
sane operation and one should not have to put dumb casts into the
expression or change the types just to avoid a compiler warning.

That's dumb.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

> Because comparing a signed value with an unsigned value is a perfectly
> sane operation and one should not have to put dumb casts into the
> expression or change the types just to avoid a compiler warning.

Most of the time that cast isn't needed and rather an indicator that
something else is wrong, right now we don't even have a chance to detect
such a situation.
Second, what's wrong with letting the compiler choose the appropriate
type? The compiler will still do it, if you change one of the types, but
the hardcoded type in min is too easy miss.

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 04:53:18 +0200

   Most of the time that cast isn't needed and rather an indicator that
   something else is wrong, right now we don't even have a chance to detect
   such a situation.

Wrong.  This is legal:

int test(unsigned long a, int b)
{
	return min(a, b);
}

And the compiler will warn about it with your typeof version.
That is dumb and unacceptable.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

> Wrong.  This is legal:
> 
> int test(unsigned long a, int b)
> {
>         return min(a, b);
> }
> 
> And the compiler will warn about it with your typeof version.
> That is dumb and unacceptable.

Please show me a place in the kernel where such code is used and is not
dumb.

Could you please answer my argument about maintainability?

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 05:23:01 +0200

   Please show me a place in the kernel where such code is used and is
   not dumb.

Why don't you point out an example yourself?  You seem pretty
confident that a comparsion between a signed and unsigned value cannot
possible be legitimate.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

>    Please show me a place in the kernel where such code is used and is
>    not dumb.
> 
> Why don't you point out an example yourself?  You seem pretty
> confident that a comparsion between a signed and unsigned value cannot
> possible be legitimate.

No, I did not say this. I said that such warning would be useful and
that the majority of users does not need the forced cast. This forced
cast is IMO more dangerous than useful.
I'd prefer people think about the warning, than suppress that warning
and let people run into subtle bugs. For the majority of uses, this will
be a bug or at least carelessness. For the minority of legal uses add a
cast + comment and everything is fine.

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[Anton Altaparmakov]

On Thu, 16 Aug 2001, David S. Miller wrote:
>    From: Roman Zippel <zippel@linux-m68k.org>
>    Date: Fri, 17 Aug 2001 05:23:01 +0200
> 
>    Please show me a place in the kernel where such code is used and is
>    not dumb.
> 
> Why don't you point out an example yourself?  You seem pretty
> confident that a comparsion between a signed and unsigned value cannot
> possible be legitimate.

If you compare int x with unsigned int y you can get the wrong result in
the case that the unsigned value has the sign bit set and the signed
value is negative.

Example:

$ cat t.c
#include <stdio.h>

int main(void)
{
        int x = -2;
        unsigned int y = 0x80000000;

        printf("x = %i, y = %u\n", x, y);
        if (x < y)
                puts("x < y");
        else if (x > y)
                puts("x > y");
        else
                puts("x == y");
        return 0;
}
$ gcc -Wall t.c
$ ./a.out
x = -2, y = 2147483648
x > y

This is clearly wrong as -2 is less than 2147483648 and I doubt very much
that bug would be caught by introducing type casts into the min
function... It will just put people at ease that everything is now fine
when in fact it won't be (unless they make the type cast to long long
which I doubt people would do).

A bug simillar to this was present in NTFS a while ago as a matter of
fact and was causing massive corruption of run lists... And gcc doesn't
even emit a warning hence it took quite a lot of work to spot that this
was the bug. In the end I found it by looking at the generated corrupt
structures and realizing that there must be a sign bug somewhere and then
I looked at the function and I saw the light and fixed this properly by
using the same type of variables rather than playing silly games with
mixing signed and unsigned values.

If gcc had emitted a warning this bug would never have been possible to
occur so I am all for generating warnings, not suppressing them
artificially, at least in this case.

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

Re: 2.4.9 does not compile [PATCH]

[Ben LaHaise]

On Thu, 16 Aug 2001, David S. Miller wrote:

> Wrong.  This is legal:
>
> int test(unsigned long a, int b)
> {
> 	return min(a, b);
> }
>
> And the compiler will warn about it with your typeof version.
> That is dumb and unacceptable.

I would hope that it would warn: what if a is the maximum size that an
array can be and b is a value passed in from userland?  Most definately
not an expected result.

		-ben

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Ben LaHaise <bcrl@redhat.com>
   Date: Fri, 17 Aug 2001 00:29:58 -0400 (EDT)

   On Thu, 16 Aug 2001, David S. Miller wrote:
   
   > Wrong.  This is legal:
   >
   > int test(unsigned long a, int b)
   > {
   > 	return min(a, b);
   > }
   
   I would hope that it would warn: what if a is the maximum size that an
   array can be and b is a value passed in from userland?  Most definately
   not an expected result.

My example was poor, consider if 'b' was something like '100'
or for some other reason you already know perfectly well
the legal range of 'b' or 'a'.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

> Because comparing a signed value with an unsigned value is a perfectly
> sane operation and one should not have to put dumb casts into the
> expression or change the types just to avoid a compiler warning.

BTW I just looked through the patch and I only found a single cast, so
there was not much need for such dumb casts. Your patch now forced the
cast into all of them...

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 05:08:40 +0200
   
   BTW I just looked through the patch and I only found a single cast, so
   there was not much need for such dumb casts. Your patch now forced the
   cast into all of them...

The cast in the new version is not dumb, it's smart.

It's the programmer saying (to both the reader of the
code and the compiler) "I want this comparison to use
type X".  Period.

There is no ambiguity, there are no multiple-evaluation
issues, and no dumb warnings from the compiler.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Roman Zippel]

Hi,

"David S. Miller" wrote:

> The cast in the new version is not dumb, it's smart.

Yes, it's the smart version of "min((type)a, (type)b)"...

> It's the programmer saying (to both the reader of the
> code and the compiler) "I want this comparison to use
> type X".  Period.

Why? Why would anyone want this? Do you need this for any other ordinary
compare? (For which the compiler will also generate "dumb" warnings.)

> There is no ambiguity, there are no multiple-evaluation
> issues, and no dumb warnings from the compiler.

I'm all for fixing the multiple-evaluation, but I don't see why this
warning should be dumb. If you don't like it, use "-Wno-sign-compare".

bye, Roman

Re: 2.4.9 does not compile [PATCH]

[David S. Miller]

   From: Roman Zippel <zippel@linux-m68k.org>
   Date: Fri, 17 Aug 2001 05:39:13 +0200

   Why? Why would anyone want this?

Ask Linus when he comes back from Finland. :-)

There are almost certainly threads deep in the linux-kernel archives
where Linus and others talk about this very issue.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile [PATCH]

[Rik van Riel]

On Thu, 16 Aug 2001, David S. Miller wrote:
>    From: tpepper@vato.org
>    Date: Thu, 16 Aug 2001 14:41:09 -0700
>
>    Confirmed here.  Looks like a pretty obvious goof to me.  Does the following
>    fix it for you?
>
> The args and semantics of min/max changed to take
> a type first argument, the problem with this ntfs file is that it
> fails to include linux/kernel.h

You should have checked this before deciding to rudely
bypass the maintainer of the file ;)

Rik
--
IA64: a worthy successor to the i860.

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

Re: 2.4.9 does not compile [PATCH]

[Anton Altaparmakov]

At 23:31 16/08/2001, David S. Miller wrote:
>    From: tpepper@vato.org
>    Date: Thu, 16 Aug 2001 14:41:09 -0700
>
>    Confirmed here.  Looks like a pretty obvious goof to me.  Does the 
> following
>    fix it for you?
>
>The args and semantics of min/max changed to take
>a type first argument, the problem with this ntfs file is that it
>fails to include linux/kernel.h

It has indeed. I do fail to see why that was necessary though...

IMHO, it would have been more elegant to use the typeof construct provided 
by gcc in the new macro instead of introducing a type parameter like this...

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

Best regards,

         Anton


-- 
   "Nothing succeeds like success." - Alexandre Dumas
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Linux NTFS Maintainer / WWW: http://linux-ntfs.sf.net/
ICQ: 8561279 / WWW: http://www-stu.christs.cam.ac.uk/~aia21/

2.4.9 does not compile (adaptec new driver)

[Luigi Genoni]

HI, fast to patch, but still noisy

make[5]: Entering directory
`/usr/src/linux-2.4.9/drivers/scsi/aic7xxx/aicasm'
gcc -I/usr/include -I. -ldb aicasm_gram.c aicasm_scan.c aicasm.c
aicasm_symbol.c -o aicasm
aicasm/aicasm_scan.l: In function `yylex':
aicasm/aicasm_scan.l:115: `T_VERSION' undeclared (first use in this
function)
aicasm/aicasm_scan.l:115: (Each undeclared identifier is reported only
once
aicasm/aicasm_scan.l:115: for each function it appears in.)
aicasm/aicasm_scan.l:132: `T_STRING' undeclared (first use in this
function)
make[5]: *** [aicasm] Error 1
make[5]: Leaving directory
`/usr/src/linux-2.4.9/drivers/scsi/aic7xxx/aicasm'
make[4]: *** [aicasm/aicasm] Error 2
make[4]: Leaving directory `/usr/src/linux-2.4.9/drivers/scsi/aic7xxx'
make[3]: *** [first_rule] Error 2
make[3]: Leaving directory `/usr/src/linux-2.4.9/drivers/scsi/aic7xxx'
make[2]: *** [_subdir_aic7xxx] Error 2
make[2]: Leaving directory `/usr/src/linux-2.4.9/drivers/scsi'
make[1]: *** [_subdir_scsi] Error 2
make[1]: Leaving directory `/usr/src/linux-2.4.9/drivers'
make: *** [_dir_drivers] Error 2

bests
Luigi

Re: 2.4.9 does not compile

[Adam J. Richter]

	The macro "min(n1,n2)", is a very standard practice in C
programming.  Having a different version of "min" adds unnecessary
porting work and ifdef clutter when code is port between the kernel
and other uses (typically small mathematical calculations, data
structure manipulations, and data stream tranformations like
compression and encryption, such as in freeswan, which I had to
modify).

	If a programmer wants to explicitly specify which type the
comparison converts to, then he or she can cast the arguments explicitly.
Programmers already have the opportunity "to think about it." Impeding
programming just to make programmers "think about it" rather than
treating their time as precious and letting them determine how to
invest it is rarely a winning trade-off, especially when you also
weigh the quality effects of increased code clutter and less programmer
time available for other improvements.  To call this change Pascal-like
would be an insult to Pascal.

	If you really want this facility, you could just declare
a distinct "typed_min" macro.

Adam J. Richter     __     ______________   4880 Stevens Creek Blvd, Suite 104
adam@yggdrasil.com     \ /                  San Jose, California 95129-1034
+1 408 261-6630         | g g d r a s i l   United States of America
fax +1 408 261-6631      "Free Software For The Rest Of Us."

Re: 2.4.9 does not compile

[David S. Miller]

   From: "Adam J. Richter" <adam@yggdrasil.com>
   Date: Thu, 16 Aug 2001 22:03:07 -0700

   	The macro "min(n1,n2)", is a very standard practice in C
   programming.

If min() in it's "very standard practice" form is broken at
the core, breaking it like I have is a fix.

Let this go till Linus returns from Finland in a week or so,
I'm sure he'll be more than happy to state why he wanted
me to do these changes.

Later,
David S. Miller
davem@redhat.com

Re: 2.4.9 does not compile

[Adam J. Richter]

>   	The macro "min(n1,n2)", is a very standard practice in C
>   programming.

>If min() in it's "very standard practice" form is broken at
>the core, breaking it like I have is a fix.

	Granted, in some cases, I am in favor of the Linux kernel
being the innovator in violating some prevailing standards that I
regard as sufficientliy "broken at the core", like, say, the
popularity of really long variable names.  However, I think just about
nobody else in this discussion accepts the major premise of that
syllogism that min(n1,n2) is "broken to the core", at least not
to the degree that changing it that way is a fix.

>Let this go till Linus returns from Finland in a week or so,
>I'm sure he'll be more than happy to state why he wanted
>me to do these changes.

	OK.

Adam J. Richter     __     ______________   4880 Stevens Creek Blvd, Suite 104
adam@yggdrasil.com     \ /                  San Jose, California 95129-1034
+1 408 261-6630         | g g d r a s i l   United States of America
fax +1 408 261-6631      "Free Software For The Rest Of Us."

Re: 2.4.9 does not compile

[Alan Cox]

> 	If you really want this facility, you could just declare
> a distinct "typed_min" macro.

For -ac I intend to change it to exactly that. Then we can also back
propogate compatibility macros. Abusing min prevents that