| Message ID | 1469707079-9049-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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>
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 >
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
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
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(+)