linux-user: Use correct alignment for long long on i386 guests

Commit Message

Laurent Vivier July 28, 2016, 8:36 p.m. UTC

Le 28/07/2016 à 13:57, Peter Maydell a écrit :
> For i386, the ABI specifies that 'long long' (8 byte values)
> need only be 4 aligned, but we were requiring them to be
> 8-aligned. This meant we were laying out the target_epoll_event
> structure wrongly. Add a suitable ifdef to abitypes.h to
> specify the i386-specific alignment requirement.

gdb qemu-i386
(gdb) p &(((struct target_epoll_event *)0)->data)
$1 = (target_epoll_data_t *) 0x8

whereas:

gdb qemu-x86_64
(gdb) p &(((struct target_epoll_event *)0)->data)
$1 = (target_epoll_data_t *) 0x4

I've checked on real systems x86_64/i386:
-----
#include <sys/epoll.h>

int main(void)
{
    volatile struct epoll_event e;

    e.events = 0;
}
----
(gdb) p &(((struct epoll_event *)0)->data)
$1 = (epoll_data_t *) 0x4

but on ppc64, I have

(gdb) p &(((struct epoll_event *)0)->data)
$1 = (epoll_data_t *) 0x8

In fact, the structure should be packed in both cases, something like:

 #define TARGET_EPOLL_PACKED

on my Fedora systems x86_64/i386:

/usr/include/bits/epoll.h

#define __EPOLL_PACKED __attribute__ ((__packed__))

/usr/include/sys/epoll.h

struct epoll_event
{
  uint32_t events;      /* Epoll events */
  epoll_data_t data;    /* User data variable */
} __EPOLL_PACKED;

but I don't understand why in linux source tree we have

#ifdef __x86_64__
#define EPOLL_PACKED __attribute__((packed))
#else
#define EPOLL_PACKED
#endif

struct epoll_event {
        __u32 events;
        __u64 data;
} EPOLL_PACKED;

Laurent

Comments

Peter Maydell July 28, 2016, 8:43 p.m. UTC | #1

On 28 July 2016 at 21:36, Laurent Vivier <[e-mail]> wrote:
>
>
> Le 28/07/2016 à 13:57, Peter Maydell a écrit :
>> For i386, the ABI specifies that 'long long' (8 byte values)
>> need only be 4 aligned, but we were requiring them to be
>> 8-aligned. This meant we were laying out the target_epoll_event
>> structure wrongly. Add a suitable ifdef to abitypes.h to
>> specify the i386-specific alignment requirement.
>
> gdb qemu-i386
> (gdb) p &(((struct target_epoll_event *)0)->data)
> $1 = (target_epoll_data_t *) 0x8

This is wrong; perhaps the debug info doesn't correctly
include alignment requirements for types? (If you
print sizes/offsets in QEMU's C code I believe you get
different answers from what gdb is telling you here.)

> whereas:
>
> gdb qemu-x86_64
> (gdb) p &(((struct target_epoll_event *)0)->data)
> $1 = (target_epoll_data_t *) 0x4
>
> I've checked on real systems x86_64/i386:
> -----
> #include <sys/epoll.h>
>
> int main(void)
> {
>     volatile struct epoll_event e;
>
>     e.events = 0;
> }
> ----
> (gdb) p &(((struct epoll_event *)0)->data)
> $1 = (epoll_data_t *) 0x4
>
> but on ppc64, I have
>
> (gdb) p &(((struct epoll_event *)0)->data)
> $1 = (epoll_data_t *) 0x8

Yep; ppc64 doesn't pack the struct and it has an 8-align
requirement for 64-bit types.

> In fact, the structure should be packed in both cases, something like:
>
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2562,7 +2562,7 @@ struct target_mq_attr {
>  #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG |
> FUTEX_CLOCK_REALTIME)
>
>  #ifdef CONFIG_EPOLL
> -#if defined(TARGET_X86_64)
> +#if defined(TARGET_X86_64) || defined(TARGET_I386)
>  #define TARGET_EPOLL_PACKED QEMU_PACKED
>  #else
>  #define TARGET_EPOLL_PACKED

If we have the correctly defined alignment requirement
on the target 64-bit type, then you don't need this.
(See the kernel sources you quote below which only
provide the packed attribute on x86-64, not on i386).

> on my Fedora systems x86_64/i386:
>
> /usr/include/bits/epoll.h
>
> #define __EPOLL_PACKED __attribute__ ((__packed__))
>
> /usr/include/sys/epoll.h
>
> struct epoll_event
> {
>   uint32_t events;      /* Epoll events */
>   epoll_data_t data;    /* User data variable */
> } __EPOLL_PACKED;

These are the libc data structures, which aren't
defined the same way as the kernel ones.

> but I don't understand why in linux source tree we have
>
> #ifdef __x86_64__
> #define EPOLL_PACKED __attribute__((packed))
> #else
> #define EPOLL_PACKED
> #endif
>
> struct epoll_event {
>         __u32 events;
>         __u64 data;
> } EPOLL_PACKED;

This is the kernel structure. x86-64 is a weird outlier
because they wanted to make the structure layout identical
to i386 (to ease the compat-syscall implementation).
Every other arch just lets the struct be naturally laid
out for the types it contains.

thanks
-- PMM

Patch

--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -2562,7 +2562,7 @@  struct target_mq_attr {
 #define FUTEX_CMD_MASK          ~(FUTEX_PRIVATE_FLAG |
FUTEX_CLOCK_REALTIME)

 #ifdef CONFIG_EPOLL
-#if defined(TARGET_X86_64)
+#if defined(TARGET_X86_64) || defined(TARGET_I386)
 #define TARGET_EPOLL_PACKED QEMU_PACKED
 #else