[PATCH] Beginnings of conpat 32 code cleanups

[PATCH] Beginnings of conpat 32 code cleanups

[Stephen Rothwell]

Hi Linus,

There is a lot of duplicated code among the 32 compatibility layers in our
64 bit architectures.  I am proposing to considate this as much as
possible. To that end, I first need to tidy up the relevant header files
and make them as common as possible.  Discussions with Dave Miller, Andi
Kleen, and Anton Blanchard has led to the creation of compat32.h to
contain all the 32 compatibility data types.

This patch merely adds include/asm-generic/compat32.h which is the header
information that is common to all the 32 bit compatibility code across all
the architectures (except parisc as I don't pretend to understand that
:-)).

I will follow this up with patches for each architecture that I can. I
also intend to intruduce a CONFIG_COMPAT32 define that will be used to
wrap generic implementations of some of the 32 bit compatibility code in
our architecture independent code.  The idea is to share as much of the
non compatibility code and where that is not possible, to keep the 32 bit
versions of code near their "normal" (i.e.64 bit) counterparts in order to
minimise the number of bugs introduced and maximise the number of bugs
fixed.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

diff -ruN 2.5.48/include/asm-generic/compat32.h 2.5.48-32bit.1/include/asm-generic/compat32.h
--- 2.5.48/include/asm-generic/compat32.h	1970-01-01 10:00:00.000000000 +1000
+++ 2.5.48-32bit.1/include/asm-generic/compat32.h	2002-11-22 16:02:56.000000000 +1100
@@ -0,0 +1,31 @@
+#ifndef _ASM_GENERIC_COMPAT32_H
+#define _ASM_GENERIC_COMPAT32_H
+
+#include <linux/types.h>
+
+typedef unsigned int		__kernel_size_t32;
+typedef int			__kernel_ssize_t32;
+typedef int			__kernel_time_t32;
+typedef int			__kernel_clock_t32;
+typedef int			__kernel_pid_t32;
+typedef unsigned int		__kernel_ino_t32;
+typedef int			__kernel_daddr_t32;
+typedef int			__kernel_off_t32;
+typedef unsigned int		__kernel_caddr_t32;
+typedef long			__kernel_loff_t32;
+typedef __kernel_fsid_t		__kernel_fsid_t32;
+
+struct timespec32 {
+	s32			tv_sec;
+	s32			tv_nsec;
+};
+
+struct flock32 {
+	short			l_type;
+	short			l_whence;
+	__kernel_off_t32	l_start;
+	__kernel_off_t32	l_len;
+	__kernel_pid_t32	l_pid;
+};
+
+#endif /* _ASM_GENERIC_COMPAT32_H */

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Linus Torvalds]

On Fri, 22 Nov 2002, Stephen Rothwell wrote:
> 
> This patch merely adds include/asm-generic/compat32.h which is the header
> information that is common to all the 32 bit compatibility code across all
> the architectures (except parisc as I don't pretend to understand that
> :-)).

What kind of strange _crap_ is this?

	+typedef unsigned int           __kernel_size_t32;
	+typedef int                    __kernel_ssize_t32;
	+typedef int                    __kernel_time_t32;
	+typedef int                    __kernel_clock_t32;
	+typedef int                    __kernel_pid_t32;
	+typedef unsigned int           __kernel_ino_t32;
	+typedef int                    __kernel_daddr_t32;
	+typedef int                    __kernel_off_t32;
	+typedef unsigned int           __kernel_caddr_t32;
	+typedef long                   __kernel_loff_t32;

You're doing a compat layer, and then you're using various undefined types 
that can be random sizes, and calling them xxx_t32.

For christ sake, somebody is on drugs here.

If they are called "xxx_t32", then that means that you _know_ the size 
already statically, and you should use "u32" or "s32" which are shorter 
and clearer anyway. You should sure as hell not use some random C type 
that can be different depending on compiler options etc, and then calling 
it a "compat" library.

Quite frankly, I don't see the point of this AT ALL. You're introducing 
new types that cannot be sanely used directly anyway. What's the point?

