Re: [mingo@elte.hu: [git pull] headers_check fixes]

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Linus Torvalds]

On Tue, 27 Jan 2009, Ingo Molnar wrote:
> 
> Should i perhaps not bother with the stuff below? Cannot turn off 
> CONFIG_HEADERS_CHECK in my builds because it can cause build failures.

I really hate the patch. I think it's fundamentally flawed. I hate scripts 
that test for things that are readable, and encourage people to then write 
crap instead.

The thing is, the headers_check stuff is just wrong if it causes these 
things, and I'd rather just turn it off.

If those 

	#ifdef CONFIG_XYZ

things result in problems, then we should just make the rule be that we 
turn that kind of string into

	#if 0

automatically when exporting the kernel headers. IOW, just about 
_anything_ that headers_check complains about automatically is something 
that should just be _fixed_ automatically at header install time rather 
than make the code harder to read.

So I think it makes our headers worse. Code like

	> +#ifdef __KERNEL__
	> +# ifdef CONFIG_X86_BSWAP
	> +# define __X86_BSWAP  
	> +# endif /* CONFIG_X86_BSWAP */
	> +#endif /* __KERNEL__ */

just doesn't make sense. It doesn't make sense _inside_ the kernel, and it 
doesn't make sense _outside_ it either.

As far as I can tell, the header install script could literally just do 
something like run 'sed' over the headers as it installs them, and do 
something like

	sed 's/\<CONFIG_[A-Z0-9_]*\>/__kernel_only__/g'

which I realize is not really the complete/correct solution (ie you could 
write a nicer thing that does a better job), but my point here is that 
rather than have scripts that _whine_ about these kinds of trivial things 
and cause people to write less readable header files, we should just make 
sure that if we can recognize them so easily, we can just fix them 
instead.

End result: headers that don't suck.

If the damn headers-check isn't working for people, then let's turn the 
thing off, not make our code look worse.

There are parts of the patches that look fine, like moving __KERNEL__ 
checks around a bit, and changing <asm/types.h> to <linux/types.h> which 
looks correct _both_ in a kernel and in a user context, but I dislike the 
stupid parts.

			Linus

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> So I think it makes our headers worse. Code like
> 
> 	> +#ifdef __KERNEL__
> 	> +# ifdef CONFIG_X86_BSWAP
> 	> +# define __X86_BSWAP  
> 	> +# endif /* CONFIG_X86_BSWAP */
> 	> +#endif /* __KERNEL__ */
> 
> just doesn't make sense. It doesn't make sense _inside_ the kernel, and it 
> doesn't make sense _outside_ it either.
> 
> As far as I can tell, the header install script could literally just do 
> something like run 'sed' over the headers as it installs them, and do 
> something like
> 
> 	sed 's/\<CONFIG_[A-Z0-9_]*\>/__kernel_only__/g'
> 
> which I realize is not really the complete/correct solution (ie you could 
> write a nicer thing that does a better job), but my point here is that 
> rather than have scripts that _whine_ about these kinds of trivial things 
> and cause people to write less readable header files, we should just make 
> sure that if we can recognize them so easily, we can just fix them 
> instead.
> 

We already run the headers through unifdef, so this should be trivial to 
add.

The intent of headers_check is to try to catch people who put things 
that depend on CONFIG_* stuff in exported headers (which, as we have 
seen, have been too sadly common.)  If we declare that the export 
process will treat all CONFIG_* as undefined, we do lose some coverage 
but potentially end up with cleaner code.  Not sure which is worse...

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Linus Torvalds]

On Tue, 27 Jan 2009, H. Peter Anvin wrote:
>
> The intent of headers_check is to try to catch people who put things that
> depend on CONFIG_* stuff in exported headers (which, as we have seen, have
> been too sadly common.)  If we declare that the export process will treat all
> CONFIG_* as undefined, we do lose some coverage but potentially end up with
> cleaner code.  Not sure which is worse...

