From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten
Date: Wed, 02 Dec 2009 15:27:45 UTC
Message-ID: <fa.iJ9gA2VhV/B/eOtyNrnuFJzMRNA@ifi.uio.no>

On Wed, 2 Dec 2009, tip-bot for Jan Beulich wrote:
>
> locking, x86: Slightly shorten __ticket_spin_trylock()

NAK. This is horrible crap. Don't ever send me this kind of broken patch
again.

> -	int new;
> +	union { int i; bool b; } new;

No thank you. This is total bogosity.

You have zero idea what type "bool" is, do you? It can well be "int", it
can be "char", it can be some compiler-internal type ("_Bool"). You have
no idea what size it is.

And maybe it _is_ just a byte. But even if it is, using 'bool' here is
wrong. The fact is, bool has magic semantic properties outside of sizing.
You can't mix it with inline asm, because you simply don't know what the
compiler rules for 'bool' are.

For example, maybe the rules are that it's always passed as an integer,
and is always guaranteed to have the values 0/1. So even if 'sizeof'
returns 1, that doesn't actually mean that you can necessarily pass it
around as a char - it only means that it will take one byte in a structure
(except that bool arrays might be packed, I think).

In other words, the semantics of 'bool' are such that you have no clue
what the actual ABI for 'bool' is. You cannot mix this with asm.

Secondly, the notion of using a union here is just totally broken. There's
no point to it, and it just makes the code look horrible.

So if you want to do this, then just keep 'new' as an int, and make sure
that the function returns a 'char'. No games with 'bool' which is badly
defined, no games with unions.

And please do make sure that it actually doesn't deprove code at the
callers too.

				Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten
Date: Wed, 02 Dec 2009 15:34:52 UTC
Message-ID: <fa.OurMjvE+FjOoLtZzIx5xDB4VUGo@ifi.uio.no>

On Wed, 2 Dec 2009, Jan Beulich wrote:

> >>> Ingo Molnar <mingo@elte.hu> 02.12.09 15:21 >>>
> 792a99a9 <_raw_spin_lock>:
> ...
> >792a99f3:	89 d8                	mov    %ebx,%eax
> >792a99f5:	ff 15 d0 6c f2 79    	call   *0x79f26cd0
> >792a99fb:	85 c0                	test   %eax,%eax
> ...
> >792a9a2e:	89 f8                	mov    %edi,%eax
> >792a9a30:	ff 15 d0 6c f2 79    	call   *0x79f26cd0
> >792a9a36:	85 c0                	test   %eax,%eax
>
> Assuming that these are the calls to __raw_spin_trylock, it is clear that
> the generated code isn't what we want: It should be test %al, %al in
> both cases.

See my previous email.  Using 'bool' was a mistake. It's _always_ a
mistake.

