[PATCH] s390 (7/13): gcc 3.3 adaptions.

[PATCH] s390 (7/13): gcc 3.3 adaptions.

[Martin Schwidefsky]

updates for compiling with gcc-3.3pre

- add -finline-limit=10000 to make it build
- drop .eh_frame elf section from vmlinux
- fix common warnings inn asm headers
- make dasd compile
- Don't warn about signed/unsigned comparisions
- fix inline syscall macros

diff -urN linux-2.5.62/arch/s390/Makefile linux-2.5.62-s390/arch/s390/Makefile
--- linux-2.5.62/arch/s390/Makefile	Mon Feb 17 23:56:00 2003
+++ linux-2.5.62-s390/arch/s390/Makefile	Mon Feb 24 18:24:21 2003
@@ -18,7 +18,7 @@
 LDFLAGS_vmlinux := -e start
 LDFLAGS_BLOB	:= --format binary --oformat elf32-s390
 
-CFLAGS += -pipe -fno-strength-reduce
+CFLAGS += -pipe -fno-strength-reduce -finline-limit=10000 -Wno-sign-compare
 
 head-y := arch/$(ARCH)/kernel/head.o arch/$(ARCH)/kernel/init_task.o
 
diff -urN linux-2.5.62/arch/s390/vmlinux.lds.S linux-2.5.62-s390/arch/s390/vmlinux.lds.S
--- linux-2.5.62/arch/s390/vmlinux.lds.S	Mon Feb 17 23:56:11 2003
+++ linux-2.5.62-s390/arch/s390/vmlinux.lds.S	Mon Feb 24 18:24:21 2003
@@ -64,6 +64,9 @@
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __start___param = .;
+  __param : { *(__param) }
+  __stop___param = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 
@@ -98,6 +101,7 @@
 	*(.exit.text)
 	*(.exit.data)
 	*(.exitcall.exit)
+	*(.eh_frame)
 	}
 
   /* Stabs debugging sections.  */
diff -urN linux-2.5.62/arch/s390x/Makefile linux-2.5.62-s390/arch/s390x/Makefile
--- linux-2.5.62/arch/s390x/Makefile	Mon Feb 17 23:56:15 2003
+++ linux-2.5.62-s390/arch/s390x/Makefile	Mon Feb 24 18:24:21 2003
@@ -19,7 +19,7 @@
 MODFLAGS += -fpic -D__PIC__
 LDFLAGS_BLOB	:= --format binary --oformat elf64-s390
 
-CFLAGS += -pipe -fno-strength-reduce
+CFLAGS += -pipe -fno-strength-reduce -finline-limit=10000 -Wno-sign-compare
 
 head-y := arch/$(ARCH)/kernel/head.o arch/$(ARCH)/kernel/init_task.o
 
diff -urN linux-2.5.62/arch/s390x/vmlinux.lds.S linux-2.5.62-s390/arch/s390x/vmlinux.lds.S
--- linux-2.5.62/arch/s390x/vmlinux.lds.S	Mon Feb 17 23:56:10 2003
+++ linux-2.5.62-s390/arch/s390x/vmlinux.lds.S	Mon Feb 24 18:24:21 2003
@@ -64,6 +64,9 @@
   __setup_start = .;
   .init.setup : { *(.init.setup) }
   __setup_end = .;
+  __start___param = .;
+  __param : { *(__param) }
+  __stop___param = .;
   __initcall_start = .;
   .initcall.init : {
 	*(.initcall1.init) 
@@ -98,6 +101,7 @@
 	*(.exit.text)
 	*(.exit.data)
 	*(.exitcall.exit)
+	*(.eh_frame)
 	}
 
   /* Stabs debugging sections.  */
diff -urN linux-2.5.62/drivers/s390/block/dasd_eckd.h linux-2.5.62-s390/drivers/s390/block/dasd_eckd.h
--- linux-2.5.62/drivers/s390/block/dasd_eckd.h	Mon Feb 17 23:56:13 2003
+++ linux-2.5.62-s390/drivers/s390/block/dasd_eckd.h	Mon Feb 24 18:24:21 2003
@@ -5,7 +5,7 @@
  * Bugreports.to..: <Linux390@de.ibm.com>
  * (C) IBM Corporation, IBM Deutschland Entwicklung GmbH, 1999,2000
  *
- * $Revision: 1.5 $
+ * $Revision: 1.6 $
  *
  * History of changes 
  * 
@@ -109,7 +109,7 @@
 		unsigned char cfw:1;	/* Cache fast write */
 		unsigned char dfw:1;	/* DASD fast write */
 	} __attribute__ ((packed)) attributes;
-	__u16 short blk_size;	/* Blocksize */
+	__u16 blk_size;		/* Blocksize */
 	__u16 fast_write_id;
 	__u8 ga_additional;	/* Global Attributes Additional */
 	__u8 ga_extended;	/* Global Attributes Extended	*/
diff -urN linux-2.5.62/include/asm-s390/bitops.h linux-2.5.62-s390/include/asm-s390/bitops.h
--- linux-2.5.62/include/asm-s390/bitops.h	Mon Feb 17 23:55:50 2003
+++ linux-2.5.62-s390/include/asm-s390/bitops.h	Mon Feb 24 18:24:21 2003
@@ -469,7 +469,7 @@
  * This routine doesn't need to be atomic.
  */
 
-static inline int __test_bit(int nr, volatile unsigned long *ptr)
+static inline int __test_bit(int nr, const volatile unsigned long *ptr)
 {
 	unsigned long addr;
 	unsigned char ch;
@@ -480,7 +480,7 @@
 }
 
 static inline int 
