WARN_ON() which sometimes sucks

WARN_ON() which sometimes sucks

[Alexey Dobriyan]

It started when I tried to write

	WARN_ON(m->seq_ops_allocated);

in today's "[PATCH] single_open/seq_release leak diagnostics¹.
Suprisingly compiler told me piss off with:

	  CC      fs/seq_file.o
	fs/seq_file.c: In function 'seq_release':
	fs/seq_file.c:285: error: 'typeof' applied to a bit-field

Well, duh! Earlier versions of WARN_ON allowed that until commit
684f978347deb42d180373ac4c427f82ef963171² which added typeof().

OK, nobody noticed that WARN_ON(bitfield) stopped working. But I
question the rationale of that commit:

	Letting WARN_ON/WARN_ON_ONCE return the condition means that you could
	do

	if (WARN_ON(blah)) {
	        handle_impossible_case
	}

	Rather than

	if (unlikely(blah)) {
	        WARN_ON(1)
	        handle_impossible_case
	}

I think that second case is more clear and immediately understandable.
If folks agree, I'll send revert so that WARN_ON(var), WARN_ON(ptr),
WARN_ON(bitfield) and WARN_ON(condition) work as expected.

¹ http://marc.info/?l=linux-kernel&m=118588389612083&w=2
²

tree db9025d8c6b267565c7110e09b16193957186a48
parent 6299a2dec89d22940e36832f15c0addfb77e6910
author Herbert Xu <herbert@gondor.apana.org.au> 1159520346 -0700
committer Linus Torvalds <torvalds@g5.osdl.org> 1159546686 -0700

[PATCH] Let WARN_ON/WARN_ON_ONCE return the condition

Letting WARN_ON/WARN_ON_ONCE return the condition means that you could do

if (WARN_ON(blah)) {
	handle_impossible_case
}

Rather than

if (unlikely(blah)) {
	WARN_ON(1)
	handle_impossible_case
}

I checked all the newly added WARN_ON_ONCE users and none of them test the
return status so we can still change it.

[akpm@osdl.org: warning fix]
[akpm@osdl.org: build fix]
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Re: WARN_ON() which sometimes sucks

[Al Viro]

On Tue, Jul 31, 2007 at 07:55:27PM +0400, Alexey Dobriyan wrote:
> It started when I tried to write
> 
> 	WARN_ON(m->seq_ops_allocated);
> 
> in today's "[PATCH] single_open/seq_release leak diagnostics?.
> Suprisingly compiler told me piss off with:
> 
> 	  CC      fs/seq_file.o
> 	fs/seq_file.c: In function 'seq_release':
> 	fs/seq_file.c:285: error: 'typeof' applied to a bit-field
> 
> Well, duh! Earlier versions of WARN_ON allowed that until commit
> 684f978347deb42d180373ac4c427f82ef963171? which added typeof().
> 
> OK, nobody noticed that WARN_ON(bitfield) stopped working. But I
> question the rationale of that commit:

Actually, the real problem is different - WTF do we need that typeof
anyway?
	int ret_warn_on = !!(condition);
	[same as now]
will work just fine...

Re: WARN_ON() which sometimes sucks

[Paul Mackerras]

Al Viro writes:

> Actually, the real problem is different - WTF do we need that typeof
> anyway?
> 	int ret_warn_on = !!(condition);
> 	[same as now]
> will work just fine...

It will mean more code on architectures which have a
conditional-trap-on-nonzero instruction, such as powerpc, since the
compiler will generate instructions to evaluate !!x.  But I don't see
any reason why ret_warn_on couldn't be a long.

Paul.

Re: WARN_ON() which sometimes sucks

[Linus Torvalds]

On Wed, 1 Aug 2007, Paul Mackerras wrote:
>
> It will mean more code on architectures which have a
> conditional-trap-on-nonzero instruction, such as powerpc, since the
> compiler will generate instructions to evaluate !!x.  But I don't see
> any reason why ret_warn_on couldn't be a long.

Umm. The WARN_ON() might actually get a "long long" value for all we know. 
Ie it's perfectly possible that the WARN_ON might look like

	/* Must not have high bits on */
	WARN_ON(offset & 0xffffffff00000000);

which on a 32-bit pcc would apparently do the wrong thing entirely as it 
stands now. No?

I think I'll commit the !!(x) version, and you guys can try to figure out 
what the right thing is long-term. For all I know, the proper solution is 
to just revert the whole mess, and *not* make WARN_ON() return a value at 
all, since that seems to be the fundamental mistake here.

			Linus

Re: WARN_ON() which sometimes sucks

[Paul Mackerras]

Linus Torvalds writes:

> Umm. The WARN_ON() might actually get a "long long" value for all we know. 
> Ie it's perfectly possible that the WARN_ON might look like
> 
> 	/* Must not have high bits on */
> 	WARN_ON(offset & 0xffffffff00000000);
> 
> which on a 32-bit pcc would apparently do the wrong thing entirely as it 
> stands now. No?

Actually, because of the typeof in the powerpc WARN_ON, I think it
would fail to build since we'd be passing a long long value to an
inline asm, or at least I hope it would fail to build. :)

But your criticism is correct with regard to the powerpc BUG_ON, and
you're correct that a long wouldn't be sufficient if someone passes in
a long long.  Oh well.  I guess we just wear the extra two
instructions.

Paul.

Re: WARN_ON() which sometimes sucks

[Joe Korty]

On Wed, Aug 01, 2007 at 02:20:48PM +1000, Paul Mackerras wrote:
> Linus Torvalds writes:
> 
> > Umm. The WARN_ON() might actually get a "long long" value for all we know. 
> > Ie it's perfectly possible that the WARN_ON might look like
> > 
> > 	/* Must not have high bits on */
> > 	WARN_ON(offset & 0xffffffff00000000);
> > 
> > which on a 32-bit pcc would apparently do the wrong thing entirely as it 
> > stands now. No?
> 
> Actually, because of the typeof in the powerpc WARN_ON, I think it
> would fail to build since we'd be passing a long long value to an
> inline asm, or at least I hope it would fail to build. :)


Turning the condition into an integer should work ...

	#define NEW_WARN_ON(x) OLD_WARN_ON(!!(x))

Regards,
Joe