'bool' basically says that the compiler can assume magic things, and
compile the thing to be anything it wants that is convenient for it.
Put another way, 'bool' has a magic API. For all you know, the rule might
even be that a 'bool' return value is always returned in a flag register
(ok, on x86 that would be _very_ inconvenient, but it's _possible_).

So the calling convention could have been

	call <bool-returning-function>
	jne ..	// jump if it returned non-zero

because the rule about bool is that it is just one bit of information,
but exactly _how_ that bit is done is totally up to the compiler ABI.

In this case, the rule gcc implements on x86 is probably just "we pass it
around as an 'int' that contains 0 or 1". So the code would work if you
had left in the 'movzbl'.

End result: don't use bool.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten
Date: Wed, 02 Dec 2009 16:57:57 UTC
Message-ID: <fa.HJm8Upo//wQAP4xhp2Ir7SZ9PMw@ifi.uio.no>

On Wed, 2 Dec 2009, Jan Beulich wrote:
>
> This just can't be the case: In order for two compilers to be
> interoperable, the processor specific ABI has to define the handling of
> bool, just like it has to for any other data type.

You are full of crap.

The fact is, compilers are _not_ interoperable in general, and ABI's are
often compiler-specific. Look at MSVC vs gcc on x86 for just a really
obvious and trivial example. The fact is, on x86-unix we simply don't
have any real ABI at all, and gcc picks whatever randon choices it has.

So stop making excuses. Just admit that 'bool' was wrong, and you made a
fundamental mistake in choosing it. The fact is, the compiler can do
whatever the hell it does, which is not necessarily sensible with any
other type. 'bool' really is special.

			Linus

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten
Date: Wed, 02 Dec 2009 17:06:26 UTC
Message-ID: <fa.mbg1TrWLDfeEU2f/NflkeQKuRG0@ifi.uio.no>

On Wed, 2 Dec 2009, Linus Torvalds wrote:
>
> So stop making excuses. Just admit that 'bool' was wrong, and you made a
> fundamental mistake in choosing it. The fact is, the compiler can do
> whatever the hell it does, which is not necessarily sensible with any
> other type. 'bool' really is special.

Btw, even if gcc just treats 'bool' as 'char' (which is the sane thing to
do on x86 anyway - I don't see why it should ever do anything else), we
actually mess up in the kernel and make that type confusion even worse.

Do this:

	git grep typedef.*bool

and then do this:

	git grep 'define bool'

and then sit down and cry silently.

		Linus

From: "H. Peter Anvin" <hpa@zytor.com>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten  
	__ticket_spin_trylock()
Date: Wed, 02 Dec 2009 17:25:11 UTC
Message-ID: <fa.xns+H7mtEW2oXE5zlnxF+ospK5g@ifi.uio.no>

On 12/02/2009 09:05 AM, Linus Torvalds wrote:
>
> Btw, even if gcc just treats 'bool' as 'char' (which is the sane thing to
> do on x86 anyway - I don't see why it should ever do anything else), we
> actually mess up in the kernel and make that type confusion even worse.
>

For what it's worth, the gcc ABI for i386-Linux treats _Bool (bool) as
follows:

When in memory, except stack slots:

	sizeof(_Bool) = 1
	0 is false, 1 is true, any other value is *undefined behavior*.

When in registers, or in a stack slot:

	Registers, and stack slots, are always 4 bytes
	0 is false, 1 is true, any other value is *undefined behavior*.

All of which is well-defined.  However, it also only applies to values
in memory, or values passed across function boundaries in registers[1]
-- anything else is *by definition outside the scope of the ABI* and
therefore the compiler can legitimately do whatever is appropriate at
any point in time.

As such, I would agree with Linus in that using an u8 is the right thing
to be handed through the inline asm boundary -- if the compiler needs to
extend it, it will, and if it doesn't, there is no penalty.  Similarly,
a bool can be cast to u8 without penalty.

	-hpa

--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

From: Linus Torvalds <torvalds@linux-foundation.org>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten
Date: Wed, 02 Dec 2009 17:49:14 UTC
Message-ID: <fa.7Hx12AjurWuYhXSngaXzPhnbUq0@ifi.uio.no>

On Wed, 2 Dec 2009, H. Peter Anvin wrote:
>
> For what it's worth, the gcc ABI for i386-Linux treats _Bool (bool) as
> follows:
>
> When in memory, except stack slots:
>
> 	sizeof(_Bool) = 1
> 	0 is false, 1 is true, any other value is *undefined behavior*.
>
> When in registers, or in a stack slot:
>
> 	Registers, and stack slots, are always 4 bytes
> 	0 is false, 1 is true, any other value is *undefined behavior*.

Hmm. Odd. I just checked:

	_Bool myfunction(char val)
	{
		return val;
	}

and compiling it with

	gcc -O2 -S -m32 -mregparm=3 -fomit-frame-pointer t.c

I get

	myfunction:
		testb	%al, %al
		setne	%al
		ret

which only sets the low 8 bits. So my gcc actually seems to think that
_Bool is just 8 bits, at least for return values, and then upper 24 bits
are undefined. It also generates 'testb' for a test of a return value.

So it so happens that I think Jan's patch would have worked - except for
the PV_OPS mess. _Bool does act like a 'char' on x86 at least with gcc. I
still think that it's fundamentally wrong to use 'bool' because of how
subtly it can act.

			Linus

From: "H. Peter Anvin" <hpa@zytor.com>
Newsgroups: fa.linux.kernel
Subject: Re: [tip:core/locking] locking, x86: Slightly shorten  
	__ticket_spin_trylock()
Date: Wed, 02 Dec 2009 17:59:35 UTC
Message-ID: <fa.yTgTFoNF3kyFp3DopDgMBWp3zK0@ifi.uio.no>

On 12/02/2009 09:48 AM, Linus Torvalds wrote:
>
> Hmm. Odd. I just checked:
>
> 	_Bool myfunction(char val)
> 	{
> 		return val;
> 	}
>
> and compiling it with
>
> 	gcc -O2 -S -m32 -mregparm=3 -fomit-frame-pointer t.c
>
> I get
>
> 	myfunction:
> 		testb	%al, %al
> 		setne	%al
> 		ret
>
> which only sets the low 8 bits. So my gcc actually seems to think that
> _Bool is just 8 bits, at least for return values, and then upper 24 bits
> are undefined. It also generates 'testb' for a test of a return value.
>

Damn.  I stand corrected :-/  I just tested it on x86-64, and gcc 4.4.1
actually *violates the documented ABI* for x86-64.

> So it so happens that I think Jan's patch would have worked - except for
> the PV_OPS mess. _Bool does act like a 'char' on x86 at least with gcc. I
> still think that it's fundamentally wrong to use 'bool' because of how
> subtly it can act.

I personally think using "bool" for C values is a good thing -- people
have a very nasty tendency to come up with the clever idea of "oh, there
is this flag which is 'int'... well, in this special case let's set it
to -1 or 2", and of course there is absolutely no way to know, globally,
that this value once in a blue moon gets set to a bizarre value.  I have
seen this a number of times in the kernel.  It doesn't mean one should
pass it to assembly code.

	-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.