IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[John Cherry]

drivers/net/ne.c:168: warning: unused variable `irq'
drivers/scsi/imm.c:1146: warning: `ports' might be used uninitialized in this function
drivers/scsi/ppa.c:1006: warning: `ports' might be used uninitialized in this function

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[viro]

On Fri, Feb 06, 2004 at 03:22:35AM -0800, John Cherry wrote:
> drivers/net/ne.c:168: warning: unused variable `irq'
> drivers/scsi/imm.c:1146: warning: `ports' might be used uninitialized in this function
> drivers/scsi/ppa.c:1006: warning: `ports' might be used uninitialized in this function

Sigh...  we have
        switch (dev->mode) {
        case IMM_NIBBLE:
        case IMM_PS2:
                ports = 3;
                break;
        case IMM_EPP_8:
        case IMM_EPP_16:
        case IMM_EPP_32:
                ports = 8;
                break;
        default:        /* Never gets here */
                BUG();
        }
	...
	/* use the value of 'ports' */

IOW, gcc doesn't realize that we never return from BUG().  AFAICS, it
should.  Some changes of __volatile__ semantics?

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Linus Torvalds]

On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> IOW, gcc doesn't realize that we never return from BUG().  AFAICS, it
> should.  Some changes of __volatile__ semantics?

Thsrs is no way to tell gcc that an inline asm doesn't return. The only
way to do it would be to add something like a "for (;;);" (that gcc will
actually generate real code for) inside the BUG() macro, but I'd hate to 
do that.

Better to just initialize the variable to a default value and avoid the 
warning for now.

		Linus

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[viro]

On Fri, Feb 06, 2004 at 08:52:00AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> > 
> > IOW, gcc doesn't realize that we never return from BUG().  AFAICS, it
> > should.  Some changes of __volatile__ semantics?
> 
> Thsrs is no way to tell gcc that an inline asm doesn't return. The only
> way to do it would be to add something like a "for (;;);" (that gcc will
> actually generate real code for) inside the BUG() macro, but I'd hate to 
> do that.

Umm...  How about

static inline void BUG() __attribute__((noreturn));

static inline void BUG(void)
{
	__asm__ ....
}
 
> Better to just initialize the variable to a default value and avoid the 
> warning for now.

Alternatively, we can just turn the damn thing into
	if (dev->mode == IMM_NIBBLE || dev->mode = IMM_PS2)
		ports = 3;
	else
		ports = 8;
and be done with that...

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Petr Vandrovec]

On Fri, Feb 06, 2004 at 06:22:08PM +0000, viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> Umm...  How about
> 
> static inline void BUG() __attribute__((noreturn));
> 
> static inline void BUG(void)
> {
> 	__asm__ ....
> }

Unfortunately gcc thinks that it knows better than you whether function
can return or not. I attempted to use noreturn attribute in couple of
my projects, but it is useless - unless you terminate your function
with for(;;) or with call to _exit(), it won't work.

vana:~# more m.c
static inline void BUG() __attribute__((noreturn));

static inline void BUG(void) {
        __asm__ ("1: jmp 1b");
}

void main(void) {
        BUG();
}

vana:~# gcc -W -Wall m.c
m.c:7: warning: return type of `main' is not `int'
m.c: In function `BUG':
m.c:3: warning: `noreturn' function does return
vana:~#

(gcc version 3.3.3 20040125 (prerelease) (Debian))

> > Better to just initialize the variable to a default value and avoid the 
> > warning for now.
> 
> Alternatively, we can just turn the damn thing into
> 	if (dev->mode == IMM_NIBBLE || dev->mode = IMM_PS2)
> 		ports = 3;
> 	else
> 		ports = 8;
> and be done with that...

Yes.
				Petr Vandrovec

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Linus Torvalds]

On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> 
> Umm...  How about
> 
> static inline void BUG() __attribute__((noreturn));
> 
> static inline void BUG(void)
> {
> 	__asm__ ....
> }

Did you try that? Last time I tried, gcc would complain every time it saw 
the thing about "noreturn function does return".

		Linus

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[viro]

On Fri, Feb 06, 2004 at 10:42:00AM -0800, Linus Torvalds wrote:
> 
> 
> 
> On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
> > 
> > Umm...  How about
> > 
> > static inline void BUG() __attribute__((noreturn));
> > 
> > static inline void BUG(void)
> > {
> > 	__asm__ ....
> > }
> 
> Did you try that? Last time I tried, gcc would complain every time it saw 
> the thing about "noreturn function does return".

So it does ;-/

Anyway, in both of those cases BUG() is gratitious.  What that code tries
to do is "if we decided to use one of the old modes, we'll need 3 ports;
otherwise we should claim 8".  So simple if() would be enough.  I'll send
a patch in a few.

It would be nice to have gcc understand that BUG() is a sink.  Oh, well...

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[viro]

On Fri, Feb 06, 2004 at 06:47:07PM +0000, viro@parcelfarce.linux.theplanet.co.uk wrote:
> Anyway, in both of those cases BUG() is gratitious.  What that code tries
> to do is "if we decided to use one of the old modes, we'll need 3 ports;
> otherwise we should claim 8".  So simple if() would be enough.  I'll send
> a patch in a few.

Please, apply

diff -urN RC2-bk1-base/drivers/scsi/imm.c RC2-bk1-current/drivers/scsi/imm.c
--- RC2-bk1-base/drivers/scsi/imm.c	Thu Feb  5 18:48:49 2004
+++ RC2-bk1-current/drivers/scsi/imm.c	Fri Feb  6 14:07:08 2004
@@ -1208,19 +1208,10 @@
 		goto out1;
 
 	/* now the glue ... */