Make your compat stuff use u32/s32/u64 directly, instead of making up ugly 
new types that make no sense.

		Linus

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Larry McVoy]

> Make your compat stuff use u32/s32/u64 directly, instead of making up ugly 
> new types that make no sense.

IMHO, the thing that the early Unix systems did wrong was to not have 
u8, u16, u32, etc as basic ctypes in sys/types.h.  And C should have 
had a way to fake it if they weren't native.

Anyone who has ported a networking stack or worked on driver knows exactly
what I'm talking about.

And while I'm whining, 

	assert(strlen(any typedef) < 8));

I like my stack variable declarations to line up.  I despise some_long_name_t
typedefs with a passion.
-- 
---
Larry McVoy            	 lm at bitmover.com           http://www.bitmover.com/lm 

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Cort Dougan]

"IMHO"?  Larry, you're never humble about your opinions :)

} IMHO, the thing that the early Unix systems did wrong was to not have 
} u8, u16, u32, etc as basic ctypes in sys/types.h.  And C should have 
} had a way to fake it if they weren't native.
} 
} Anyone who has ported a networking stack or worked on driver knows exactly
} what I'm talking about.
} 
} And while I'm whining, 
} 
} 	assert(strlen(any typedef) < 8));

Plan9 takes it a step further and tackles the char/8-byte issue.  A
printable character is a 16-byte entity - a rune - while char is an 8-byte
quantity.  Doing everything in UNICODE forced the issue, I think.

Even better, everything was designed to run with different byte-ordering
schemes so:

ulong
getlong(void)
{
	ulong l;

	l = (getchar()&0xFF)<<24;
	|= (getchar()&0xFF)<<16;
	l |= (getchar()&0xFF)<<8;
	l |= (getchar()&0xFF)<<0;
	return l;
}

always works correctly.  They even ripped out the darn c-preprocessor and
just made a smarter optimization compiler.

Improvements in C without ending up with something like C# or C++.  Can't
beat that with a stick.

Ah, yes... the streets are paved with gold in the land of Plan9.

} I like my stack variable declarations to line up.  I despise some_long_name_t
} typedefs with a passion.

There is a special kind of mind that allows people to follow 5 levels worth
of typedef to struct including stuct with typedefs in them.  It involves
having a very unhealthy outlook on the world.

Re: [PATCH] Beginnings of conpat 32 code cleanups

[H. Peter Anvin]

Followup to:  <20021122131351.C30808@duath.fsmlabs.com>
By author:    Cort Dougan <cort@fsmlabs.com>
In newsgroup: linux.dev.kernel
> 
> Plan9 takes it a step further and tackles the char/8-byte issue.  A
> printable character is a 16-byte entity - a rune - while char is an 8-byte
> quantity.  Doing everything in UNICODE forced the issue, I think.
> 

... at which point it promptly fell apart as 16-bit Unicode was very
quickly found to be insufficient (not really surprising since the
16-bit decision was based on technical convenience rather than actual
requirements.)

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<amsp@zytor.com>

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Stephen Rothwell]

HI Linus,

On Fri, 22 Nov 2002 11:47:27 -0800 (PST) Linus Torvalds <torvalds@transmeta.com> wrote:
>
> On Fri, 22 Nov 2002, Stephen Rothwell wrote:
> > 
> > This patch merely adds include/asm-generic/compat32.h which is the header
> > information that is common to all the 32 bit compatibility code across all
> > the architectures (except parisc as I don't pretend to understand that
> > :-)).
> 
> What kind of strange _crap_ is this?

Thanks for abusing me in public - its just what I needed after having my
attempts at reducing the mess in the arch dependent code ignored for so
long!

> 
> 	+typedef unsigned int           __kernel_size_t32;
> 	+typedef int                    __kernel_ssize_t32;
> 	+typedef int                    __kernel_time_t32;
> 	+typedef int                    __kernel_clock_t32;
> 	+typedef int                    __kernel_pid_t32;
> 	+typedef unsigned int           __kernel_ino_t32;
> 	+typedef int                    __kernel_daddr_t32;
> 	+typedef int                    __kernel_off_t32;
> 	+typedef unsigned int           __kernel_caddr_t32;
> 	+typedef long                   __kernel_loff_t32;

