[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130226233342.GF20323@brightrain.aerifal.cx>
Date: Tue, 26 Feb 2013 18:33:42 -0500
From: Rich Felker <dalias@...ifal.cx>
To: musl@...ts.openwall.com
Subject: Re: Fix for fields in utmp
On Tue, Feb 26, 2013 at 09:21:12PM +0100, Szabolcs Nagy wrote:
> see inline comments
>
> * Ivan Kanakarakis <ivan.kanak@...il.com> [2013-02-26 12:34:43 +0200]:
> > Linux suicidemachine 3.7.5-1-ARCH #1 SMP PREEMPT Mon Jan 28 10:03:32
> > CET 2013 x86_64 GNU/Linux
> > ----------------------------------
> > --- data/glibc.sizeof 2013-02-26 12:27:48.112569344 +0200
> > +++ data/musl.sizeof 2013-02-26 12:27:48.119236080 +0200
> > @@ -90,3 +90,3 @@
> > float 4
> > -float_t 4
> > +float_t 8
>
> looks like musl does not respect FLT_EVAL_METHOD on x86_64
> this should be fixed
I just changed this to 4, as it should be. Or does x86_64 support
changing the FLT_EVAL_METHOD, in which case we'd need to treat it like
on i386..
> > fpos_t 16
> > @@ -112,4 +112,4 @@
> > int8_t 1
> > -int_fast16_t 8
> > -int_fast32_t 8
> > +int_fast16_t 4
> > +int_fast32_t 4
> > int_fast64_t 8
>
> these diffs may cause problems
These are actually omitted from the ABI documents if I remember
correctly, with the idea that different compilers might define them
differently and that they shouldn't be used for interfaces between
functions, only internal use. There may be compelling reasons we
should match anyway, but there's one compelling reason NOT to match:
the glibc values are WRONG. 64-bit integers are much slower than
32-bit integers if you just need 16 or 32 bits of data for at least
two operations: division and modulo. And they're at best the same
speed (but larger and thus slower in a cache pressure sense) for other
arithmetic operations.
> > @@ -122,3 +122,3 @@
> > intptr_t 8
> > -jmp_buf 200
> > +jmp_buf 64
This is because our jmp_buf does not contain sigset_t. However due to
ABI issues for C++ (glibc's jmp_buf and sigjmp_buf sharing the same
struct tag) we might want to make them the same anyway. There's an
existing thread somewhere on the topic.
> > key_t 4
> > @@ -181,6 +181,6 @@
> > quad_t 8
> > -regex_t 64
> > +regex_t 56
I think this should be fixed to match.
> > register_t 8
> > -regmatch_t 8
> > -regoff_t 4
> > +regmatch_t 16
> > +regoff_t 8
These are a bug in glibc; their types do not conform to the
requirements imposed on regex.h and regexec().
> > res_state 8
> > @@ -196,3 +196,3 @@
> > sighandler_t 8
> > -siginfo_t 128
> > +siginfo_t 136
This is probably a mistake, maybe in the padding..?
> > sigjmp_buf 200
> > @@ -215,3 +215,3 @@
> > struct ar_hdr 60
> > -struct arpd_request 40
> > +struct arpd_request 28
No idea.
> > struct arphdr 8
> > @@ -222,3 +222,3 @@
> > struct cmsghdr 16
> > -struct crypt_data 131232
> > +struct crypt_data 260
This is intentional. glibc's crypt_data is uselessly large and does
not fit on thread stacks. The only useful part of crypt_data is a
buffer for storing the resulting hash.
> > struct dirent 280
> > @@ -282,3 +282,3 @@
> > struct itimerval 32
> > -struct lastlog 292
> > +struct lastlog 296
Probably needs to be checked.
> > struct lconv 96
> > @@ -312,3 +312,3 @@
> > struct ns_tsig_key 2072
> > -struct ntptimeval 72
> > +struct ntptimeval 32
Should we change this? Probably.
> > struct option 32
> > @@ -326,4 +326,4 @@
> > struct rtentry 120
> > -struct rusage 144
> > -struct sched_param 4
> > +struct rusage 272
> > +struct sched_param 48
Not sure about rusage. For sched_param, we support the extra fields
for the sporadic server option even though we don't support it (and
Linux probably never will).
> > struct sembuf 6
> > @@ -348,3 +348,3 @@
> > struct sockaddr_ll 20
> > -struct sockaddr_storage 128
> > +struct sockaddr_storage 136
Probably wrong.
> > struct sockaddr_un 110
> > @@ -361,3 +361,3 @@
> > struct strrecvfd 20
> > -struct sysinfo 112
> > +struct sysinfo 368
Just extra padding for growth. Maybe it should be changed.
> > struct termios 60
> > @@ -376,3 +376,3 @@
> > struct utimbuf 16
> > -struct utmpx 384
> > +struct utmpx 400
>
> so we need the glibc hacks to be abi compatible:
>
> struct utmpx
> {
> short ut_type;
> pid_t ut_pid;
> char ut_line[UT_LINESIZE];
> char ut_id[4];
> char ut_user[32];
> char ut_host[256];
> struct {
> short e_termination;
> short e_exit;
> } ut_exit;
> #if 64 bit abi
> __int32_t ut_session;
> struct {__int32_t tv_sec; __int32_t tv_usec;} ut_tv;
> #else
> long ut_session;
> struct timeval ut_tv;
> #endif
> unsigned ut_addr_v6[4];
> char __unused[20];
> };
>
> the comment in glibc sysdeps/gnu/bits/utmpx.h is
>
> /* The fields ut_session and ut_tv must be the same size when compiled
> 32- and 64-bit. This allows files and shared memory to be shared
> between 32- and 64-bit applications. */
This is idiotic and breaks correct applications that access the ut_tv
member as ut_tv rather than just accessing its members, since ut_tv
now has the wrong type and cannot be assigned to/from timeval structs
and cannot have its address passed to functions that expect timeval
structs.
Since utmp is not supported anyway, I think it's better to have the
right types to avoid breaking correct code; the data will never be
written or read anyway; it just goes to the bitbucket so layout does
not matter so much.
> > struct utsname 390
> > @@ -399,4 +399,4 @@
> > uint8_t 1
> > -uint_fast16_t 8
> > -uint_fast32_t 8
> > +uint_fast16_t 4
> > +uint_fast32_t 4
> > uint_fast64_t 8
See above.
> > @@ -416,4 +416,4 @@
> > wchar_t 4
> > -wctrans_t 8
> > -wctype_t 8
> > +wctrans_t 4
> > +wctype_t 4
> > wint_t 4
> >
>
> it seems wctype_t is unconditionally long in glibc
As it should be. This is a bug in musl. Being an opaque type that
could (and often should) be implemented as a pointer, wctype_t should
always be pointer-sized.
Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