Do you think the "fix headers_check" patches spend lots of time analyzing 
things? I bet no. They just try to make the warning go away, so you don't 
actually end up with any more "coverage" anyway. Quite the reverse - 
instead of having a simple rule ("CONFIG_xyz options simply do not exist 
in user space"), you end up having ad-hoc hacks on a per-fix basis.

		Linus

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Do you think the "fix headers_check" patches spend lots of time analyzing 
> things? I bet no. They just try to make the warning go away, so you don't 
> actually end up with any more "coverage" anyway. Quite the reverse - 
> instead of having a simple rule ("CONFIG_xyz options simply do not exist 
> in user space"), you end up having ad-hoc hacks on a per-fix basis.
> 

This is probably true.  I think we should add this as one more of the 
preprocessing rules which we really should just do, as well as automatic 
mangling of integer types.

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Linus Torvalds]

On Tue, 27 Jan 2009, H. Peter Anvin wrote:

> Linus Torvalds wrote:
> > 
> > Do you think the "fix headers_check" patches spend lots of time analyzing
> > things? I bet no. They just try to make the warning go away, so you don't
> > actually end up with any more "coverage" anyway. Quite the reverse - instead
> > of having a simple rule ("CONFIG_xyz options simply do not exist in user
> > space"), you end up having ad-hoc hacks on a per-fix basis.
> > 
> 
> This is probably true.  I think we should add this as one more of the
> preprocessing rules which we really should just do, as well as automatic
> mangling of integer types.

Btw, the really scary thing is that I bet there are programs out there 
that "know" about kernel internals, and do things like

	#define CONFIG_SMP 1
	#define __KERNEL__ 1
	#include <asm/atomic.h>

in order to get the atomic helpers from the kernel, and using CONFIG_xyz 
markers to force the exact version they want.

And we will inevitably always end up breaking stuff like that. Nothing we 
can do about it - in the end, users can do infinitely odd things and know 
about our internals, and whatever changes we do will occasionally break 
some of the more incestuous code.

			Linus

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Linus Torvalds wrote:
>>>
>> This is probably true.  I think we should add this as one more of the
>> preprocessing rules which we really should just do, as well as automatic
>> mangling of integer types.
> 
> Btw, the really scary thing is that I bet there are programs out there 
> that "know" about kernel internals, and do things like
> 
> 	#define CONFIG_SMP 1
> 	#define __KERNEL__ 1
> 	#include <asm/atomic.h>
> 
> in order to get the atomic helpers from the kernel, and using CONFIG_xyz 
> markers to force the exact version they want.
> 
> And we will inevitably always end up breaking stuff like that. Nothing we 
> can do about it - in the end, users can do infinitely odd things and know 
> about our internals, and whatever changes we do will occasionally break 
> some of the more incestuous code.
> 

Yes, that's just PEBKAC.

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> There are parts of the patches that look fine, like moving __KERNEL__ 
> checks around a bit, and changing <asm/types.h> to <linux/types.h> which 
> looks correct _both_ in a kernel and in a user context, but I dislike 
> the stupid parts.

okay - that was the general thinking and this went through a few 
iterations already - not enough it appears. The CONFIG_* thing was 
something that looked somewhat dubious to me too - changing it to those 
random __foo symbols didnt seem like an improvement.

Jaswider, would you mind re-doing the tree filtering out the CONFIG_* 
changes? I'll go over the end result once more to make sure it has no 
changes that make the code look worse.

I still think what i expressed elsewhere in these threads on lkml: 
'exporting' 75,000 lines of random kernel headers to user-space is really 
stretching the term - kernel-space is unaware of it in 90% of the cases. 
"spilling our guts to user-space" would be a more fair description.

It would be much better if we exported _much_ less and reduced our 
cross-section to user-space. Also, the include/linux/Kbuild rules are all 
but transparent: it would also be nice if whatever we exported was be 
visible straight in the header itself, to make it obvious to people who 
modify/extend those files that those definitions are going to be exported 
to user-space.

Some __user_export tag on structures perhaps? I have no good ideas here - 
#ifdefs are ugly and tags obscure the purity of the code.

	Ingo

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Linus Torvalds]

On Wed, 28 Jan 2009, Ingo Molnar wrote:
> 
> It would be much better if we exported _much_ less and reduced our 
> cross-section to user-space. Also, the include/linux/Kbuild rules are all 
> but transparent: it would also be nice if whatever we exported was be 
> visible straight in the header itself, to make it obvious to people who 
> modify/extend those files that those definitions are going to be exported 
> to user-space.
> 
> Some __user_export tag on structures perhaps? I have no good ideas here - 
> #ifdefs are ugly and tags obscure the purity of the code.

I agree, and we probably could do so with some sparse extension (just make 
the rule be that in order to make a __user pointer, the base type needs to 
have been tagged with __user_visible). So we _could_ do something like 
that, and have a sane checking model where sparse would warn if it sees a 
user pointer of something that wasn't marked as being a user-visible type.

But I doubt it's really worth it. A lot of the usage cases for users end 
up being about constants etc that we really can't check sanely and 
automatically. And some historical user-space usage is literally just 
about user space finding the kernel headers really helpful (ie using the 
list.h headers for user-space lists, or the inline asms for atomic stuff 
for threaded apps that don't rely on ptrace locking).

		Linus

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Vegard Nossum]

On Wed, Jan 28, 2009 at 12:31 AM, Ingo Molnar <mingo@elte.hu> wrote:
> It would be much better if we exported _much_ less and reduced our
> cross-section to user-space. Also, the include/linux/Kbuild rules are all
> but transparent: it would also be nice if whatever we exported was be
> visible straight in the header itself, to make it obvious to people who
> modify/extend those files that those definitions are going to be exported
> to user-space.
>
> Some __user_export tag on structures perhaps? I have no good ideas here -
> #ifdefs are ugly and tags obscure the purity of the code.

Something that might or might not be doable in practice, but at least
it's a suggestion that I haven't seen elsewhere:

Create an include/user/ directory that contains a "mirror" of the
include/ directory _structure_, so that random exported header
include/linux/foo.h now has two parts -- include/linux/foo.h and
include/user/linux/foo.h.

- include/user/linux/foo.h contains the definitions that are needed by
both kernel and userspace
- include/linux/foo.h contains the definitions that are needed only by
the kernel
- include/linux/foo.h can simply #include <user/linux/foo.h> and no
other change (to source files which _use_ this header) is necessary
- no dependency on a kernel header will exist in a "user" header --
that's how it is now, but this way is more explicit
- the whole include/user/ can be shipped verbatim to /usr/include (or
wherever it is needed)
- no #ifdef __KERNEL__ or #ifdef CONFIG_ stuff in the "user" headers;
no stripping or unifdefing is needed
- it's easier to see exactly what is being exported

Of course, obvious disadvantages are:

- less readable in the sense that what used to be in one file is now
spread across two
- the split itself would probably require a tremendous effort
- other things?

It's just an idea...


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Jaswinder Singh Rajput]