-__constant_test_bit(int nr, volatile unsigned long * addr) {
+__constant_test_bit(int nr, const volatile unsigned long * addr) {
     return (((volatile char *) addr)[(nr>>3)^3] & (1<<(nr&7))) != 0;
 }
 
diff -urN linux-2.5.62/include/asm-s390/idals.h linux-2.5.62-s390/include/asm-s390/idals.h
--- linux-2.5.62/include/asm-s390/idals.h	Mon Feb 17 23:55:52 2003
+++ linux-2.5.62-s390/include/asm-s390/idals.h	Mon Feb 24 18:24:21 2003
@@ -191,10 +191,10 @@
 __idal_buffer_is_needed(struct idal_buffer *ib)
 {
 #ifdef CONFIG_ARCH_S390X
-	return ib->size > (4096 << ib->page_order) ||
+	return ib->size > (4096ul << ib->page_order) ||
 		idal_is_needed(ib->data[0], ib->size);
 #else
-	return ib->size > (4096 << ib->page_order);
+	return ib->size > (4096ul << ib->page_order);
 #endif
 }
 
diff -urN linux-2.5.62/include/asm-s390/unistd.h linux-2.5.62-s390/include/asm-s390/unistd.h
--- linux-2.5.62/include/asm-s390/unistd.h	Mon Feb 24 18:18:38 2003
+++ linux-2.5.62-s390/include/asm-s390/unistd.h	Mon Feb 24 18:24:21 2003
@@ -266,27 +266,29 @@
 
 #define _syscall0(type,name)                                 \
 type name(void) {                                            \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name)                          \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
 #define _syscall1(type,name,type1,arg1)                      \
 type name(type1 arg1) {                                      \
         register type1 __arg1 asm("2") = arg1;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -294,15 +296,16 @@
 type name(type1 arg1, type2 arg2) {                          \
         register type1 __arg1 asm("2") = arg1;               \
         register type2 __arg2 asm("3") = arg2;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -311,16 +314,17 @@
         register type1 __arg1 asm("2") = arg1;               \
         register type2 __arg2 asm("3") = arg2;               \
         register type3 __arg3 asm("4") = arg3;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
                   "d" (__arg3)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -331,17 +335,18 @@
         register type2 __arg2 asm("3") = arg2;               \
         register type3 __arg3 asm("4") = arg3;               \
         register type4 __arg4 asm("5") = arg4;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
                   "d" (__arg3),                              \
                   "d" (__arg4)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -354,11 +359,11 @@
         register type3 __arg3 asm("4") = arg3;               \
         register type4 __arg4 asm("5") = arg4;               \
         register type5 __arg5 asm("6") = arg5;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lr  %0,2"                               \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
@@ -366,6 +371,7 @@
                   "d" (__arg4),                              \
                   "d" (__arg5)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
diff -urN linux-2.5.62/include/asm-s390x/bitops.h linux-2.5.62-s390/include/asm-s390x/bitops.h
--- linux-2.5.62/include/asm-s390x/bitops.h	Mon Feb 17 23:56:59 2003
+++ linux-2.5.62-s390/include/asm-s390x/bitops.h	Mon Feb 24 18:24:21 2003
@@ -473,7 +473,7 @@
  * This routine doesn't need to be atomic.
  */
 
-static inline int __test_bit(unsigned long nr, volatile unsigned long *ptr)
+static inline int __test_bit(unsigned long nr, const volatile unsigned long *ptr)
 {
 	unsigned long addr;
 	unsigned char ch;
@@ -484,7 +484,7 @@
 }
 
 static inline int 
-__constant_test_bit(unsigned long nr, volatile unsigned long *addr) {
+__constant_test_bit(unsigned long nr, const volatile unsigned long *addr) {
     return (((volatile char *) addr)[(nr>>3)^7] & (1<<(nr&7))) != 0;
 }
 
diff -urN linux-2.5.62/include/asm-s390x/idals.h linux-2.5.62-s390/include/asm-s390x/idals.h
--- linux-2.5.62/include/asm-s390x/idals.h	Mon Feb 17 23:56:22 2003
+++ linux-2.5.62-s390/include/asm-s390x/idals.h	Mon Feb 24 18:24:21 2003
@@ -191,10 +191,10 @@
 __idal_buffer_is_needed(struct idal_buffer *ib)
 {
 #ifdef CONFIG_ARCH_S390X
-	return ib->size > (4096 << ib->page_order) ||
+	return ib->size > (4096ul << ib->page_order) ||
 		idal_is_needed(ib->data[0], ib->size);
 #else
-	return ib->size > (4096 << ib->page_order);
+	return ib->size > (4096ul << ib->page_order);
 #endif
 }
 
diff -urN linux-2.5.62/include/asm-s390x/unistd.h linux-2.5.62-s390/include/asm-s390x/unistd.h
--- linux-2.5.62/include/asm-s390x/unistd.h	Mon Feb 24 18:18:38 2003
+++ linux-2.5.62-s390/include/asm-s390x/unistd.h	Mon Feb 24 18:24:21 2003
@@ -233,27 +233,29 @@
 
 #define _syscall0(type,name)                                 \
 type name(void) {                                            \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name)                          \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
 #define _syscall1(type,name,type1,arg1)                      \
 type name(type1 arg1) {                                      \
         register type1 __arg1 asm("2") = arg1;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -261,15 +263,16 @@
 type name(type1 arg1, type2 arg2) {                          \
         register type1 __arg1 asm("2") = arg1;               \
         register type2 __arg2 asm("3") = arg2;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -278,16 +281,17 @@
         register type1 __arg1 asm("2") = arg1;               \
         register type2 __arg2 asm("3") = arg2;               \
         register type3 __arg3 asm("4") = arg3;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
                   "d" (__arg3)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -298,17 +302,18 @@
         register type2 __arg2 asm("3") = arg2;               \
         register type3 __arg3 asm("4") = arg3;               \
         register type4 __arg4 asm("5") = arg4;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
                   "d" (__arg3),                              \
                   "d" (__arg4)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 
@@ -321,11 +326,11 @@
         register type3 __arg3 asm("4") = arg3;               \
         register type4 __arg4 asm("5") = arg4;               \
         register type5 __arg5 asm("6") = arg5;               \
-        register long __res asm("2");                        \
+        register long __svcres asm("2");                     \
+        long __res;                                          \
         __asm__ __volatile__ (                               \
                 "    svc %b1\n"                              \
-                "    lgr  %0,2"                              \
-                : "=d" (__res)                               \
+                : "=d" (__svcres)                            \
                 : "i" (__NR_##name),                         \
                   "0" (__arg1),                              \
                   "d" (__arg2),                              \
@@ -333,6 +338,7 @@
                   "d" (__arg4),                              \
                   "d" (__arg5)                               \
                 : _svc_clobber );                            \
+	__res = __svcres;                                    \
         __syscall_return(type,__res);                        \
 }
 

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Richard B. Johnson]

On Mon, 24 Feb 2003, Martin Schwidefsky wrote:

> updates for compiling with gcc-3.3pre
> 

[SNIPPED...]
> - Don't warn about signed/unsigned comparisions

I think you must keep these warnings in! There are many bugs
that these uncover uncluding loops that don't terminate correctly
but seem to work for "most all" cases. These are the hard-to-find
bugs that hit you six months after release.

size_t i;

   while((i = do_forever()) > 0)
          ;

... do_forever() finally errors out and returns -1 stuck(forever).


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Mon, 24 Feb 2003, Richard B. Johnson wrote:
> 
> I think you must keep these warnings in! There are many bugs
> that these uncover uncluding loops that don't terminate correctly
> but seem to work for "most all" cases. These are the hard-to-find
> bugs that hit you six months after release.

At least historically gcc has been so f*cking bad at the "unsigned vs 
signed" warnings that they are totally useless.

Maybe things are better in gcc-3.3.

Maybe not.


> size_t i;
> 
>    while((i = do_forever()) > 0)
>           ;
> 
> ... do_forever() finally errors out and returns -1 stuck(forever).

Does gcc still warn about things like

	#define COUNT (sizeof(array)/sizeof(element))

	int i;
	for (i = 0; i < COUNT; i++)
		...

where COUNT is obviously unsigned (because sizeof is size_t and thus 
unsigned)?

Gcc used to complain about things like that, which is a FUCKING DISASTER. 

Any compiler that complains about the above should be shot in the head, 
and the warning should be killed.

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jeff Garzik]

On Mon, Feb 24, 2003 at 01:02:39PM -0800, Linus Torvalds wrote:
> Does gcc still warn about things like
> 
> 	#define COUNT (sizeof(array)/sizeof(element))
> 
> 	int i;
> 	for (i = 0; i < COUNT; i++)
> 		...
> 
> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> unsigned)?
> 
> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> Any compiler that complains about the above should be shot in the head, 
> and the warning should be killed.

Maybe...  I suppose it's an implementation issue, because the lack of
signedness issues is probably only noticeable after data value analysis.

Playing devil's advocate here, I actually don't mind it warning for a
scenarion like this, because quite often it indicates an area where, if
s/int/unsigned int/ is performed, the compiler could potentially do a
better job of optimizing.

I agree your above specific example shouldn't trigger a warning
[implementation excuses aside].

	Jeff

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Richard B. Johnson]

On Mon, 24 Feb 2003, Linus Torvalds wrote:

> 
> On Mon, 24 Feb 2003, Richard B. Johnson wrote:
> > 
> > I think you must keep these warnings in! There are many bugs
> > that these uncover uncluding loops that don't terminate correctly
> > but seem to work for "most all" cases. These are the hard-to-find
> > bugs that hit you six months after release.
> 
> At least historically gcc has been so f*cking bad at the "unsigned vs 
> signed" warnings that they are totally useless.
> 
> Maybe things are better in gcc-3.3.
> 
> Maybe not.
> 
> 
> > size_t i;
> > 
> >    while((i = do_forever()) > 0)
> >           ;
> > 
> > ... do_forever() finally errors out and returns -1 stuck(forever).
> 
> Does gcc still warn about things like
> 
> 	#define COUNT (sizeof(array)/sizeof(element))
> 
> 	int i;
> 	for (i = 0; i < COUNT; i++)
> 		...
> 
> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> unsigned)?
> 
> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> Any compiler that complains about the above should be shot in the head, 
> and the warning should be killed.
> 
> 		Linus
>

Well yes. And it's correct. Variable `i' in the following code
should be a 'size_t'.
 
Script started on Mon Feb 24 16:23:19 2003
# cat xxx.c
#define ArraySize(a) (sizeof(a) / sizeof(a[0]))
int foo[12] = {0,};
int count()
{
    int i;
    for(i=0; i< ArraySize(foo); i++)
        ;
    return i;
}

# gcc -Wall -Wsign-compare -c -o xxx.o xxx.c
xxx.c: In function `count':
xxx.c:11: warning: comparison between signed and unsigned
# exit
exit
Script done on Mon Feb 24 16:23:50 2003

... and we all know that it's dumb to do:
    for(i=0; i< (int) ArraySize(foo); i++)
... just to placate a compiler. But the compiler did find what
could-have-been a real problem so the compiler is, indeed, correct.

Maybe what is needed is -Wsign-compare and -Wsign-strict, where
-Wsign-compare warns of obvious errors that could cause continuous
loops, etc., and -Wsign-strict acts like the current -Wsign-compare.
I agree that the current -Wsign-compare is way too strict for the
usual coding style where most int compares are "don't care" things
that are not worth casts.

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Linus Torvalds <torvalds@transmeta.com> writes:

|> Does gcc still warn about things like
|> 
|> 	#define COUNT (sizeof(array)/sizeof(element))
|> 
|> 	int i;
|> 	for (i = 0; i < COUNT; i++)
|> 		...
|> 
|> where COUNT is obviously unsigned (because sizeof is size_t and thus 
|> unsigned)?
|> 
|> Gcc used to complain about things like that, which is a FUCKING DISASTER. 

How can you distinguish that from other occurrences of (int)<(size_t)?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jeff Garzik]

On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
> Linus Torvalds <torvalds@transmeta.com> writes:
> 
> |> Does gcc still warn about things like
> |> 
> |> 	#define COUNT (sizeof(array)/sizeof(element))
> |> 
> |> 	int i;
> |> 	for (i = 0; i < COUNT; i++)
> |> 		...
> |> 
> |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> |> unsigned)?
> |> 
> |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> How can you distinguish that from other occurrences of (int)<(size_t)?

The bounds are obviously constant and unsigned at compile time.

	Jeff

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Jeff Garzik <jgarzik@pobox.com> writes:

|> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
|> > Linus Torvalds <torvalds@transmeta.com> writes:
|> > 
|> > |> Does gcc still warn about things like
|> > |> 
|> > |> 	#define COUNT (sizeof(array)/sizeof(element))
|> > |> 
|> > |> 	int i;
|> > |> 	for (i = 0; i < COUNT; i++)
|> > |> 		...
|> > |> 
|> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
|> > |> unsigned)?
|> > |> 
|> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
|> > 
|> > How can you distinguish that from other occurrences of (int)<(size_t)?
|> 
|> The bounds are obviously constant and unsigned at compile time.

But that's not the point.  It's the runtime value of i that gets converted
(to unsigned), not the compile time value of COUNT.  Thus if i ever gets
negative you have a problem.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jörn Engel]

On Mon, 24 February 2003 23:07:25 +0100, Andreas Schwab wrote:
> 
> Jeff Garzik <jgarzik@pobox.com> writes:
> 
> |> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
> |> > Linus Torvalds <torvalds@transmeta.com> writes:
> |> > 
> |> > |> Does gcc still warn about things like
> |> > |> 
> |> > |> 	#define COUNT (sizeof(array)/sizeof(element))
> |> > |> 
> |> > |> 	int i;
> |> > |> 	for (i = 0; i < COUNT; i++)
> |> > |> 		...
> |> > |> 
> |> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> |> > |> unsigned)?
> |> > |> 
> |> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> |> > 
> |> > How can you distinguish that from other occurrences of (int)<(size_t)?
> |> 
> |> The bounds are obviously constant and unsigned at compile time.
> 
> But that's not the point.  It's the runtime value of i that gets converted
> (to unsigned), not the compile time value of COUNT.  Thus if i ever gets
> negative you have a problem.

COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
to the variable's type to fix this problem.
Or, gcc can see that i starts with 0, it's value monotonously
increases and will never wrap around due to COUNT < INT_MAX. Not a
cheap test, but still possible.

Jörn

-- 
My second remark is that our intellectual powers are rather geared to
master static relations and that our powers to visualize processes
evolving in time are relatively poorly developed.
-- Edsger W. Dijkstra

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jörn Engel]

On Mon, 24 February 2003 23:21:44 +0100, Jörn Engel wrote:
> 
> COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
> to the variable's type to fix this problem.

Ok, Adrians example proved this approach stupid. But the one below
remains useful and catches one example, but not the other.

> Or, gcc can see that i starts with 0, it's value monotonously
> increases and will never wrap around due to COUNT < INT_MAX. Not a
> cheap test, but still possible.


Jörn

-- 
ticks = jiffies;
while (ticks == jiffies);
ticks = jiffies;
-- /usr/src/linux/init/main.c

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Jörn Engel <joern@wohnheim.fh-wedel.de> writes:

|> On Mon, 24 February 2003 23:07:25 +0100, Andreas Schwab wrote:
|> > 
|> > Jeff Garzik <jgarzik@pobox.com> writes:
|> > 
|> > |> On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
|> > |> > Linus Torvalds <torvalds@transmeta.com> writes:
|> > |> > 
|> > |> > |> Does gcc still warn about things like
|> > |> > |> 
|> > |> > |> 	#define COUNT (sizeof(array)/sizeof(element))
|> > |> > |> 
|> > |> > |> 	int i;
|> > |> > |> 	for (i = 0; i < COUNT; i++)
|> > |> > |> 		...
|> > |> > |> 
|> > |> > |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
|> > |> > |> unsigned)?
|> > |> > |> 
|> > |> > |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
|> > |> > 
|> > |> > How can you distinguish that from other occurrences of (int)<(size_t)?
|> > |> 
|> > |> The bounds are obviously constant and unsigned at compile time.
|> > 
|> > But that's not the point.  It's the runtime value of i that gets converted
|> > (to unsigned), not the compile time value of COUNT.  Thus if i ever gets
|> > negative you have a problem.
|> 
|> COUNT is constant and COUNT < INT_MAX. gcc can cast the constant bound
|> to the variable's type to fix this problem.

This is not how C works.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Mon, 24 Feb 2003, Andreas Schwab wrote:
> 
> But that's not the point.  It's the runtime value of i that gets converted
> (to unsigned), not the compile time value of COUNT.  Thus if i ever gets
> negative you have a problem.

The point is that the compiler should see that the run-time value of i is 
_obviously_never_negative_ and as such the warning is total and utter 
crap.

The compiler actually does end up doing some of that kind of analysis when
optimizing, since it is required for some of the loop optimizations 
anyway. But the warning is emitted before the analysis has taken place.

In other words, gcc is stupidly warning about something that is obviously 
not an error. And it is obviously not an error because:

 - array indexes are "int". They aren't size_t, and never have been. Thus 
   it is _correct_ to use "int" for the index. You write

	int array[5];

   and you do NOT write

	int array[5UL];

   and anybody who writes 'array[5UL]' is considered a stupid git and a 
   geek. Face it.

   Claiming that you should index an array with a size_t is crap.

 - the way this has traditionally always been done is the example I 
   posted. Warning about correct, obvious, and traditional code is WRONG, 
   if it causes the programmer to have to write something _less_ obvious
   just to make a stupid compiler warning go away makes the warnign WORSE 
   THAN USELESS.

And yes, gcc could do the work necessary to only give the warning if it 
actually has reason to believe that the code is wrong. As it is, it gives 
the warning for code that is good.

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Alan Cox]

On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> And yes, gcc could do the work necessary to only give the warning if it 
> actually has reason to believe that the code is wrong. As it is, it gives 
> the warning for code that is good.

gcc gives the warning only when you ask it to annoy you. Seems a good trade off. There
are about 15 bug fixes in 2.4.21-pre4ac4,ac5,ac6 solely from that, all real bugs and some
very non obvious
 

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On 25 Feb 2003, Alan Cox wrote:
> On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> > And yes, gcc could do the work necessary to only give the warning if it 
> > actually has reason to believe that the code is wrong. As it is, it gives 
> > the warning for code that is good.
> 
> gcc gives the warning only when you ask it to annoy you. Seems a good trade off.

That _used_ to be true.

Look at the subject line. gcc-3.3 gives the warning for -Wall.

			Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Alan Cox]

On Mon, 2003-02-24 at 23:20, Linus Torvalds wrote:
> On 25 Feb 2003, Alan Cox wrote:
> > On Mon, 2003-02-24 at 22:39, Linus Torvalds wrote:
> > > And yes, gcc could do the work necessary to only give the warning if it 
> > > actually has reason to believe that the code is wrong. As it is, it gives 
> > > the warning for code that is good.
> > 
> > gcc gives the warning only when you ask it to annoy you. Seems a good trade off.
> 
> That _used_ to be true.
> 
> Look at the subject line. gcc-3.3 gives the warning for -Wall.

gcc-3.3 doesnt exist yet. Maybe it wont do that now 8)

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On 25 Feb 2003, Alan Cox wrote:
> 
> gcc-3.3 doesnt exist yet. Maybe it wont do that now 8)

Right now there are some other problems with gcc-3.3 too, ie the inlining
is apparently broken enough that we'll either have to start using
__attribute__((force_inline)) or we'd better hope that the gcc people
decide to take the "inline" keyword more seriously (it's being discussed
on the gcc lists, so we'll see)

But yes, these are all obviously with "early versions", and it may be that 
it changes before the real release.

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Chris Wedgwood]

On Mon, Feb 24, 2003 at 02:39:36PM -0800, Linus Torvalds wrote:

> Claiming that you should index an array with a size_t is crap.

it's broken in general; there is *lots* of code which does things like
"foo[-1] = bar" (string parsers for example)


  --cw

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jamie Lokier]

Chris Wedgwood wrote:
> On Mon, Feb 24, 2003 at 02:39:36PM -0800, Linus Torvalds wrote:
> 
> > Claiming that you should index an array with a size_t is crap.
> 
> it's broken in general; there is *lots* of code which does things like
> "foo[-1] = bar" (string parsers for example)

Yeah, the index should be ptrdiff_t :)

-- Jamie

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Linus Torvalds <torvalds@transmeta.com> writes:

|> On Mon, 24 Feb 2003, Andreas Schwab wrote:
|> > 
|> > But that's not the point.  It's the runtime value of i that gets converted
|> > (to unsigned), not the compile time value of COUNT.  Thus if i ever gets
|> > negative you have a problem.
|> 
|> The point is that the compiler should see that the run-time value of i is 
|> _obviously_never_negative_ and as such the warning is total and utter 
|> crap.

This requires a complete analysis of the loop body, which means that the
warning must be moved down from the front end (the common type of the
operands only depends on the type of the operands, not of any current
value of the expressions).

|> The compiler actually does end up doing some of that kind of analysis when
|> optimizing, since it is required for some of the loop optimizations 
|> anyway. But the warning is emitted before the analysis has taken place.
|> 
|> In other words, gcc is stupidly warning about something that is obviously 
|> not an error. And it is obviously not an error because:
|> 
|>  - array indexes are "int". They aren't size_t, and never have been. Thus 
|>    it is _correct_ to use "int" for the index. You write
|> 
|> 	int array[5];
|> 
|>    and you do NOT write
|> 
|> 	int array[5UL];
|> 
|>    and anybody who writes 'array[5UL]' is considered a stupid git and a 
|>    geek. Face it.

But array[-1] is wrong.  An array can never have a negative index (I'm
*not* talking about pointers).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Tue, 25 Feb 2003, Andreas Schwab wrote:
> |> 
> |> The point is that the compiler should see that the run-time value of i is 
> |> _obviously_never_negative_ and as such the warning is total and utter 
> |> crap.
> 
> This requires a complete analysis of the loop body, which means that the
> warning must be moved down from the front end (the common type of the
> operands only depends on the type of the operands, not of any current
> value of the expressions).

So? Gcc does that anyway. _Any_ good compiler has to.

And if the compiler isn't good enough to do it, then the compiler 
shouldn't be warning about something that it hasn't got a clue about.

> |>    and anybody who writes 'array[5UL]' is considered a stupid git and a 
> |>    geek. Face it.
> 
> But array[-1] is wrong.  An array can never have a negative index (I'm
> *not* talking about pointers).

You're wrong.

Yes, when declaring an array, you cannot use "array[-1]". But that's not 
because the thing is unsigned: the standard says that the array 
declaration has to be a "integer value larger than zero". It is not 
unsigned: it's _positive_.

However, in _indexing_ an array (as opposed to declaring it), "array[-1]" 
is indeed perfectly fine, and is defined by the C language to be exactly 
the same as "*(array-1)". And negative values are perfectly fine, even for 
arrays. Trivial example:

	int x[2][2];

	int main(int argc, char **argv)
	{
		return x[1][-1];
	}


the above is actually a well-defined C program, and 100%
standards-conforming ("strictly conforming").

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Linus Torvalds <torvalds@transmeta.com> writes:

|> On Tue, 25 Feb 2003, Andreas Schwab wrote:
|> > |> 
|> > |> The point is that the compiler should see that the run-time value of i is 
|> > |> _obviously_never_negative_ and as such the warning is total and utter 
|> > |> crap.
|> > 
|> > This requires a complete analysis of the loop body, which means that the
|> > warning must be moved down from the front end (the common type of the
|> > operands only depends on the type of the operands, not of any current
|> > value of the expressions).
|> 
|> So? Gcc does that anyway. _Any_ good compiler has to.

But the point is that determining the common type does not require _any_
kind of data flow analysis, and this is the place where the unsigned
warning is generated.

|> Trivial example:
|> 
|> 	int x[2][2];
|> 
|> 	int main(int argc, char **argv)
|> 	{
|> 		return x[1][-1];
|> 	}
|> 
|> 
|> the above is actually a well-defined C program, and 100%
|> standards-conforming ("strictly conforming").

This isn't as trivial as it seems.  Look in comp.std.c for recent
discussions on this topic (out-of-array references).

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Tue, 25 Feb 2003, Andreas Schwab wrote:
> |> 
> |> So? Gcc does that anyway. _Any_ good compiler has to.
> 
> But the point is that determining the common type does not require _any_
> kind of data flow analysis, and this is the place where the unsigned
> warning is generated.

Go back and read my mail. In fact, go back and read just about _any_ of my 
mails on the subject. Gop back and read the part you snipped:

 "And if the compiler isn't good enough to do it, then the compiler 
  shouldn't be warning about something that it hasn't got a clue about."

In other words, either gcc should do it right, or it should not be done at 
all. Right now the warning IS GENERATED IN THE WRONG PLACE. Your argument 
seems to be "but that's the place it is generated" is a total 
non-argument. It's just stating a fact, and it's exactly that fact that 
causes the BUG IN GCC.

> |> Trivial example:
> |> 
> |> 	int x[2][2];
> |> 
> |> 	int main(int argc, char **argv)
> |> 	{
> |> 		return x[1][-1];
> |> 	}
> |> 
> |> 
> |> the above is actually a well-defined C program, and 100%
> |> standards-conforming ("strictly conforming").
> 
> This isn't as trivial as it seems.  Look in comp.std.c for recent
> discussions on this topic (out-of-array references). 

It is as trivial as it seems, and this is _not_ an out-of-array reference.  
Sorry. C very clearly specifies the layout of arrays (6.5.2.1: last
subscript varies fastest, together with sizeof(array) = sizeof(entry)*n),
which _guarantees_ that the pointers involved in the above calculations
are all within the object, and thus there is _zero_ undefined behaviour.

So if they argued about this on comp.std.c, they either had clueless
people there (in addition to the ones that obviously aren't ;), _or_ you
misunderstood the argument.

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Tue, 25 Feb 2003, Linus Torvalds wrote:
>
> 		return x[1][-1];

Btw, don't get me wrong. I don't think the above is really code that 
should survive, and if the compiler were to generate a warning for 
something like that, where the subscripts are clearly out of the range 
that they were in the declaration, then I'd be entirely supportive of 
that.

I don't know how we got side-tracked to negative subscripts. They are 
clearly legal with pointers, but that wasn't even the issue: the code that 
generated the "signed/unsigned" warning didn't use any negative 
subscripts, never had, and never will.

And unlike the abomination above, the code that generates the warning is 
the _clearest_ version of code you can humanly write. And THAT is the 
problem with the warning: there's no way to avoid the warning without 
making the source code _worse_ in some way.

And _that_ is what my argument really boils down to. Nothing else.

		Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Andreas Schwab]

Linus Torvalds <torvalds@transmeta.com> writes:

|> On Tue, 25 Feb 2003, Andreas Schwab wrote:
|> > |> 
|> > |> So? Gcc does that anyway. _Any_ good compiler has to.
|> > 
|> > But the point is that determining the common type does not require _any_
|> > kind of data flow analysis, and this is the place where the unsigned
|> > warning is generated.
|> 
|> Go back and read my mail. In fact, go back and read just about _any_ of my 
|> mails on the subject. Gop back and read the part you snipped:
|> 
|>  "And if the compiler isn't good enough to do it, then the compiler 
|>   shouldn't be warning about something that it hasn't got a clue about."
|> 
|> In other words, either gcc should do it right, or it should not be done at 
|> all. Right now the warning IS GENERATED IN THE WRONG PLACE. Your argument 
|> seems to be "but that's the place it is generated" is a total 
|> non-argument. It's just stating a fact, and it's exactly that fact that 
|> causes the BUG IN GCC.

Then we have to agree to disagree.  Signed/unsigned mixups are just too
easy to get wrong in C, and a constant source of security bugs.

|> > |> Trivial example:
|> > |> 
|> > |> 	int x[2][2];
|> > |> 
|> > |> 	int main(int argc, char **argv)
|> > |> 	{
|> > |> 		return x[1][-1];
|> > |> 	}
|> > |> 
|> > |> 
|> > |> the above is actually a well-defined C program, and 100%
|> > |> standards-conforming ("strictly conforming").
|> > 
|> > This isn't as trivial as it seems.  Look in comp.std.c for recent
|> > discussions on this topic (out-of-array references). 
|> 
|> It is as trivial as it seems, and this is _not_ an out-of-array reference.  

It is out of the x[1] array, *if* you consider it an object of its own
(whether this is the right viewpoint is the point of the issue).

|> So if they argued about this on comp.std.c, they either had clueless
|> people there (in addition to the ones that obviously aren't ;), _or_ you
|> misunderstood the argument.

I'd guess that the language experts on comp.std.c are more clueful about
the C standard than both you and me.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Richard B. Johnson]

On Tue, 25 Feb 2003, Linus Torvalds wrote:

> 
> On Tue, 25 Feb 2003, Andreas Schwab wrote:
> > |> 
> > |> The point is that the compiler should see that the run-time value of i is 
> > |> _obviously_never_negative_ and as such the warning is total and utter 
> > |> crap.
> > 
> > This requires a complete analysis of the loop body, which means that the
> > warning must be moved down from the front end (the common type of the
> > operands only depends on the type of the operands, not of any current
> > value of the expressions).
> 
> So? Gcc does that anyway. _Any_ good compiler has to.
> 
> And if the compiler isn't good enough to do it, then the compiler 
> shouldn't be warning about something that it hasn't got a clue about.
> 
> > |>    and anybody who writes 'array[5UL]' is considered a stupid git and a 
> > |>    geek. Face it.
> > 
> > But array[-1] is wrong.  An array can never have a negative index (I'm
> > *not* talking about pointers).
> 
> You're wrong.
> 
> Yes, when declaring an array, you cannot use "array[-1]". But that's not 
> because the thing is unsigned: the standard says that the array 
> declaration has to be a "integer value larger than zero". It is not 
> unsigned: it's _positive_.
> 
> However, in _indexing_ an array (as opposed to declaring it), "array[-1]" 
> is indeed perfectly fine, and is defined by the C language to be exactly 
> the same as "*(array-1)". And negative values are perfectly fine, even for 
> arrays. Trivial example:
> 
> 	int x[2][2];
> 
> 	int main(int argc, char **argv)
> 	{
> 		return x[1][-1];
> 	}
> 
> 
> the above is actually a well-defined C program, and 100%
> standards-conforming ("strictly conforming").
> 
> 		Linus
> 

I'm glad you tackled that. I was going to write a response, but
backed off because I'm getting meek in my old age. I do wish to
add that, even in ix86 assembly language, indexing by using a
register and a memory oprand is a signed displacement, i.e,

here:	.byte	0
foo:	.byte	0

        movl	$0xffffffff, %ebx
	movl	foo,(%ebx), %eax

... accesses a memory location one byte before label foo, i.e.,
foo[-1] or here[0].  This means that with a N-bit addressing,
indexed addressing cannot access more than 2^(N-1) memory locations.

Something to consider when trying to access large arrays. For
instance, if an operating system were to allow flat-mode access from
0x00000000 to 0xffffffff memory addresses in 32-bit user-mode, could
you actually use all that address space in 'C'? 

Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Linus Torvalds]

On Mon, 24 Feb 2003, Andreas Schwab wrote:
> |> 
> |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> How can you distinguish that from other occurrences of (int)<(size_t)?

Which is indeed my point. If you cannot distinguish it from incorrect
uses, you shouldn't be warnign the user, because the compiler obviously
doesn't know enough to make a sufficiently educated guess.

That said, a good compiler _can_ make a good warning. But to do so, you 
have to actually do value analysis, instead of just blindly warning about 
code that is obviously correct to a human.

Until gcc does sufficient value analysis, that signed warning is annoying,
worthless and a damn pain in the ass.

			Linus

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Jakub Jelinek]

On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
> Linus Torvalds <torvalds@transmeta.com> writes:
> 
> |> Does gcc still warn about things like
> |> 
> |> 	#define COUNT (sizeof(array)/sizeof(element))
> |> 
> |> 	int i;
> |> 	for (i = 0; i < COUNT; i++)
> |> 		...
> |> 
> |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> |> unsigned)?
> |> 
> |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> How can you distinguish that from other occurrences of (int)<(size_t)?

Value range propagation pass, then warn?

	Jakub

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[John Alvord]

On Mon, 24 Feb 2003 17:39:34 -0500, Jakub Jelinek <jakub@redhat.com>
wrote:

>On Mon, Feb 24, 2003 at 10:35:24PM +0100, Andreas Schwab wrote:
>> Linus Torvalds <torvalds@transmeta.com> writes:
>> 
>> |> Does gcc still warn about things like
>> |> 
>> |> 	#define COUNT (sizeof(array)/sizeof(element))
>> |> 
>> |> 	int i;
>> |> 	for (i = 0; i < COUNT; i++)
>> |> 		...
>> |> 
>> |> where COUNT is obviously unsigned (because sizeof is size_t and thus 
>> |> unsigned)?
>> |> 
>> |> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
>> 
>> How can you distinguish that from other occurrences of (int)<(size_t)?
>
>Value range propagation pass, then warn?

I know it is stupid/unnecessary etc, but you could do

#if COUNT > INT_MAX
#error you idiot... 
#endif

	int i;
	for(i =0; i < (int)COUNT; i++)
	...

where the #if was placed in whatever header COUNT was defined.

and have safe code with no runtime overhead and looking only mildly
idiotic.

john alvord

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Adrian Bunk]

On Mon, Feb 24, 2003 at 01:02:39PM -0800, Linus Torvalds wrote:
>...
> Does gcc still warn about things like
> 
> 	#define COUNT (sizeof(array)/sizeof(element))
> 
> 	int i;
> 	for (i = 0; i < COUNT; i++)
> 		...
> 
> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> unsigned)?
> 
> Gcc used to complain about things like that, which is a FUCKING DISASTER. 
> 
> Any compiler that complains about the above should be shot in the head, 
> and the warning should be killed.


In the program below -Wall doesn't warn in neither 2.95 nor 3.2. You
need -Wsign-compare to get a warning. IMHO a warning is useful in this
example, some people might wrongly assume that the "Hello, world!" would
be printed more than once.


<--  snip  -->

#include <stdio.h>

int array[] = {1, 2, 3, 4, 5};

#define COUNT (sizeof(array)/sizeof(array[0]))

int main()
{
  int i;

  for (i = 0; i < COUNT; i++)
    {
      i -= 2;
      printf("Hello, world!\n");
    }

  return(0);
}

<--  snip  -->

> 		Linus

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Richard Henderson]

On Mon, Feb 24, 2003 at 01:02:39PM -0800, Linus Torvalds wrote:
> Does gcc still warn about things like
> 
> 	#define COUNT (sizeof(array)/sizeof(element))
> 
> 	int i;
> 	for (i = 0; i < COUNT; i++)
> 		...
> 
> where COUNT is obviously unsigned (because sizeof is size_t and thus 
> unsigned)?

Yes.  We don't do complete value-range propagation to figure
out if a warning is needed.  We only look at the comparison
itself and note that one of the arguments changed signedness
due to forced promotions.


r~

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Arnd Bergmann]

Richard B. Johnson wrote:

> I think you must keep these warnings in! There are many bugs
> that these uncover uncluding loops that don't terminate correctly
> but seem to work for "most all" cases. These are the hard-to-find
> bugs that hit you six months after release.

This was my change. Obviously the warning is a good idea in general,
but I don't see the point of scrolling through hundreds of lines
with the same warning in someone else's code. I actually plan to fix
these warnings in arch/s390 and drivers/s390 as well as include/
and make the s390 kernel compile with -Werror, but the rest looks 
more like a task for the Janitors. Note that before gcc-3.3, 
-Wsign-compare has not been part of -Wall.

        Arnd <><

Re: [PATCH] s390 (7/13): gcc 3.3 adaptions.

[Richard B. Johnson]

On Mon, 24 Feb 2003, Arnd Bergmann wrote:

> Richard B. Johnson wrote:
> 
> > I think you must keep these warnings in! There are many bugs
> > that these uncover uncluding loops that don't terminate correctly
> > but seem to work for "most all" cases. These are the hard-to-find
> > bugs that hit you six months after release.
> 
> This was my change. Obviously the warning is a good idea in general,
> but I don't see the point of scrolling through hundreds of lines
> with the same warning in someone else's code. I actually plan to fix
> these warnings in arch/s390 and drivers/s390 as well as include/
> and make the s390 kernel compile with -Werror, but the rest looks 
> more like a task for the Janitors. Note that before gcc-3.3, 
> -Wsign-compare has not been part of -Wall.
> 

Yep. There are a lot of things, not part of -Wall. To find 'need coffee'
errors, I use:

	-Wall -Werror -Wstrict-prototypes -Wwrite-strings -Wshadow \
	-Wsign-compare -Wno-trigraphs

        plus...
	-fno-builtin -fno-common -fomit-frame-pointer.

I also use -march=1486 so it will boot. Some machines don't like
`cmov` and other stuff the compiler may emit. 


Cheers,
Dick Johnson
Penguin : Linux version 2.4.18 on an i686 machine (797.90 BogoMips).
Why is the government concerned about the lunatic fringe? Think about it.