Fix for fields in utmp

Fix for fields in utmp

[Chris Spiegel]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

The utmp.h header defines a few macros to access __ut_exit in the utmpx
struct; however, the underscores were removed in utmpx, so the macros
now point to non-existent struct members.  Attached is a simple patch
which removes them.

Chris

[-- Attachment #2: no-private-utmp-names.diff --]
[-- Type: text/plain, Size: 348 bytes --]

diff --git a/include/utmp.h b/include/utmp.h
index b145a11..c24d0a8 100644
--- a/include/utmp.h
+++ b/include/utmp.h
@@ -19,9 +19,6 @@ struct lastlog {
 
 #define ut_time ut_tv.tv_sec
 #define ut_name ut_user
-#define ut_exit __ut_exit
-#define e_termination __e_termination
-#define e_exit __e_exit
 #define utmp utmpx
 #define utmpname(x) (-1)
 

Re: Fix for fields in utmp

[Szabolcs Nagy]

* Chris Spiegel <cspiegel@gmail.com> [2013-02-20 10:23:09 -0800]:
> The utmp.h header defines a few macros to access __ut_exit in the utmpx
> struct; however, the underscores were removed in utmpx, so the macros
> now point to non-existent struct members.  Attached is a simple patch
> which removes them.

note that the current code is not compatible with glibc

http://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/gnu/bits/utmpx.h
vs
http://git.musl-libc.org/cgit/musl/tree/include/utmpx.h

it seems the __ prefix depends on _GNU_SOURCE
and the 64bit abi is different on musl
(see ut_session and ut_tv)

> diff --git a/include/utmp.h b/include/utmp.h
> index b145a11..c24d0a8 100644
> --- a/include/utmp.h
> +++ b/include/utmp.h
> @@ -19,9 +19,6 @@ struct lastlog {
>  
>  #define ut_time ut_tv.tv_sec
>  #define ut_name ut_user
> -#define ut_exit __ut_exit
> -#define e_termination __e_termination
> -#define e_exit __e_exit
>  #define utmp utmpx
>  #define utmpname(x) (-1)
>  

Re: Fix for fields in utmp

[Rich Felker]

On Wed, Feb 20, 2013 at 07:49:37PM +0100, Szabolcs Nagy wrote:
> * Chris Spiegel <cspiegel@gmail.com> [2013-02-20 10:23:09 -0800]:
> > The utmp.h header defines a few macros to access __ut_exit in the utmpx
> > struct; however, the underscores were removed in utmpx, so the macros
> > now point to non-existent struct members.  Attached is a simple patch
> > which removes them.
> 
> note that the current code is not compatible with glibc
> 
> http://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/gnu/bits/utmpx.h
> vs
> http://git.musl-libc.org/cgit/musl/tree/include/utmpx.h
> 
> it seems the __ prefix depends on _GNU_SOURCE

The versions with the __ prefix are never intended to be accessed by
name; they're just there to get the padding right. If some programs do
use them, however, we might need to provide them... BTW this is all
stub code; musl does not use utmp.

> and the 64bit abi is different on musl
> (see ut_session and ut_tv)

That should be fixed. Since it's all stubs, I don't think ABI is a big
issue here.

Rich

Re: Fix for fields in utmp

[Rich Felker]

On Wed, Feb 20, 2013 at 07:56:35PM -0500, Rich Felker wrote:
> On Wed, Feb 20, 2013 at 07:49:37PM +0100, Szabolcs Nagy wrote:
> > * Chris Spiegel <cspiegel@gmail.com> [2013-02-20 10:23:09 -0800]:
> > > The utmp.h header defines a few macros to access __ut_exit in the utmpx
> > > struct; however, the underscores were removed in utmpx, so the macros
> > > now point to non-existent struct members.  Attached is a simple patch
> > > which removes them.
> > 
> > note that the current code is not compatible with glibc
> > 
> > http://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/gnu/bits/utmpx.h
> > vs
> > http://git.musl-libc.org/cgit/musl/tree/include/utmpx.h
> > 
> > it seems the __ prefix depends on _GNU_SOURCE
> 
> The versions with the __ prefix are never intended to be accessed by
> name; they're just there to get the padding right. If some programs do
> use them, however, we might need to provide them... BTW this is all
> stub code; musl does not use utmp.

Committed. If anyone demonstrates a need for compatibility with the
__-prefixed names, we can discuss supporting them too, but this is all
really just ugly cruft...

> > and the 64bit abi is different on musl
> > (see ut_session and ut_tv)
> 
> That should be fixed. Since it's all stubs, I don't think ABI is a big
> issue here.

Can you elaborate on what needs to be changed here?

Rich

Re: Fix for fields in utmp

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-02-26 01:49:29 -0500]:
> On Wed, Feb 20, 2013 at 07:56:35PM -0500, Rich Felker wrote:
> > On Wed, Feb 20, 2013 at 07:49:37PM +0100, Szabolcs Nagy wrote:
> > > and the 64bit abi is different on musl
> > > (see ut_session and ut_tv)
> > 
> > That should be fixed. Since it's all stubs, I don't think ABI is a big
> > issue here.
> 
> Can you elaborate on what needs to be changed here?
> 

from the code it seemed they have ifdefs for 64bit

i think someone should run

http://nsz.repo.hu/git/?p=musl-tables;a=blob;f=sizeof.c

on x86_64 with glibc and musl and report back the
outputs (you can git clone and make sizeof)
(it may be useful to do this other archs as well)

Re: Fix for fields in utmp

[Ivan Kanakarakis]

[-- Attachment #1: Type: text/plain, Size: 3508 bytes --]

here is the output from musl-tables' sizeof test (also attached).
I had to comment out 'elf_fpxregset_t' to make it build.
this is run with latest musl from git (HEAD e201d82)


Linux suicidemachine 3.7.5-1-ARCH #1 SMP PREEMPT Mon Jan 28 10:03:32
CET 2013 x86_64 GNU/Linux
----------------------------------
diff --git a/sizeof.c b/sizeof.c
index f334769..47a7790 100644
--- a/sizeof.c
+++ b/sizeof.c
@@ -252,7 +252,7 @@ p(div_t)
 p(double)
 p(double_t)
 p(elf_fpregset_t)
-p(elf_fpxregset_t)
+// FIXME p(elf_fpxregset_t)
 p(elf_greg_t)
 p(elf_gregset_t)
 p(epoll_data_t)
@@ -551,7 +551,7 @@ p(struct ucred)
 p(struct udphdr)
 p(struct user)
 p(struct user_fpregs_struct)
-p(struct user_fpxregs_struct)
+// FIXME p(struct user_fpxregs_struct)
 p(struct user_regs_struct)
 p(struct utimbuf)
 p(struct utmpx)
----------------------------------
./sizeof-glibc >data/glibc.sizeof
./sizeof-musl >data/musl.sizeof
diff -U1 data/glibc.sizeof data/musl.sizeof >data/sizeof.diff || true
----------------------------------
--- 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
 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
@@ -122,3 +122,3 @@
 intptr_t 8
-jmp_buf 200
+jmp_buf 64
 key_t 4
@@ -181,6 +181,6 @@
 quad_t 8
-regex_t 64
+regex_t 56
 register_t 8
-regmatch_t 8
-regoff_t 4
+regmatch_t 16
+regoff_t 8
 res_state 8
@@ -196,3 +196,3 @@
 sighandler_t 8
-siginfo_t 128
+siginfo_t 136
 sigjmp_buf 200
@@ -215,3 +215,3 @@
 struct ar_hdr 60
-struct arpd_request 40
+struct arpd_request 28
 struct arphdr 8
@@ -222,3 +222,3 @@
 struct cmsghdr 16
-struct crypt_data 131232
+struct crypt_data 260
 struct dirent 280
@@ -282,3 +282,3 @@
 struct itimerval 32
-struct lastlog 292
+struct lastlog 296
 struct lconv 96
@@ -312,3 +312,3 @@
 struct ns_tsig_key 2072
-struct ntptimeval 72
+struct ntptimeval 32
 struct option 32
@@ -326,4 +326,4 @@
 struct rtentry 120
-struct rusage 144
-struct sched_param 4
+struct rusage 272
+struct sched_param 48
 struct sembuf 6
@@ -348,3 +348,3 @@
 struct sockaddr_ll 20
-struct sockaddr_storage 128
+struct sockaddr_storage 136
 struct sockaddr_un 110
@@ -361,3 +361,3 @@
 struct strrecvfd 20
-struct sysinfo 112
+struct sysinfo 368
 struct termios 60
@@ -376,3 +376,3 @@
 struct utimbuf 16
-struct utmpx 384
+struct utmpx 400
 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
@@ -416,4 +416,4 @@
 wchar_t 4
-wctrans_t 8
-wctype_t 8
+wctrans_t 4
+wctype_t 4
 wint_t 4



On 26 February 2013 11:22, Szabolcs Nagy <nsz@port70.net> wrote:
> * Rich Felker <dalias@aerifal.cx> [2013-02-26 01:49:29 -0500]:
>> On Wed, Feb 20, 2013 at 07:56:35PM -0500, Rich Felker wrote:
>> > On Wed, Feb 20, 2013 at 07:49:37PM +0100, Szabolcs Nagy wrote:
>> > > and the 64bit abi is different on musl
>> > > (see ut_session and ut_tv)
>> >
>> > That should be fixed. Since it's all stubs, I don't think ABI is a big
>> > issue here.
>>
>> Can you elaborate on what needs to be changed here?
>>
>
> from the code it seemed they have ifdefs for 64bit
>
> i think someone should run
>
> http://nsz.repo.hu/git/?p=musl-tables;a=blob;f=sizeof.c
>
> on x86_64 with glibc and musl and report back the
> outputs (you can git clone and make sizeof)
> (it may be useful to do this other archs as well)



--
Ivan c00kiemon5ter V Kanakarakis  >:3

[-- Attachment #2: report.txt --]
[-- Type: text/plain, Size: 2484 bytes --]

Linux suicidemachine 3.7.5-1-ARCH #1 SMP PREEMPT Mon Jan 28 10:03:32 CET 2013 x86_64 GNU/Linux
----------------------------------
diff --git a/sizeof.c b/sizeof.c
index f334769..47a7790 100644
--- a/sizeof.c
+++ b/sizeof.c
@@ -252,7 +252,7 @@ p(div_t)
 p(double)
 p(double_t)
 p(elf_fpregset_t)
-p(elf_fpxregset_t)
+// FIXME p(elf_fpxregset_t)
 p(elf_greg_t)
 p(elf_gregset_t)
 p(epoll_data_t)
@@ -551,7 +551,7 @@ p(struct ucred)
 p(struct udphdr)
 p(struct user)
 p(struct user_fpregs_struct)
-p(struct user_fpxregs_struct)
+// FIXME p(struct user_fpxregs_struct)
 p(struct user_regs_struct)
 p(struct utimbuf)
 p(struct utmpx)
----------------------------------
./sizeof-glibc >data/glibc.sizeof
./sizeof-musl >data/musl.sizeof
diff -U1 data/glibc.sizeof data/musl.sizeof >data/sizeof.diff || true
----------------------------------
--- 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
 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
@@ -122,3 +122,3 @@
 intptr_t	8
-jmp_buf	200
+jmp_buf	64
 key_t	4
@@ -181,6 +181,6 @@
 quad_t	8
-regex_t	64
+regex_t	56
 register_t	8
-regmatch_t	8
-regoff_t	4
+regmatch_t	16
+regoff_t	8
 res_state	8
@@ -196,3 +196,3 @@
 sighandler_t	8
-siginfo_t	128
+siginfo_t	136
 sigjmp_buf	200
@@ -215,3 +215,3 @@
 struct ar_hdr	60
-struct arpd_request	40
+struct arpd_request	28
 struct arphdr	8
@@ -222,3 +222,3 @@
 struct cmsghdr	16
-struct crypt_data	131232
+struct crypt_data	260
 struct dirent	280
@@ -282,3 +282,3 @@
 struct itimerval	32
-struct lastlog	292
+struct lastlog	296
 struct lconv	96
@@ -312,3 +312,3 @@
 struct ns_tsig_key	2072
-struct ntptimeval	72
+struct ntptimeval	32
 struct option	32
@@ -326,4 +326,4 @@
 struct rtentry	120
-struct rusage	144
-struct sched_param	4
+struct rusage	272
+struct sched_param	48
 struct sembuf	6
@@ -348,3 +348,3 @@
 struct sockaddr_ll	20
-struct sockaddr_storage	128
+struct sockaddr_storage	136
 struct sockaddr_un	110
@@ -361,3 +361,3 @@
 struct strrecvfd	20
-struct sysinfo	112
+struct sysinfo	368
 struct termios	60
@@ -376,3 +376,3 @@
 struct utimbuf	16
-struct utmpx	384
+struct utmpx	400
 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
@@ -416,4 +416,4 @@
 wchar_t	4
-wctrans_t	8
-wctype_t	8
+wctrans_t	4
+wctype_t	4
 wint_t	4

Re: Fix for fields in utmp

[Szabolcs Nagy]

see inline comments

* Ivan Kanakarakis <ivan.kanak@gmail.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

>  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

> @@ -122,3 +122,3 @@
>  intptr_t 8
> -jmp_buf 200
> +jmp_buf 64
>  key_t 4
> @@ -181,6 +181,6 @@
>  quad_t 8
> -regex_t 64
> +regex_t 56
>  register_t 8
> -regmatch_t 8
> -regoff_t 4
> +regmatch_t 16
> +regoff_t 8
>  res_state 8
> @@ -196,3 +196,3 @@
>  sighandler_t 8
> -siginfo_t 128
> +siginfo_t 136
>  sigjmp_buf 200
> @@ -215,3 +215,3 @@
>  struct ar_hdr 60
> -struct arpd_request 40
> +struct arpd_request 28
>  struct arphdr 8
> @@ -222,3 +222,3 @@
>  struct cmsghdr 16
> -struct crypt_data 131232
> +struct crypt_data 260
>  struct dirent 280
> @@ -282,3 +282,3 @@
>  struct itimerval 32
> -struct lastlog 292
> +struct lastlog 296
>  struct lconv 96
> @@ -312,3 +312,3 @@
>  struct ns_tsig_key 2072
> -struct ntptimeval 72
> +struct ntptimeval 32
>  struct option 32
> @@ -326,4 +326,4 @@
>  struct rtentry 120
> -struct rusage 144
> -struct sched_param 4
> +struct rusage 272
> +struct sched_param 48
>  struct sembuf 6
> @@ -348,3 +348,3 @@
>  struct sockaddr_ll 20
> -struct sockaddr_storage 128
> +struct sockaddr_storage 136
>  struct sockaddr_un 110
> @@ -361,3 +361,3 @@
>  struct strrecvfd 20
> -struct sysinfo 112
> +struct sysinfo 368
>  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.  */

>  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
> @@ -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

Re: Fix for fields in utmp

[Rich Felker]

On Tue, Feb 26, 2013 at 09:21:12PM +0100, Szabolcs Nagy wrote:
> see inline comments
> 
> * Ivan Kanakarakis <ivan.kanak@gmail.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

Re: Fix for fields in utmp

[Szabolcs Nagy]

* Rich Felker <dalias@aerifal.cx> [2013-02-26 18:33:42 -0500]:
> On Tue, Feb 26, 2013 at 09:21:12PM +0100, Szabolcs Nagy wrote:
> > * Ivan Kanakarakis <ivan.kanak@gmail.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..

i dont know, so current solution is ok

> > > -int_fast16_t 8
> > > -int_fast32_t 8
> > > +int_fast16_t 4
> > > +int_fast32_t 4
> > 
> > these diffs may cause problems
> 
> These are actually omitted from the ABI documents if I remember

ok then keep it as is

> > > -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.

ok

> > > -regex_t 64
> > > +regex_t 56
> 
> I think this should be fixed to match.

glibc's definition is something like:

typedef struct {
	char *__x1;
	long __x2;
	long __x3;
	long __x4;
	char *__x5;
	char *__x6;
	size_t re_nsub;
	char __x7;
} regex_t;

musl's definition (only re_nsub and __opaque are used):

typedef struct {
	size_t re_nsub;
	void *__opaque, *__padding[4];
	size_t __nsub2;
} regex_t;


> > > -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().

so regex abi wont be compatible

> > > -siginfo_t 128
> > > +siginfo_t 136
> 
> This is probably a mistake, maybe in the padding..?

yes, the si_fields union is 8byte aligned

glibc's definition (there are some bits left out here and
i think their definition is incorrectly 136 bytes on x32):

struct siginfo {
	int si_signo;
	int si_errno;
	int si_code;
	union {
#if 64bit
		int _pad[128/sizeof(int) - 4];
#elif 32bit
		int _pad[128/sizeof(int) - 3];
#endif
		/* ... */
	} _sifields;
} siginfo_t;


musl's definition:

struct __siginfo {
	int si_signo, si_errno, si_code;
	union {
		char __pad[128 - 3*sizeof(int)];
		/* ... */
	} __si_fields;
};

> > > -struct arpd_request 40
> > > +struct arpd_request 28
> 
> No idea.

i think this is not used anymore

(it seems there used to be some ioctl command a
userspace arpd might used on linux but the git history
does not go back far enough to tell exactly when it
was dropped and even then it was only used by said
arpd)

glibc's definition:

struct arpd_request {
	unsigned short int req;
	u_int32_t ip;
	unsigned long int dev;
	unsigned long int stamp;
	unsigned long int updated;
	unsigned char ha[7];
};

musl's definition:

struct arpd_request {
	uint16_t req;
	uint32_t ip;
	uint32_t dev;
	uint32_t stamp;
	uint32_t updated;
	uint8_t ha[7];
};

> > > -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.

ok

> > > -struct lastlog 292
> > > +struct lastlog 296
> 
> Probably needs to be checked.

glibc's definition:
(using the same hack for time_t as in utmpx)

struct lastlog {
	int32_t ll_time;
	char ll_line[32];
	char ll_host[256];
};

musl's definition:

struct lastlog {
	time_t ll_time;
	char ll_line[32];
	char ll_host[256];
};

> > > -struct ntptimeval 72
> > > +struct ntptimeval 32
> 
> Should we change this? Probably.

musl does not have ntp_gettime so it probably
should not define this struct

(it is a syscall in bsds and various other unices
with a struct ntptimeval* argument, glibc emulates
this with the linux adjtimex syscall, but probably
this should be done in ntpdate which already has
configure checks for ntp_gettime and struct ntptimeval
and implements its own when they are not available)

(the sizeof ntptimeval has changed and it is
only 72 byte when the NTP_API macro is 4, musl's
definition is the NTP_API 3)

> > > -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).

musl's rusage (the difference is the __reserved part):

struct rusage
{
	struct timeval ru_utime;
	struct timeval ru_stime;
	/* linux extentions, but useful */
	long	ru_maxrss;
	long	ru_ixrss;
	long	ru_idrss;
	long	ru_isrss;
	long	ru_minflt;
	long	ru_majflt;
	long	ru_nswap;
	long	ru_inblock;
	long	ru_oublock;
	long	ru_msgsnd;
	long	ru_msgrcv;
	long	ru_nsignals;
	long	ru_nvcsw;
	long	ru_nivcsw;
	/* room for more... */
	long    __reserved[16];
};

> > > -struct sockaddr_storage 128
> > > +struct sockaddr_storage 136
> 
> Probably wrong.

wrong padding

glibc's definition:

struct sockaddr_storage
{
	sa_family_t ss_family;
	unsigned long int __ss_align;
	char __ss_padding[(128 - (2 * sizeof (unsigned long int)))];
};

musl's definition:

struct sockaddr_storage
{
	sa_family_t ss_family;
	union {
		long long __align;
		char __padding[126];
	} __padding;
};

> > > -struct sysinfo 112
> > > +struct sysinfo 368
> 
> Just extra padding for growth. Maybe it should be changed.

> > > -struct utmpx 384
> > > +struct utmpx 400
> > 
> > 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.

ok

> > > -uint_fast16_t 8
> > > -uint_fast32_t 8
> > > +uint_fast16_t 4
> > > +uint_fast32_t 4
> 
> See above.

> > > -wctrans_t 8
> > > -wctype_t 8
> > > +wctrans_t 4
> > > +wctype_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.

then this should be fixed

Re: Fix for fields in utmp

[Rich Felker]

On Sun, Mar 03, 2013 at 12:29:57AM +0100, Szabolcs Nagy wrote:
> > > > -int_fast16_t 8
> > > > -int_fast32_t 8
> > > > +int_fast16_t 4
> > > > +int_fast32_t 4
> > > 
> > > these diffs may cause problems
> > 
> > These are actually omitted from the ABI documents if I remember
> 
> ok then keep it as is

This still should be checked. It _could_ be changed if it matters..

> > > > -regex_t 64
> > > > +regex_t 56
> > 
> > I think this should be fixed to match.
> 
> glibc's definition is something like:
> 
> typedef struct {
> 	char *__x1;
> 	long __x2;
> 	long __x3;
> 	long __x4;
> 	char *__x5;
> 	char *__x6;
> 	size_t re_nsub;
> 	char __x7;
> } regex_t;
> 
> musl's definition (only re_nsub and __opaque are used):
> 
> typedef struct {
> 	size_t re_nsub;
> 	void *__opaque, *__padding[4];
> 	size_t __nsub2;
> } regex_t;

Fixed, even though it doesn't matter with the other ABI
incompatibility issues.

> > > > -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().
> 
> so regex abi wont be compatible

Yes, this will need some special dynamic linker glue if we want to be
able to run glibc libs/bins using regex. But it's likely they might
not work anyway without glibc extensions...

> > > > -siginfo_t 128
> > > > +siginfo_t 136
> > 
> > This is probably a mistake, maybe in the padding..?
> 
> yes, the si_fields union is 8byte aligned

Fixed.

> > > > -struct arpd_request 40
> > > > +struct arpd_request 28
> > 
> > No idea.
> 
> i think this is not used anymore

Fixed anyway.

> > > > -struct lastlog 292
> > > > +struct lastlog 296
> > 
> > Probably needs to be checked.
> 
> glibc's definition:
> (using the same hack for time_t as in utmpx)

This can probably be ignored. The files will be incompatible, but it's
intended that they not be used anyway..

> > > > -struct ntptimeval 72
> > > > +struct ntptimeval 32
> > 
> > Should we change this? Probably.
> 
> musl does not have ntp_gettime so it probably
> should not define this struct

You're probably right, but I'm holding off a bit on removing it in
case others have input..

> > > > -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).
> 
> musl's rusage (the difference is the __reserved part):

Should we change it or keep the extra reserved space?

> > > > -struct sockaddr_storage 128
> > > > +struct sockaddr_storage 136
> > 
> > Probably wrong.
> 
> wrong padding

Fixed.

> > > > -wctrans_t 8
> > > > -wctype_t 8
> > > > +wctrans_t 4
> > > > +wctype_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.
> 
> then this should be fixed

I will fix this, but as a separate commit since it actually has
nontrivial effects.

Rich

Re: Fix for fields in utmp

[Ivan Kanakarakis]

[-- Attachment #1.1: Type: text/plain, Size: 4881 bytes --]

I update musl-git and rerun nsz's test, here are the current results
in case you see something to be fixed before the release..

--- data/glibc.sizeof 2013-04-12 14:01:35.768425683 +0300
+++ data/musl.sizeof 2013-04-12 14:01:35.771759052 +0300
@@ -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
@@ -122,3 +122,3 @@
 intptr_t 8
-jmp_buf 200
+jmp_buf 64
 key_t 4
@@ -183,4 +183,4 @@
 register_t 8
-regmatch_t 8
-regoff_t 4
+regmatch_t 16
+regoff_t 8
 res_state 8
@@ -222,3 +222,3 @@
 struct cmsghdr 16
-struct crypt_data 131232
+struct crypt_data 260
 struct dirent 280
@@ -282,3 +282,3 @@
 struct itimerval 32
-struct lastlog 292
+struct lastlog 296
 struct lconv 96
@@ -312,3 +312,3 @@
 struct ns_tsig_key 2072
-struct ntptimeval 72
+struct ntptimeval 32
 struct option 32
@@ -326,4 +326,4 @@
 struct rtentry 120
-struct rusage 144
-struct sched_param 4
+struct rusage 272
+struct sched_param 48
 struct sembuf 6
@@ -361,3 +361,3 @@
 struct strrecvfd 20
-struct sysinfo 112
+struct sysinfo 368
 struct termios 60
@@ -376,3 +376,3 @@
 struct utimbuf 16
-struct utmpx 384
+struct utmpx 400
 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


also attached the diff



On 5 March 2013 00:06, Rich Felker <dalias@aerifal.cx> wrote:

> On Sun, Mar 03, 2013 at 12:29:57AM +0100, Szabolcs Nagy wrote:
> > > > > -int_fast16_t 8
> > > > > -int_fast32_t 8
> > > > > +int_fast16_t 4
> > > > > +int_fast32_t 4
> > > >
> > > > these diffs may cause problems
> > >
> > > These are actually omitted from the ABI documents if I remember
> >
> > ok then keep it as is
>
> This still should be checked. It _could_ be changed if it matters..
>
> > > > > -regex_t 64
> > > > > +regex_t 56
> > >
> > > I think this should be fixed to match.
> >
> > glibc's definition is something like:
> >
> > typedef struct {
> >       char *__x1;
> >       long __x2;
> >       long __x3;
> >       long __x4;
> >       char *__x5;
> >       char *__x6;
> >       size_t re_nsub;
> >       char __x7;
> > } regex_t;
> >
> > musl's definition (only re_nsub and __opaque are used):
> >
> > typedef struct {
> >       size_t re_nsub;
> >       void *__opaque, *__padding[4];
> >       size_t __nsub2;
> > } regex_t;
>
> Fixed, even though it doesn't matter with the other ABI
> incompatibility issues.
>
> > > > > -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().
> >
> > so regex abi wont be compatible
>
> Yes, this will need some special dynamic linker glue if we want to be
> able to run glibc libs/bins using regex. But it's likely they might
> not work anyway without glibc extensions...
>
> > > > > -siginfo_t 128
> > > > > +siginfo_t 136
> > >
> > > This is probably a mistake, maybe in the padding..?
> >
> > yes, the si_fields union is 8byte aligned
>
> Fixed.
>
> > > > > -struct arpd_request 40
> > > > > +struct arpd_request 28
> > >
> > > No idea.
> >
> > i think this is not used anymore
>
> Fixed anyway.
>
> > > > > -struct lastlog 292
> > > > > +struct lastlog 296
> > >
> > > Probably needs to be checked.
> >
> > glibc's definition:
> > (using the same hack for time_t as in utmpx)
>
> This can probably be ignored. The files will be incompatible, but it's
> intended that they not be used anyway..
>
> > > > > -struct ntptimeval 72
> > > > > +struct ntptimeval 32
> > >
> > > Should we change this? Probably.
> >
> > musl does not have ntp_gettime so it probably
> > should not define this struct
>
> You're probably right, but I'm holding off a bit on removing it in
> case others have input..
>
> > > > > -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).
> >
> > musl's rusage (the difference is the __reserved part):
>
> Should we change it or keep the extra reserved space?
>
> > > > > -struct sockaddr_storage 128
> > > > > +struct sockaddr_storage 136
> > >
> > > Probably wrong.
> >
> > wrong padding
>
> Fixed.
>
> > > > > -wctrans_t 8
> > > > > -wctype_t 8
> > > > > +wctrans_t 4
> > > > > +wctype_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.
> >
> > then this should be fixed
>
> I will fix this, but as a separate commit since it actually has
> nontrivial effects.
>
> Rich
>



-- 
*Ivan c00kiemon5ter Kanakarakis*  >:3

[-- Attachment #1.2: Type: text/html, Size: 14380 bytes --]

[-- Attachment #2: sizeof.diff --]
[-- Type: application/octet-stream, Size: 1154 bytes --]

--- data/glibc.sizeof	2013-04-12 14:01:35.768425683 +0300
+++ data/musl.sizeof	2013-04-12 14:01:35.771759052 +0300
@@ -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
@@ -122,3 +122,3 @@
 intptr_t	8
-jmp_buf	200
+jmp_buf	64
 key_t	4
@@ -183,4 +183,4 @@
 register_t	8
-regmatch_t	8
-regoff_t	4
+regmatch_t	16
+regoff_t	8
 res_state	8
@@ -222,3 +222,3 @@
 struct cmsghdr	16
-struct crypt_data	131232
+struct crypt_data	260
 struct dirent	280
@@ -282,3 +282,3 @@
 struct itimerval	32
-struct lastlog	292
+struct lastlog	296
 struct lconv	96
@@ -312,3 +312,3 @@
 struct ns_tsig_key	2072
-struct ntptimeval	72
+struct ntptimeval	32
 struct option	32
@@ -326,4 +326,4 @@
 struct rtentry	120
-struct rusage	144
-struct sched_param	4
+struct rusage	272
+struct sched_param	48
 struct sembuf	6
@@ -361,3 +361,3 @@
 struct strrecvfd	20
-struct sysinfo	112
+struct sysinfo	368
 struct termios	60
@@ -376,3 +376,3 @@
 struct utimbuf	16
-struct utmpx	384
+struct utmpx	400
 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