Hello Ingo,

On Wed, 2009-01-28 at 00:31 +0100, Ingo Molnar wrote:

> Jaswider, would you mind re-doing the tree filtering out the CONFIG_* 
> changes? I'll go over the end result once more to make sure it has no 
> changes that make the code look worse.
> 

Sure, why not ;-)

But in your -tip tree you already have these patches. Are you going to
revert them or should I make patches for linus tree.

--
JSR

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Ingo Molnar]

* Jaswinder Singh Rajput <jaswinder@kernel.org> wrote:

> Hello Ingo,
> 
> On Wed, 2009-01-28 at 00:31 +0100, Ingo Molnar wrote:
> 
> > Jaswider, would you mind re-doing the tree filtering out the CONFIG_* 
> > changes? I'll go over the end result once more to make sure it has no 
> > changes that make the code look worse.
> 
> Sure, why not ;-)
> 
> But in your -tip tree you already have these patches. Are you going to 
> revert them or should I make patches for linus tree.

yes, i'll replace that branch with your tree - the bad commits are 
intermixed with the good commits so reverting does not help (there would 
be way too many ugly reverts).

As long as you base your patches on Linus's -git tree i'll be able to sort 
it all out.

	Ingo

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Harvey Harrison]

On Tue, 2009-01-27 at 14:57 -0800, Linus Torvalds wrote:
> 
> On Tue, 27 Jan 2009, Ingo Molnar wrote:
> > 
> > Should i perhaps not bother with the stuff below? Cannot turn off 
> > CONFIG_HEADERS_CHECK in my builds because it can cause build failures.

> So I think it makes our headers worse. Code like
> 
> 	> +#ifdef __KERNEL__
> 	> +# ifdef CONFIG_X86_BSWAP
> 	> +# define __X86_BSWAP  
> 	> +# endif /* CONFIG_X86_BSWAP */
> 	> +#endif /* __KERNEL__ */
> 
> just doesn't make sense. It doesn't make sense _inside_ the kernel, and it 
> doesn't make sense _outside_ it either.
> 

Or just wrap all of asm/swab.h in #ifdef __KERNEL__.

Harvey

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Jaswinder Singh Rajput]

On Tue, 2009-01-27 at 14:57 -0800, Linus Torvalds wrote:

> 
> The thing is, the headers_check stuff is just wrong if it causes these 
> things, and I'd rather just turn it off.
> 

Basic idea of headers_check is:
1. clean header files so that they do not export useless things in userspace, like:
    usr/include/linux/elf-fdpic.h:62: extern's make no sense in userspace

2. provide sufficient stuff like:
   usr/include/asm/swab.h:7: found __[us]{8,16,32,64} type without #include <linux/types.h>

3. warns about exporting kernel space defines which are not valid in userspace and is always false for userspace
   usr/include/asm/swab.h:10: leaks CONFIG_X86 to userspace where it is not valid

And by headers_check tools we also able to find blunder mistakes which are very difficult to find by naked eye like:
   usr/include/asm-generic/fcntl.h:127: leaks CONFIG_64BIT to userspace where it is not valid 
for userspace this will be always false.
and some files was totally covered with ifdefs CONFIG_* and exported which are useless like:
  usr/include/linux/if_frad.h:29: leaks CONFIG_DLCI to userspace where it is not valid


> If those 
> 
> 	#ifdef CONFIG_XYZ
> 
> things result in problems, then we should just make the rule be that we 
> turn that kind of string into
> 
> 	#if 0
> 

This will looks ugly in userspace with so many #if 0 and make impossible
to read the code.

> automatically when exporting the kernel headers. IOW, just about 
> _anything_ that headers_check complains about automatically is something 
> that should just be _fixed_ automatically at header install time rather 
> than make the code harder to read.
> 
> So I think it makes our headers worse. Code like
> 
> 	> +#ifdef __KERNEL__
> 	> +# ifdef CONFIG_X86_BSWAP
> 	> +# define __X86_BSWAP  
> 	> +# endif /* CONFIG_X86_BSWAP */
> 	> +#endif /* __KERNEL__ */
> 
> just doesn't make sense. It doesn't make sense _inside_ the kernel, and it 
> doesn't make sense _outside_ it either.
> 

my earlier patch was like this:

diff --git a/arch/x86/include/asm/swab.h b/arch/x86/include/asm/swab.h
index 306d417..613be68 100644
--- a/arch/x86/include/asm/swab.h
+++ b/arch/x86/include/asm/swab.h
@@ -1,12 +1,15 @@
 #ifndef _ASM_X86_SWAB_H
 #define _ASM_X86_SWAB_H
 
-#include <asm/types.h>
+#include <linux/types.h>
+#ifdef __KERNEL__
 #include <linux/compiler.h>
