2.4.9 does not compile

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

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

@@ -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++);

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

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

  "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/

From: [email protected]
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
[email protected]

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/

From: Anton Altaparmakov <[email protected]>
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
[email protected]

Hi,

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

From: Roman Zippel <[email protected]>
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
[email protected]

Hi,

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

From: Roman Zippel <[email protected]>
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
[email protected]

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/

Hi,

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

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

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

From: Roman Zippel <[email protected]>
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
[email protected]

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/

Hi,

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

From: Roman Zippel <[email protected]>
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
[email protected]

Hi,

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

From: Roman Zippel <[email protected]>
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
[email protected]

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

From: Ben LaHaise <[email protected]>
Date: Fri, 17 Aug 2001 00:29:58 -0400 (EDT)

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
[email protected]

Hi,

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

Hi,

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

From: Roman Zippel <[email protected]>
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
[email protected]

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
@@ -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         *

*********************************************************

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.

  "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/

lvm does not compile either.

Here's the complete patch for 2.4.9

Jeff

@@ -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++) {

@@ -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);
 

@@ -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++);

following

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.

--
Daniel

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

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.

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

  "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/

From: Anton Altaparmakov <[email protected]>
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
[email protected]

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

Hi,

Thanks for the reply, but...

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...

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/

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

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...

-

From: Andrew Morton <[email protected]>
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
[email protected]

__y; })

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

From: Daniel Phillips <[email protected]>
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
[email protected]

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