If you had bothered to look, you would have noticed that these are taken
straight from the compatibility code that already exists in all the 64
bit architectures that have 32 bit compatibility layers.  I am into doing
incremental changes that people can see easily are not harmful and then
doing better cleanups later.

> You're doing a compat layer, and then you're using various undefined types 
> that can be random sizes, and calling them xxx_t32.

I am copying what is already there, I am not inventing anything new, just tidying up the crap that has already been written.

> For christ sake, somebody is on drugs here.

Well if anyone, then aim at those who copied each other when creating
the code in the first place.

> If they are called "xxx_t32", then that means that you _know_ the size 
> already statically, and you should use "u32" or "s32" which are shorter 
> and clearer anyway. You should sure as hell not use some random C type 
> that can be different depending on compiler options etc, and then calling 
> it a "compat" library.

The _t32 refers to the fact that they are for the 32 bit compatibility layer
not that they are 32 bit types (you knew that didn't you?).

> Quite frankly, I don't see the point of this AT ALL. You're introducing 
> new types that cannot be sanely used directly anyway. What's the point?

I am not introducing anything, I am tidying up.  As I said the medium
term goal is to consolidate as much of the code as possible to stop people
from copying bugs and/or not having to fix them in every architecture
as we do now.

> Make your compat stuff use u32/s32/u64 directly, instead of making up ugly 
> new types that make no sense.

Once more I AM NOT MAKING ANYTHING UP.  I am consolidating existing
practise.
-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Linus Torvalds]

On Sat, 23 Nov 2002, Stephen Rothwell wrote:
> 
> Once more I AM NOT MAKING ANYTHING UP.  I am consolidating existing
> practise.

It's _still_ crap.

The fact is, that a

	unsigned int xxxx

in a <asm-i386/xxxxx.h> file is fine. On asm-i386, "unsigned int" has 
well-defined behaviour, and is a well-defined type within that world.

However, if you consolidate different files from different architectures,
what is acceptable in some random architecture header file is _not_ any
more acceptable in a "consolidated" entry. Suddenly, "unsigned int" has no
well-defined meaning any more, and certainly not something like "long"  
which will sometimes change even on the same architecture depending on
compiler options etc.

THAT is what I'm complaining about. Code sharing without thinking is BAD. 

Get rid of made-up types and clean them up to use well-defined types. The
whole point of your patch was to clan stuff up, no?

In other words: for something like this, don't even bother with strange
types like "__kernel_loffset_t32". The type simply does not make sense.
The only reason we have __kernel_xxxx types is to export architecture-
specific stuff to user space through "types.h", without polluting the
POSIX- mandated namespaces.

For the 32-bit compatibility stuff, that simply doesn't make sense:

 - the types should never be exposed to user space at all on a source 
   level. It's a ABI compatibility thing, not an API compatibility.

   So the "__kernel_xxx" stuff makes no sense, and only shows that 
   somebody was lazy and did some cut-and-paste followed by adding crud
   instead of doing it right (and yeah, that crap was there from before, 
   but don't sell it as a cleanup)

 - the types aren't even architechure-specific any more, since the whole 
   _point_ of your cleanups is to make the compat32 layer generic. So they 
   shouldn't be things like "loff_t" at _all_ (never mind the __kernel_ 
   prefix), they should just be the architecture-independent "u32" or
   "s64" or whatever the compat code uses.

This is why I think the whole file makes zero sense. Yes, people have 
copied crap before. But that doesn't make it any better.

		Linus

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Anton Blanchard]

 
> You're doing a compat layer, and then you're using various undefined types 
> that can be random sizes, and calling them xxx_t32.
> 
> For christ sake, somebody is on drugs here.

Then it must be davem, Andi and I :) Stephen is just merging what we
already have.