+#endif /* __KERNEL__ */
 
 static inline __attribute_const__ __u32 __arch_swab32(__u32 val)
 {
 #ifdef __i386__
+#ifdef __KERNEL__
 # ifdef CONFIG_X86_BSWAP
        asm("bswap %0" : "=r" (val) : "0" (val));
 # else
@@ -16,7 +19,13 @@ static inline __attribute_const__ __u32
__arch_swab32(__u32 val)
            : "=q" (val)
            : "0" (val));
 # endif
-
+#else /* __KERNEL__ */
+       asm("xchgb %b0,%h0\n\t" /* swap lower bytes     */
+           "rorl $16,%0\n\t"   /* swap words           */
+           "xchgb %b0,%h0"     /* swap higher bytes    */
+           : "=q" (val)
+           : "0" (val));
+#endif /* __KERNEL__ */
 #else /* __i386__ */
        asm("bswapl %0"
            : "=r" (val)
@@ -37,6 +46,7 @@ static inline __attribute_const__ __u64
__arch_swab64(__u64 val)
                __u64 u;
        } v;
        v.u = val;
+#ifdef __KERNEL__
 # ifdef CONFIG_X86_BSWAP
        asm("bswapl %0 ; bswapl %1 ; xchgl %0,%1"
            : "=r" (v.s.a), "=r" (v.s.b)
@@ -48,6 +58,13 @@ static inline __attribute_const__ __u64
__arch_swab64(__u64 val)
            : "=r" (v.s.a), "=r" (v.s.b)
            : "0" (v.s.a), "1" (v.s.b));
 # endif
+#else /* __KERNEL__ */
+       v.s.a = __arch_swab32(v.s.a);
+       v.s.b = __arch_swab32(v.s.b);
+       asm("xchgl %0,%1"
+           : "=r" (v.s.a), "=r" (v.s.b)
+           : "0" (v.s.a), "1" (v.s.b));
+#endif /* __KERNEL__ */
        return v.u;
 #else /* __i386__ */
        asm("bswapq %0"

to get rid of multiple blocks so I used above technique.

If we do not want to export __arch_swab32 and __arch_swab64 then we can
put whole block in #ifdef __KERNEL__ then we will get rid of above
solution.

--
JSR

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Arnd Bergmann]

On Wednesday 28 January 2009, Jaswinder Singh Rajput wrote:
> If we do not want to export __arch_swab32 and __arch_swab64 then we can
> put whole block in #ifdef __KERNEL__ then we will get rid of above
> solution.

For the specific x86 swab code, that would certainly be the simplest
way, user space should not be using those inline assemblies either
way.

I think the more interesting question is whether we want to export
*any* inline helpers that are not part of the ABI to user space.
We already killed most of them (spinlocks, atomics, ...) and what
remains is basically just the byteorder code. All that is required
for the ABI is the information whether the system is big- or
little-endian, but not all the rest.

	Arnd <><

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Arnd Bergmann wrote:
> 
> For the specific x86 swab code, that would certainly be the simplest
> way, user space should not be using those inline assemblies either
> way.
> 
> I think the more interesting question is whether we want to export
> *any* inline helpers that are not part of the ABI to user space.
> We already killed most of them (spinlocks, atomics, ...) and what
> remains is basically just the byteorder code. All that is required
> for the ABI is the information whether the system is big- or
> little-endian, but not all the rest.
> 

In general, no.  The byteswap API is a legacy exception.

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Harvey Harrison]

On Wed, 2009-01-28 at 09:48 -0800, H. Peter Anvin wrote:
> Arnd Bergmann wrote:
> > 
> > For the specific x86 swab code, that would certainly be the simplest
> > way, user space should not be using those inline assemblies either
> > way.
> > 
> > I think the more interesting question is whether we want to export
> > *any* inline helpers that are not part of the ABI to user space.
> > We already killed most of them (spinlocks, atomics, ...) and what
> > remains is basically just the byteorder code. All that is required
> > for the ABI is the information whether the system is big- or
> > little-endian, but not all the rest.
> > 
> 
> In general, no.  The byteswap API is a legacy exception.
> 

But now that swab.h has been separated out, we could just stop exporting the
asm/swab.h bits while still providing a generic C-based implementation to
userspace.

Harvey

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Linus Torvalds]

On Wed, 28 Jan 2009, Harvey Harrison wrote:
> On Wed, 2009-01-28 at 09:48 -0800, H. Peter Anvin wrote:
> > 
> > In general, no.  The byteswap API is a legacy exception.
> 
> But now that swab.h has been separated out, we could just stop exporting the
> asm/swab.h bits while still providing a generic C-based implementation to
> userspace.

Well, the _reason_ the byteswap stuff has been interesting to user space 
is that the kernel did it better than the alternatives. Rather than having 
purely "work with big-endian data" (the networking htonl etc functions), 
the kernel had good and fairly optimized handling of various different 
forms of byte order handling.

Which is why people wanted to use it in the first place - and which is why 
then doing just the generic C-based thing doesn't really fix the issue. 
Things may compile, but they kind of lost the point.

		Linus

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Harvey Harrison]