-	switch (dev->mode) {
-	case IMM_NIBBLE:
-	case IMM_PS2:
+	if (dev->mode == IMM_NIBBLE || dev->mode == IMM_PS2)
 		ports = 3;
-		break;
-	case IMM_EPP_8:
-	case IMM_EPP_16:
-	case IMM_EPP_32:
+	else
 		ports = 8;
-		break;
-	default:	/* Never gets here */
-		BUG();
-	}
 
 	INIT_WORK(&dev->imm_tq, imm_interrupt, dev);
 
diff -urN RC2-bk1-base/drivers/scsi/ppa.c RC2-bk1-current/drivers/scsi/ppa.c
--- RC2-bk1-base/drivers/scsi/ppa.c	Thu Feb  5 18:48:57 2004
+++ RC2-bk1-current/drivers/scsi/ppa.c	Fri Feb  6 14:07:50 2004
@@ -1069,21 +1069,10 @@
 		goto out1;
 
 	/* now the glue ... */
-	switch (dev->mode) {
-	case PPA_NIBBLE:
+	if (dev->mode == PPA_NIBBLE || dev->mode == PPA_PS2)
 		ports = 3;
-		break;
-	case PPA_PS2:
-		ports = 3;
-		break;
-	case PPA_EPP_8:
-	case PPA_EPP_16:
-	case PPA_EPP_32:
+	else
 		ports = 8;
-		break;
-	default:	/* Never gets here */
-		BUG();
-	}
 
 	INIT_WORK(&dev->ppa_tq, ppa_interrupt, dev);
 

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Jon Foster]

Linus wrote:
> On Fri, 6 Feb 2004 viro wrote:
>>
>> IOW, gcc doesn't realize that we never return from BUG(). AFAICS, it
>> should. Some changes of __volatile__ semantics?
> 
> Thsrs is no way to tell gcc that an inline asm doesn't return. The only
> way to do it would be to add something like a "for (;;);" (that gcc will
> actually generate real code for) inside the BUG() macro, but I'd hate to
> do that.

Why would you hate to do that?  Because of the extra code this generates?
Or because it's a hack (we should just be able to tell GCC that the inline
assembly never returns)?

Although the loop does generate real code, GCC is currently generating real
code to handle what happens after BUG() returns.  The two pretty much cancel
each other out - they're both basically a jump (possibly with a nop for
alignment).  This is extremely non-obvious (at least to me), since the code
to handle BUG() returning is implicit, whereas the for(;;); is explicit.

I've just done some benchmarks (using a vendor 2.4 kernel) and the for(;;);
change actually reduced the kernel size.  The change was a negligible
amount, almost lost in the noise, but at least the size didn't go up.  See
my recent l-k post "Re: [Compile Regression in 2.4.25-pre8][PATCH 37/42]".

I agree with Viro that the best solution would be if there was some way
to tell GCC that the inline assembly doesn't return - probably by attaching
__attribute__((noreturn)) to it.

Kind regards,

Jon

> 
> Better to just initialize the variable to a default value and avoid the
> warning for now.
> 
> Linus

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[viro]

On Sun, Feb 08, 2004 at 01:00:06AM +0000, Jon Foster wrote:
> I agree with Viro that the best solution would be if there was some way
> to tell GCC that the inline assembly doesn't return - probably by attaching
> __attribute__((noreturn)) to it.

That __attribute__ would have to go with __asm__ - it should tell gcc
that insn has no successors.  And that would be a PITA to deal with
- gcc parser is not a thing of beauty as it is, so changing it would
not be fun.

Easier to express that as an asm constraint - then it will have no impact
on the high-level parser and constraint parser is simple enough...

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Dick Streefland]

Linus Torvalds <torvalds@osdl.org> wrote:
| On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
| > 
| > Umm...  How about
| > 
| > static inline void BUG() __attribute__((noreturn));
| > 
| > static inline void BUG(void)
| > {
| > 	__asm__ ....
| > }
| 
| Did you try that? Last time I tried, gcc would complain every time it saw 
| the thing about "noreturn function does return".

So, make sure it won't return ;-)

static inline void BUG(void)
{
        __asm__ ("1: jmp 1b");
        BUG();
}

-- 
Dick Streefland                      ////                      Altium BV
dick.streefland@altium.nl           (@ @)          http://www.altium.com
--------------------------------oOO--(_)--OOo---------------------------

Re: IA32 (2.6.2 - 2004-02-05.22.30) - 3 New warnings (gcc 3.2.2)

[Randy.Dunlap]

On Tue, 10 Feb 2004 13:32:45 -0000 spam@altium.nl (Dick Streefland) wrote:

| Linus Torvalds <torvalds@osdl.org> wrote:
| | On Fri, 6 Feb 2004 viro@parcelfarce.linux.theplanet.co.uk wrote:
| | > 
| | > Umm...  How about
| | > 
| | > static inline void BUG() __attribute__((noreturn));
| | > 
| | > static inline void BUG(void)
| | > {
| | > 	__asm__ ....
| | > }
| | 
| | Did you try that? Last time I tried, gcc would complain every time it saw 
| | the thing about "noreturn function does return".
| 
| So, make sure it won't return ;-)
| 
| static inline void BUG(void)
| {
|         __asm__ ("1: jmp 1b");
|         BUG();
| }

doesn't that sorta miss the BUG output?

--
~Randy
kernel-janitors project:  http://janitor.kernelnewbies.org/