[Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Peter Maydell]

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.

Reported-by: Icenowy Zheng <icenowy@aosc.xyz>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/user/abitypes.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
index a09d6c6..ba18860 100644
--- a/include/exec/user/abitypes.h
+++ b/include/exec/user/abitypes.h
@@ -15,6 +15,10 @@
 #define ABI_LLONG_ALIGNMENT 2
 #endif
 
+#if defined(TARGET_I386) && !defined(TARGET_X86_64)
+#define ABI_LLONG_ALIGNMENT 4
+#endif
+
 #ifndef ABI_SHORT_ALIGNMENT
 #define ABI_SHORT_ALIGNMENT 2
 #endif
-- 
1.9.1

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Laurent Vivier]

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.
> 
> Reported-by: Icenowy Zheng <icenowy@aosc.xyz>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/user/abitypes.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
> index a09d6c6..ba18860 100644
> --- a/include/exec/user/abitypes.h
> +++ b/include/exec/user/abitypes.h
> @@ -15,6 +15,10 @@
>  #define ABI_LLONG_ALIGNMENT 2
>  #endif
>  
> +#if defined(TARGET_I386) && !defined(TARGET_X86_64)
> +#define ABI_LLONG_ALIGNMENT 4
> +#endif
> +
>  #ifndef ABI_SHORT_ALIGNMENT
>  #define ABI_SHORT_ALIGNMENT 2
>  #endif
> 

Why the following program from commit

    c2e3dee linux-user: Define target alignment size

int main(void)
{
    printf("alignof(short) %ld\n", __alignof__(short));
    printf("alignof(int) %ld\n", __alignof__(int));
    printf("alignof(long) %ld\n", __alignof__(long));
    printf("alignof(long long) %ld\n", __alignof__(long long));
}


gives me:

alignof(short) 2
alignof(int) 4
alignof(long) 4
alignof(long long) 8

?

I compile it on x86_64 in 32bit mode:

cc -m32 -o align align.c
file align
align: ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV),
dynamically linked, interpreter /lib/ld-linux.so.2, for GNU/Linux
2.6.32, BuildID[sha1]=f5312f2334462b48fd9764e78a845879ff317b94, not stripped

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Peter Maydell]

On 28 July 2016 at 20:19, Laurent Vivier <laurent@vivier.eu> wrote:
> Why the following program from commit
>
>     c2e3dee linux-user: Define target alignment size
>
> int main(void)
> {
>     printf("alignof(short) %ld\n", __alignof__(short));
>     printf("alignof(int) %ld\n", __alignof__(int));
>     printf("alignof(long) %ld\n", __alignof__(long));
>     printf("alignof(long long) %ld\n", __alignof__(long long));
> }
>
>
> gives me:
>
> alignof(short) 2
> alignof(int) 4
> alignof(long) 4
> alignof(long long) 8
>
> ?

Because gcc __alignof__ gives you the maximum preferred
alignment, not the minimum alignment. If you want to know
what the alignment used for setting struct alignment is,
use C99 alignof from stdalign.h and/or cross-check by
using sizeof() on a struct with the type in it:

orth$ uname -a
Linux orth 3.16.0-4-686-pae #1 SMP Debian 3.16.7-ckt20-1+deb8u3
(2016-01-17) i686 GNU/Linux
orth$ cat /tmp/zz9.c
#include <stdio.h>
#include <stdalign.h>

struct epoll_event {
  int events;
  unsigned long long data;
};

#define PRINTALIGN(T) \
    printf(#T ": sizeof %zd __alignof__ %zd _Alignof %zd alignof %zd\n", \
    sizeof(T), __alignof__(T), _Alignof(T), alignof(T))

int main(void) {
    PRINTALIGN(unsigned long long);
    PRINTALIGN(struct epoll_event);
    return 0;
}
orth$ gcc -g -Wall -o /tmp/zz9 /tmp/zz9.c
orth$ /tmp/zz9
unsigned long long: sizeof 8 __alignof__ 8 _Alignof 4 alignof 4
struct epoll_event: sizeof 12 __alignof__ 4 _Alignof 4 alignof 4

Also watch out for broken clang versions:

orth$ clang -g -Wall -o /tmp/zz9clang /tmp/zz9.c
orth$ /tmp/zz9clang
unsigned long long: sizeof 8 __alignof__ 8 _Alignof 8 alignof 8
struct epoll_event: sizeof 12 __alignof__ 4 _Alignof 4 alignof 4
orth$ clang --version
Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0)
Target: i386-pc-linux-gnu
Thread model: posix

(that's fixed in later clang versions)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Laurent Vivier]

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:

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

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

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Peter Maydell]

On 28 July 2016 at 21:36, Laurent Vivier <laurent@vivier.eu> 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

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Laurent Vivier]

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.
> 
> Reported-by: Icenowy Zheng <icenowy@aosc.xyz>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/user/abitypes.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
> index a09d6c6..ba18860 100644
> --- a/include/exec/user/abitypes.h
> +++ b/include/exec/user/abitypes.h
> @@ -15,6 +15,10 @@
>  #define ABI_LLONG_ALIGNMENT 2
>  #endif
>  
> +#if defined(TARGET_I386) && !defined(TARGET_X86_64)
> +#define ABI_LLONG_ALIGNMENT 4
> +#endif
> +
>  #ifndef ABI_SHORT_ALIGNMENT
>  #define ABI_SHORT_ALIGNMENT 2
>  #endif
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Riku Voipio]

On Thu, Jul 28, 2016 at 12:57:59PM +0100, Peter Maydell wrote:
> 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.

Thanks, applied all your patches upto this patch to:

https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/for-next

I take none of the patches are important enough to warrant including
in 2.7?

> Reported-by: Icenowy Zheng <icenowy@aosc.xyz>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/exec/user/abitypes.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/exec/user/abitypes.h b/include/exec/user/abitypes.h
> index a09d6c6..ba18860 100644
> --- a/include/exec/user/abitypes.h
> +++ b/include/exec/user/abitypes.h
> @@ -15,6 +15,10 @@
>  #define ABI_LLONG_ALIGNMENT 2
>  #endif
>  
> +#if defined(TARGET_I386) && !defined(TARGET_X86_64)
> +#define ABI_LLONG_ALIGNMENT 4
> +#endif
> +
>  #ifndef ABI_SHORT_ALIGNMENT
>  #define ABI_SHORT_ALIGNMENT 2
>  #endif
> -- 
> 1.9.1
> 

Re: [Qemu-devel] [PATCH] linux-user: Use correct alignment for long long on i386 guests

[Peter Maydell]

On 1 August 2016 at 10:04, Riku Voipio <riku.voipio@iki.fi> wrote:
> On Thu, Jul 28, 2016 at 12:57:59PM +0100, Peter Maydell wrote:
>> 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.
>
> Thanks, applied all your patches upto this patch to:
>
> https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/for-next
>
> I take none of the patches are important enough to warrant including
> in 2.7?

I think I would suggest at least these for 2.7:

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


 (fixes a real user-reported bug)
linux-user: Fix memchr() argument in open_self_cmdline()
linux-user: Don't write off end of new_utsname buffer
 (both buffer overruns that could plausibly happen)
linux-user: Fix target_semid_ds structure definition
 (sysv semaphore completely broken on many guest archs)
linux-user: Handle brk() attempts with very large sizes
 (because I'd like to be able to tell the gcc folks they
  can just test with QEMU 2.7)

with perhaps the rest of the coverity-fixes on the
maybe list.

thanks
-- PMM