On Wed, 2009-01-28 at 11:44 -0800, Linus Torvalds wrote:
> 
> On Wed, 28 Jan 2009, Harvey Harrison wrote:
> > On Wed, 2009-01-28 at 09:48 -0800, H. Peter Anvin wrote:
> > > 
> > > In general, no.  The byteswap API is a legacy exception.
> > 
> > But now that swab.h has been separated out, we could just stop exporting the
> > asm/swab.h bits while still providing a generic C-based implementation to
> > userspace.
> 
> Well, the _reason_ the byteswap stuff has been interesting to user space 
> is that the kernel did it better than the alternatives. Rather than having 
> purely "work with big-endian data" (the networking htonl etc functions), 
> the kernel had good and fairly optimized handling of various different 
> forms of byte order handling.
> 
> Which is why people wanted to use it in the first place - and which is why 
> then doing just the generic C-based thing doesn't really fix the issue. 
> Things may compile, but they kind of lost the point.

There was at least some discussion on the gcc-list in November about recognizing
the byteswap idiom and inserting optimized instructions if available...so hopefully
in the future this just fixes itself.

For now, the problem is with arches like X86 that need to test the availability of
an instruction.  So the arch versions could be unconditionally offered on X86_64
and the lowest-common denominator (no BSWAP) on X86-32.  If we still want the
optimized (BSWAP) versions on X86-32 the tests will have to use the compiler arch flags
as opposed to CONFIG_BSWAP....which is probably not worth the trouble.

Harvey

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Harvey Harrison wrote:
> 
> For now, the problem is with arches like X86 that need to test the availability of
> an instruction.  So the arch versions could be unconditionally offered on X86_64
> and the lowest-common denominator (no BSWAP) on X86-32.  If we still want the
> optimized (BSWAP) versions on X86-32 the tests will have to use the compiler arch flags
> as opposed to CONFIG_BSWAP....which is probably not worth the trouble.
> 

Well, that's how the headers were originally written, I believe.

CONFIG_BSWAP can be replaced with the __i486__ macro, and since most 
distros compile for i586 or i686 these days, it might actually make 
sense for this one particular case.

	-hpa

[PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Harvey Harrison]

Use ifdef __i486__ to ensure the BSWAP instruction is available
on 32-bit x86.

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
HPA,

I'm afraid my knowledge of gcc compiler flags for various models is
lacking, I used i486 as suggested, just wanted to make sure I understood
you corectly.

 arch/x86/include/asm/swab.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/swab.h b/arch/x86/include/asm/swab.h
index 306d417..9af180c 100644
--- a/arch/x86/include/asm/swab.h
+++ b/arch/x86/include/asm/swab.h
@@ -7,7 +7,7 @@
 static inline __attribute_const__ __u32 __arch_swab32(__u32 val)
 {
 #ifdef __i386__
-# ifdef CONFIG_X86_BSWAP
+# ifdef __i486__
 	asm("bswap %0" : "=r" (val) : "0" (val));
 # else
 	asm("xchgb %b0,%h0\n\t"	/* swap lower bytes	*/
@@ -37,7 +37,7 @@ static inline __attribute_const__ __u64 __arch_swab64(__u64 val)
 		__u64 u;
 	} v;
 	v.u = val;
-# ifdef CONFIG_X86_BSWAP
+# ifdef __i486__
 	asm("bswapl %0 ; bswapl %1 ; xchgl %0,%1"
 	    : "=r" (v.s.a), "=r" (v.s.b)
 	    : "0" (v.s.a), "1" (v.s.b));
-- 
1.6.1.401.gf39d5

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Linus Torvalds]

On Wed, 28 Jan 2009, Harvey Harrison wrote:
>
> Use ifdef __i486__ to ensure the BSWAP instruction is available
> on 32-bit x86.
> 
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
> HPA,
> 
> I'm afraid my knowledge of gcc compiler flags for various models is
> lacking, I used i486 as suggested, just wanted to make sure I understood
> you corectly.

I really don't think this works.

Try this:

	cpp -dM -m32 -march=pentium t.c | grep i486

and see the marked absense of __i486__ anywhere.

At least CONFIG_X86_BSWAP gives the right behaviour for the kernel.

			Linus

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Harvey Harrison]

On Wed, 2009-01-28 at 14:13 -0800, Linus Torvalds wrote:
> 
> On Wed, 28 Jan 2009, Harvey Harrison wrote:
> >
> > Use ifdef __i486__ to ensure the BSWAP instruction is available
> > on 32-bit x86.
> > 
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> > HPA,
> > 
> > I'm afraid my knowledge of gcc compiler flags for various models is
> > lacking, I used i486 as suggested, just wanted to make sure I understood
> > you corectly.
> 
> I really don't think this works.
> 
> Try this:
> 
> 	cpp -dM -m32 -march=pentium t.c | grep i486
> 
> and see the marked absense of __i486__ anywhere.
> 
> At least CONFIG_X86_BSWAP gives the right behaviour for the kernel.

You're right, it doesn't work.  Perhaps just exporting the bswap version
all the time and i386-only people can worry about patching it out?

Harvey

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Pavel Machek]

