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
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
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
* Linus Torvalds <[email protected]> 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
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
On Wed, Jan 28, 2009 at 12:31 AM, Ingo Molnar <[email protected]> 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
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
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
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
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
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
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 <><
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
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
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
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
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
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
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
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
Use ifdef __i486__ to ensure the BSWAP instruction is available
on 32-bit x86.
Signed-off-by: Harvey Harrison <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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 <[email protected]>
> > ---
> > 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
Harvey Harrison <[email protected]> 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
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 <><
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
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
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
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
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
* Jaswinder Singh Rajput <[email protected]> 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
Ben Pfaff wrote:
> Harvey Harrison <[email protected]> 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.
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
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.