> If they are called "xxx_t32", then that means that you _know_ the size 
> already statically, and you should use "u32" or "s32" which are shorter 
> and clearer anyway. You should sure as hell not use some random C type 
> that can be different depending on compiler options etc, and then calling 
> it a "compat" library.

_t32 == 32 bit version, its not the size. eg

asm-ia64/ia32.h:		typedef unsigned short	__kernel_ipc_pid_t32;
asm-mips64/posix_types.h:	typedef int		__kernel_ipc_pid_t32;
asm-parisc/posix_types.h:	typedef unsigned short	__kernel_ipc_pid_t32;
asm-ppc64/ppc32.h:		typedef unsigned short	__kernel_ipc_pid_t32;
asm-sparc64/posix_types.h:	typedef unsigned short	__kernel_ipc_pid_t32;
asm-x86_64/ia32.h:		typedef unsigned short	__kernel_ipc_pid_t32;

Or do you mean we should use typedef u16 __kernel_ipc_pid_t32? Yeah,
I can understand that.

Anton

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Linus Torvalds]

In article <20021123051628.GA3658@krispykreme>,
Anton Blanchard  <anton@samba.org> wrote:
>
>_t32 == 32 bit version, its not the size. eg

Oh, I realize that. What I do not see is the point of the typedefs AT
ALL. They must go. They are crap. They have no reason for their
existence.

>asm-ia64/ia32.h:		typedef unsigned short	__kernel_ipc_pid_t32;
>asm-mips64/posix_types.h:	typedef int		__kernel_ipc_pid_t32;
>asm-parisc/posix_types.h:	typedef unsigned short	__kernel_ipc_pid_t32;
>asm-ppc64/ppc32.h:		typedef unsigned short	__kernel_ipc_pid_t32;
>asm-sparc64/posix_types.h:	typedef unsigned short	__kernel_ipc_pid_t32;
>asm-x86_64/ia32.h:		typedef unsigned short	__kernel_ipc_pid_t32;
>
>Or do you mean we should use typedef u16 __kernel_ipc_pid_t32? Yeah,
>I can understand that.

That helps, by removing half of the reason why they are crap - the using
of types that are not architecture-safe in a generic ABI file. But the
other half of the reason is still there:

It doesn't remove the rest of the reason: that "__kernel_" prefix is
meaningless (since the type shouldn't be visible in a non-kernel
namespace ANYWAY, and that is the only reason for the prefix in the
first place). 

Basically, you have two cases:

 - you have types that are _truly_ generic 32-bit compatibility stuff,
   and are the same on all architectures that use this compatibility
   layer. 

   But if they are truly generic, they shouldn't need a new typedef AT
   ALL.  You should just realize that "loff_t" is always a "s64", and
   then just use s64 in the compatibility functions/structures. No need
   to make up some new typedef.

 - You have types (like the above) where the compatibility layer
   actually has _different_ types for different architectures. In which
   case they should be in an architecture-specific file, not in some
   generic file. And the name should not be "__kernel_xxxx_t32", but
   "compat32_xxxx_t" or something.

In _neither_ case is it valid to have a generic architecture-independent
file that makes up new types.  See? And THAT is why I thin kthe patch is
crud. 

		Linus

Re: [PATCH] Beginnings of conpat 32 code cleanups

[Pavel Machek]

Hi!

> There is a lot of duplicated code among the 32 compatibility layers in our
> 64 bit architectures.  I am proposing to considate this as much as
> possible. To that end, I first need to tidy up the relevant header files
> and make them as common as possible.  Discussions with Dave Miller, Andi
> Kleen, and Anton Blanchard has led to the creation of compat32.h to
> contain all the 32 compatibility data types.
> 
> This patch merely adds include/asm-generic/compat32.h which is the header
> information that is common to all the 32 bit compatibility code across all
> the architectures (except parisc as I don't pretend to understand that
> :-)).
> 
> I will follow this up with patches for each architecture that I can. I

I'll be happy to test anything you have on
x86-64...

-- 
				Pavel
Written on sharp zaurus, because my Velo1 broke. If you have Velo you don't need...