On Wed 2009-01-28 14:40:44, Harvey Harrison wrote:
> On Wed, 2009-01-28 at 14:13 -0800, Linus Torvalds wrote:
> > 
> > On Wed, 28 Jan 2009, Harvey Harrison wrote:
> > >
> > > Use ifdef __i486__ to ensure the BSWAP instruction is available
> > > on 32-bit x86.
> > > 
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > > ---
> > > HPA,
> > > 
> > > I'm afraid my knowledge of gcc compiler flags for various models is
> > > lacking, I used i486 as suggested, just wanted to make sure I understood
> > > you corectly.
> > 
> > I really don't think this works.
> > 
> > Try this:
> > 
> > 	cpp -dM -m32 -march=pentium t.c | grep i486
> > 
> > and see the marked absense of __i486__ anywhere.
> > 
> > At least CONFIG_X86_BSWAP gives the right behaviour for the kernel.
> 
> You're right, it doesn't work.  Perhaps just exporting the bswap version
> all the time and i386-only people can worry about patching it out?

And break i386 in a nasty way? Please don't.
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Harvey Harrison wrote:
> I'm afraid my knowledge of gcc compiler flags for various models is
> lacking, I used i486 as suggested, just wanted to make sure I understood
> you corectly.

You did, but I misremembered... instead of having the __i386__, 
__i486__, __i586__, __i686__ being an additive chain as would make 
sense, gcc just has __i386__ plus whichever corresponds to the -march= 
option.  I keep forgetting this because it's just so incredibly dumb.

Bloody hell.  This really f*cks thing up.

What's worse, they seem to simply be adding new options, so at this 
point you'd actually need something like:

# if defined(__i486__) || defined(__i586__) || defined(__i686__) || \
	defined(__core2__) || defined(__k8__) || defined(__amdfam10__)

Worse, there isn't any kind of macro that can be used to compare for a 
negative (i.e. not i386).

This obviously is screaming to be abstracted away into a header of its 
own, but it really can't be done cleanly as far as I can tell because of 
this particular piece of major gcc braindamage.

So, one ends up doing something like:

#ifdef __i486__
# define __CPU_HAVE_BSWAP
#endif
#ifdef __i586__
# define __CPU_HAVE_BSWAP
#endif

... and so on, and have to keep this up to date with the latest 
inventions of the gcc people.  *Sob.*

	-hpa

	-hpa

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Harvey Harrison]

On Wed, 2009-01-28 at 14:15 -0800, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > I'm afraid my knowledge of gcc compiler flags for various models is
> > lacking, I used i486 as suggested, just wanted to make sure I understood
> > you corectly.
> 
> You did, but I misremembered... instead of having the __i386__, 
> __i486__, __i586__, __i686__ being an additive chain as would make 
> sense, gcc just has __i386__ plus whichever corresponds to the -march= 
> option.  I keep forgetting this because it's just so incredibly dumb.
> 
> Bloody hell.  This really f*cks thing up.

> What's worse, they seem to simply be adding new options, so at this 
> point you'd actually need something like:
> 
> # if defined(__i486__) || defined(__i586__) || defined(__i686__) || \
> 	defined(__core2__) || defined(__k8__) || defined(__amdfam10__)
> 
> Worse, there isn't any kind of macro that can be used to compare for a 
> negative (i.e. not i386).

Well, that's unfortunate, how about we just export the BSWAP version
unconditionally and hope pure i386 just goes away someday?

> 
> This obviously is screaming to be abstracted away into a header of its 
> own, but it really can't be done cleanly as far as I can tell because of 
> this particular piece of major gcc braindamage.
> 
> So, one ends up doing something like:
> 
> #ifdef __i486__
> # define __CPU_HAVE_BSWAP
> #endif
> #ifdef __i586__
> # define __CPU_HAVE_BSWAP
> #endif
> 
> ... and so on, and have to keep this up to date with the latest 
> inventions of the gcc people.  *Sob.*

Unpleasant indeed.  Is there a byteswap builtin in gcc?  At least AVR32
seems to use it, but perhaps it's not generally exposed...perhaps we
could ask the gcc-folk?

Harvey

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Ben Pfaff]

Harvey Harrison <harvey.harrison@gmail.com> writes:

> Is there a byteswap builtin in gcc?  At least AVR32
> seems to use it, but perhaps it's not generally exposed...perhaps we
> could ask the gcc-folk?

Yes, GCC has byteswap builtins on x86 documented as follows:

 -- Built-in Function: int32_t __builtin_bswap32 (int32_t x)
     Returns X with the order of the bytes reversed; for example,
     `0xaabbccdd' becomes `0xddccbbaa'.  Byte here always means exactly
     8 bits.

 -- Built-in Function: int64_t __builtin_bswap64 (int64_t x)
     Similar to `__builtin_bswap32', except the argument and return
     types are 64-bit.

These were only added as of GCC 4.3 though.
-- 
Ben Pfaff 
http://benpfaff.org

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Ben Pfaff wrote:
> Harvey Harrison <harvey.harrison@gmail.com> writes:
> 
>> Is there a byteswap builtin in gcc?  At least AVR32
>> seems to use it, but perhaps it's not generally exposed...perhaps we
>> could ask the gcc-folk?
> 
> Yes, GCC has byteswap builtins on x86 documented as follows:
> 
>  -- Built-in Function: int32_t __builtin_bswap32 (int32_t x)
>      Returns X with the order of the bytes reversed; for example,
>      `0xaabbccdd' becomes `0xddccbbaa'.  Byte here always means exactly
>      8 bits.
> 
>  -- Built-in Function: int64_t __builtin_bswap64 (int64_t x)
>      Similar to `__builtin_bswap32', except the argument and return
>      types are 64-bit.
> 
> These were only added as of GCC 4.3 though.

It would make sense to use them if gcc >= 4.3 (both for userspace and
kernel space.)  IMO, for older compilers we can just punt on the
enhancement for userspace and fall back to the i386 code.

	-hpa

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

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Harvey Harrison wrote:
> 
> Well, that's unfortunate, how about we just export the BSWAP version
> unconditionally and hope pure i386 just goes away someday?
> 

Well, we already have MOVBE coming up, too...

	-hpa

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Harvey Harrison]

On Wed, 2009-01-28 at 15:27 -0800, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > 
> > Well, that's unfortunate, how about we just export the BSWAP version
> > unconditionally and hope pure i386 just goes away someday?
> > 
> 
> Well, we already have MOVBE coming up, too...
> 

Is someone already working on an __arch_swab{16|32|64}p to use them?

Harvey

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Harvey Harrison wrote:
> On Wed, 2009-01-28 at 15:27 -0800, H. Peter Anvin wrote:
>> Harvey Harrison wrote:
>>> Well, that's unfortunate, how about we just export the BSWAP version
>>> unconditionally and hope pure i386 just goes away someday?
>>>
>> Well, we already have MOVBE coming up, too...
>>
> 
> Is someone already working on an __arch_swab{16|32|64}p to use them?
> 

Not that I know of, but it's trivial enough.  They can also be used for 
all-register swapping, too, with the advantage that you get register 
decoupling.

	-hpa

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Arnd Bergmann]

On Thursday 29 January 2009, H. Peter Anvin wrote:
> Harvey Harrison wrote:
> > On Wed, 2009-01-28 at 15:27 -0800, H. Peter Anvin wrote:
> >> Harvey Harrison wrote:
> >>> Well, that's unfortunate, how about we just export the BSWAP version
> >>> unconditionally and hope pure i386 just goes away someday?
> >>>
> >> Well, we already have MOVBE coming up, too...
> >>
> > 
> > Is someone already working on an __arch_swab{16|32|64}p to use them?
> > 
> 
> Not that I know of, but it's trivial enough.  They can also be used for 
> all-register swapping, too, with the advantage that you get register 
> decoupling.

I just realized that gcc-4.3 and higher have the __builtin_bswap{16,32,64}
functions on x86, which are supposed to do the right thing on any
platform. Maybe a patch like the one below can solve this for both the
kernel and for other users of the byteorder headers. Unfortunately, I
could not find out whether the builtins are available on all other
platforms as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

--- a/arch/x86/include/asm/swab.h
+++ b/arch/x86/include/asm/swab.h
@@ -4,9 +4,17 @@
 #include <linux/types.h>
 #include <linux/compiler.h>
 
+#ifdef __CHECKER__
+extern unsigned long __builtin_bswap_32(unsigned long x);
+extern unsigned long long __builtin_bswap_64(unsigned long long x);
+#endif
+
 static inline __attribute_const__ __u32 __arch_swab32(__u32 val)
 {
-#ifdef __i386__
+#if defined(__GNUC__) && ((__GNUC__ > 4) || \
+		((__GNUC__ == 4) && (__GNUC_MINOR__ >= 3)))
+	val = __builtin_bswap32(val);
+#elif defined (__i386__)
 # ifdef CONFIG_X86_BSWAP
 	asm("bswap %0" : "=r" (val) : "0" (val));
 # else
@@ -28,7 +36,10 @@ static inline __attribute_const__ __u32 __arch_swab32(__u32 val)
 
 static inline __attribute_const__ __u64 __arch_swab64(__u64 val)
 {
-#ifdef __i386__
+#if defined(__GNUC__) && ((__GNUC__ > 4) || \
+		((__GNUC__ == 4) && (__GNUC_MINOR__ >= 3)))
+	return __builtin_bswap64(val);
+#elif defined (__i386__)
 	union {
 		struct {
 			__u32 a;

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Maciej W. Rozycki]

On Wed, 28 Jan 2009, Harvey Harrison wrote:

> Well, that's unfortunate, how about we just export the BSWAP version
> unconditionally and hope pure i386 just goes away someday?

 Or just emulate it alongside CMPXCHG and XADD? ;)

  Maciej

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Maciej W. Rozycki wrote:
> On Wed, 28 Jan 2009, Harvey Harrison wrote:
> 
>> Well, that's unfortunate, how about we just export the BSWAP version
>> unconditionally and hope pure i386 just goes away someday?
> 
>  Or just emulate it alongside CMPXCHG and XADD? ;)

That would work unless someone actually cares about 386 or 486 in the
embedded space.  I think at least 486EX is still used as an embedded
platform.

The kernel can obviously provide this service (although it slows down
the #UD handler), but anyone who actually cares about these chips would
not really want them.

At some point, we may want to ask ourselves how long keeping 386 support
(with it's broken WP handling) alive.

	-hpa

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

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[Arnd Bergmann]

On Wednesday 28 January 2009, H. Peter Anvin wrote:
> So, one ends up doing something like:
> 
> #ifdef __i486__
> # define __CPU_HAVE_BSWAP
> #endif
> #ifdef __i586__
> # define __CPU_HAVE_BSWAP
> #endif
> 
> ... and so on, and have to keep this up to date with the latest 
> inventions of the gcc people.  *Sob.*

Well, to put this into perspective: The bswap inline assembly was
introduced in Linux-1.3.51 "Greased Weasel", back in 1995 and at
no time it was ever visible to user space, unless the code manually
included <linux/config.h> (which we broke) or defined CONFIG_M486,
CONFIG_X86_BSWAP and/or __KERNEL__, depending on the kernel version.

I take this as a strong indication that user space applications
won't generally expect to get the bswap instruction from including
the kernel headers. For the longest time, we actually had

/* For avoiding bswap on i386 */
#ifdef __KERNEL__
#include <linux/config.h>
#endif

which I read as explicitly using the portable i386 version for
all user space.

	Arnd <><

Re: [PATCH] x86: do not expose CONFIG_BSWAP to userspace

[H. Peter Anvin]

Arnd Bergmann wrote:
> 
> I take this as a strong indication that user space applications
> won't generally expect to get the bswap instruction from including
> the kernel headers. For the longest time, we actually had
> 
> /* For avoiding bswap on i386 */
> #ifdef __KERNEL__
> #include <linux/config.h>
> #endif
> 
> which I read as explicitly using the portable i386 version for
> all user space.
> 

This is true, although it would be nice to be able to generate BSWAP now 
when most distros compile with -march=i586 or higher.  The reason noone 
cared for the longest time was that noone wanted to compile userspace 
for anything other than the i386 baseline.

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Sam Ravnborg]

On Wed, Jan 28, 2009 at 11:44:13AM -0800, Linus Torvalds wrote:
> 
> 
> On Wed, 28 Jan 2009, Harvey Harrison wrote:
> > On Wed, 2009-01-28 at 09:48 -0800, H. Peter Anvin wrote:
> > > 
> > > In general, no.  The byteswap API is a legacy exception.
> > 
> > But now that swab.h has been separated out, we could just stop exporting the
> > asm/swab.h bits while still providing a generic C-based implementation to
> > userspace.
> 
> Well, the _reason_ the byteswap stuff has been interesting to user space 
> is that the kernel did it better than the alternatives. Rather than having 
> purely "work with big-endian data" (the networking htonl etc functions), 
> the kernel had good and fairly optimized handling of various different 
> forms of byte order handling.
> 
> Which is why people wanted to use it in the first place - and which is why 
> then doing just the generic C-based thing doesn't really fix the issue. 
> Things may compile, but they kind of lost the point.

The right fix then is to provide these optimized versions as part
of their libc variant, not to keep exporting our internal
versions to userland. Because the internal version may
be subject to runtime patching and api changes etc.

	Sam

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[H. Peter Anvin]

Harvey Harrison wrote:
> 
> But now that swab.h has been separated out, we could just stop exporting the
> asm/swab.h bits while still providing a generic C-based implementation to
> userspace.
> 

That makes sense if and only if the generic C-based implementation 
doesn't suck.

	-hpa

Re: [mingo@elte.hu: [git pull] headers_check fixes]

[Sam Ravnborg]

On Tue, Jan 27, 2009 at 02:57:23PM -0800, Linus Torvalds wrote:
> 
> 
> On Tue, 27 Jan 2009, Ingo Molnar wrote:
> > 
> > Should i perhaps not bother with the stuff below? Cannot turn off 
> > CONFIG_HEADERS_CHECK in my builds because it can cause build failures.
> 
> I really hate the patch. I think it's fundamentally flawed. I hate scripts 
> that test for things that are readable, and encourage people to then write 
> crap instead.
> 
> The thing is, the headers_check stuff is just wrong if it causes these 
> things, and I'd rather just turn it off.
> 
> If those 
> 
> 	#ifdef CONFIG_XYZ
> 
> things result in problems, then we should just make the rule be that we 
> turn that kind of string into
> 
> 	#if 0
> 
> automatically when exporting the kernel headers. IOW, just about 
> _anything_ that headers_check complains about automatically is something 
> that should just be _fixed_ automatically at header install time rather 
> than make the code harder to read.

Almost every single patch that makes code harder to read while removing
CONFIG_ references from the exported headers just take the wrong approach.

I read later in this thread that Ingo is planning to filter out these patches
so we can get the other ones in - good.

What we should try to do is to look at the bigger picture.
Sometimes the right answer is that this whole file should not
be exported or maybe only half should be exported.
But due to the fact that we have no established way to seperate
what is internal and what is exported *on the file level* people
keep sprinkling #ifdef __KERNEL__ all over the header files.

I checked include/linux - we have 494 places where we reference __KERNEL__
And that is in 237 files.
We export 354 files from this dir - so we already have > 100 files that is
exported to userspace without modifications.

So rather than start sprinkling the exported headers with "if 0" then
we should encourage to clean them and when accetable move them to
a dedicated abi directory.

This may need some cooperation with external parties such as glibc
and other libc variants. And we could improve here.

	Sam