[Qemu-devel] [RFC] Fix for random Qemu crashes
Here's an updated patch to fix the inlining problems that make some Qemu
targets crash randomly.
As we have at least one broken target in the CVS because of this bug
(and maybe more), we have an urgent need of a fix. I'll then commit this
patch today if there is no other fix proposed that actually solves the
problem.
--
J. Mayer [EMAIL PROTECTED]
Never organized
Index: exec-all.h
===
RCS file: /sources/qemu/qemu/exec-all.h,v
retrieving revision 1.70
diff -u -d -d -p -r1.70 exec-all.h
--- exec-all.h 4 Nov 2007 02:24:57 - 1.70
+++ exec-all.h 18 Nov 2007 15:44:16 -
@@ -21,36 +21,6 @@
/* allow to see translation results - the slowdown should be negligible, so we leave it */
#define DEBUG_DISAS
-#ifndef glue
-#define xglue(x, y) x ## y
-#define glue(x, y) xglue(x, y)
-#define stringify(s) tostring(s)
-#define tostring(s) #s
-#endif
-
-#ifndef likely
-#if __GNUC__ 3
-#define __builtin_expect(x, n) (x)
-#endif
-
-#define likely(x) __builtin_expect(!!(x), 1)
-#define unlikely(x) __builtin_expect(!!(x), 0)
-#endif
-
-#ifndef always_inline
-#if (__GNUC__ 3) || defined(__APPLE__)
-#define always_inline inline
-#else
-#define always_inline __attribute__ (( always_inline )) inline
-#endif
-#endif
-
-#ifdef __i386__
-#define REGPARM(n) __attribute((regparm(n)))
-#else
-#define REGPARM(n)
-#endif
-
/* is_jmp field values */
#define DISAS_NEXT0 /* next instruction can be analyzed */
#define DISAS_JUMP1 /* only pc was modified dynamically */
Index: osdep.h
===
RCS file: /sources/qemu/qemu/osdep.h,v
retrieving revision 1.10
diff -u -d -d -p -r1.10 osdep.h
--- osdep.h 7 Jun 2007 23:09:47 - 1.10
+++ osdep.h 18 Nov 2007 15:44:16 -
@@ -3,6 +3,44 @@
#include stdarg.h
+#ifndef glue
+#define xglue(x, y) x ## y
+#define glue(x, y) xglue(x, y)
+#define stringify(s) tostring(s)
+#define tostring(s) #s
+#endif
+
+#ifndef likely
+#if __GNUC__ 3
+#define __builtin_expect(x, n) (x)
+#endif
+
+#define likely(x) __builtin_expect(!!(x), 1)
+#define unlikely(x) __builtin_expect(!!(x), 0)
+#endif
+
+#ifndef MIN
+#define MIN(a, b) (((a) (b)) ? (a) : (b))
+#endif
+#ifndef MAX
+#define MAX(a, b) (((a) (b)) ? (a) : (b))
+#endif
+
+#ifndef always_inline
+#if (__GNUC__ 3) || defined(__APPLE__)
+#define always_inline inline
+#else
+#define always_inline __attribute__ (( always_inline )) __inline__
+#endif
+#endif
+#define inline always_inline
+
+#ifdef __i386__
+#define REGPARM(n) __attribute((regparm(n)))
+#else
+#define REGPARM(n)
+#endif
+
#define qemu_printf printf
void *qemu_malloc(size_t size);
Index: qemu-common.h
===
RCS file: /sources/qemu/qemu/qemu-common.h,v
retrieving revision 1.2
diff -u -d -d -p -r1.2 qemu-common.h
--- qemu-common.h 17 Nov 2007 17:14:38 - 1.2
+++ qemu-common.h 18 Nov 2007 15:44:16 -
@@ -62,37 +62,6 @@ static inline char *realpath(const char
#endif /* !defined(NEED_CPU_H) */
-#ifndef glue
-#define xglue(x, y) x ## y
-#define glue(x, y) xglue(x, y)
-#define stringify(s) tostring(s)
-#define tostring(s) #s
-#endif
-
-#ifndef likely
-#if __GNUC__ 3
-#define __builtin_expect(x, n) (x)
-#endif
-
-#define likely(x) __builtin_expect(!!(x), 1)
-#define unlikely(x) __builtin_expect(!!(x), 0)
-#endif
-
-#ifndef MIN
-#define MIN(a, b) (((a) (b)) ? (a) : (b))
-#endif
-#ifndef MAX
-#define MAX(a, b) (((a) (b)) ? (a) : (b))
-#endif
-
-#ifndef always_inline
-#if (__GNUC__ 3) || defined(__APPLE__)
-#define always_inline inline
-#else
-#define always_inline __attribute__ (( always_inline )) inline
-#endif
-#endif
-
/* bottom halves */
typedef struct QEMUBH QEMUBH;
Index: translate-op.c
===
RCS file: /sources/qemu/qemu/translate-op.c,v
retrieving revision 1.3
diff -u -d -d -p -r1.3 translate-op.c
--- translate-op.c 18 Nov 2007 01:44:36 - 1.3
+++ translate-op.c 18 Nov 2007 15:44:16 -
@@ -24,6 +24,7 @@
#include inttypes.h
#include config.h
+#include osdep.h
enum {
#define DEF(s, n, copy_size) INDEX_op_ ## s,
Index: darwin-user/qemu.h
===
RCS file: /sources/qemu/qemu/darwin-user/qemu.h,v
retrieving revision 1.1
diff -u -d -d -p -r1.1 qemu.h
--- darwin-user/qemu.h 18 Jan 2007 20:06:33 - 1.1
+++ darwin-user/qemu.h 18 Nov 2007 15:44:16 -
@@ -1,13 +1,13 @@
#ifndef GEMU_H
#define GEMU_H
-#include thunk.h
-
#include signal.h
#include string.h
#include cpu.h
+#include thunk.h
+
#include gdbstub.h
typedef siginfo_t target_siginfo_t;
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On Fri, 2007-11-16 at 17:06 +0200, Heikki Lindholm wrote: J. Mayer kirjoitti: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o Does -winline give a warning when this happens? I did not check this, but getting a warning won't help us a lot. The generated Qemu executable would still crash. -- Jocelyn Mayer [EMAIL PROTECTED]
Re: [Qemu-devel] RFC: fix for random Qemu crashes
Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file. Presumably we only need one of the last two patches? It seems rather pointless to have always_inline *and* change the inlining heuristics. I'm ok with using always_inline for op.o (and things it uses directly) as this is required for correctness. I'm not convinced that that using always_inline everywhere is such a good idea. Paul
Re: [Qemu-devel] RFC: fix for random Qemu crashes
Jocelyn Mayer kirjoitti: On Fri, 2007-11-16 at 17:06 +0200, Heikki Lindholm wrote: J. Mayer kirjoitti: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o Does -winline give a warning when this happens? I did not check this, but getting a warning won't help us a lot. The generated Qemu executable would still crash. But a warning makes the problem more noticable. I tried -Winline and it does give warnings, but I'm not sure whether I'm hitting the same behaviour you see. I silenced the lot with --param-max-inline-insns-single=4000 -- Heikki Lindholm
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On Fri, 2007-11-16 at 15:52 +, Paul Brook wrote: Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file. Presumably we only need one of the last two patches? It seems rather pointless to have always_inline *and* change the inlining heuristics. From the tests I made, it seems that adding always_inline helps but unfortunatelly does not solve all cases. Should check in the gcc source code why it is so... I'm ok with using always_inline for op.o (and things it uses directly) as this is required for correctness. I'm not convinced that that using always_inline everywhere is such a good idea. That's exactly what I did: I changed 'inline' to 'always_inline' in headers that are included by op.c, I did not made any change in other headers. If some of the functions I changed should not be changed, that means that there is another bug, ie those functions should not be used by op.c in any way then should be moved to other headers, or maybe some of those headers should not be included directly or indirectly in op.c... If you see such cases, then we may better fix those issues first... -- Jocelyn Mayer [EMAIL PROTECTED]
Re: [Qemu-devel] RFC: fix for random Qemu crashes
J. Mayer kirjoitti: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o Does -winline give a warning when this happens? -- Heikki Lindholm
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On Fri, 2007-11-16 at 15:59 +, Jamie Lokier wrote: Heikki Lindholm wrote: J. Mayer kirjoitti: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o Does -winline give a warning when this happens? And can it be repaired with __attribute__((__always_inline__))? As I already reported in the previous message, this helps solve the problem but is not sufficient. Please refer to the proposed patches and the rest of the discussion. Further invastigation also showed me that there may be problems while compiling translate-op.c. It seems the solution would be to change the gcc inlining limits into the BASE_CFLAGS too _and_ define functions in dyngen.h as always_inline. This would also avoid most inline related warnings during the compilation (but maybe not solve all cases, as seen in the op.c case). Reading gcc source code showed me there are cases where a function could be not inline when marked 'inline' without generating any warning message. What I don't know is if those cases are likely to happen during normal compilations, but as the op.c compilation seems to be buggy and never emit those warnings, I guess we can reach those cases. -- Jocelyn Mayer [EMAIL PROTECTED]
Re: [Qemu-devel] RFC: fix for random Qemu crashes
Heikki Lindholm wrote: J. Mayer kirjoitti: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o Does -winline give a warning when this happens? And can it be repaired with __attribute__((__always_inline__))? -- Jamie
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On 16/11/2007, Jocelyn Mayer [EMAIL PROTECTED] wrote: On Fri, 2007-11-16 at 15:52 +, Paul Brook wrote: Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file. Presumably we only need one of the last two patches? It seems rather pointless to have always_inline *and* change the inlining heuristics. From the tests I made, it seems that adding always_inline helps but unfortunatelly does not solve all cases. Should check in the gcc source code why it is so... I'm ok with using always_inline for op.o (and things it uses directly) as this is required for correctness. I'm not convinced that that using always_inline everywhere is such a good idea. That's exactly what I did: I changed 'inline' to 'always_inline' in headers that are included by op.c, I did not made any change in other headers. I think a line like #define inline __attribute__ (( always_inline )) inline in dyngen-exec.h should be enough? Regards
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On Fri, 2007-11-16 at 21:32 +0100, andrzej zaborowski wrote: On 16/11/2007, Jocelyn Mayer [EMAIL PROTECTED] wrote: On Fri, 2007-11-16 at 15:52 +, Paul Brook wrote: Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file. Presumably we only need one of the last two patches? It seems rather pointless to have always_inline *and* change the inlining heuristics. From the tests I made, it seems that adding always_inline helps but unfortunatelly does not solve all cases. Should check in the gcc source code why it is so... I'm ok with using always_inline for op.o (and things it uses directly) as this is required for correctness. I'm not convinced that that using always_inline everywhere is such a good idea. That's exactly what I did: I changed 'inline' to 'always_inline' in headers that are included by op.c, I did not made any change in other headers. I think a line like #define inline __attribute__ (( always_inline )) inline in dyngen-exec.h should be As I already pointed it in the first message of the thread, this kind of define would expand recursivelly, which is particullary ugly, and which can in some cases lead to compiler warnings or errors. I already had this kind of problems using the linux kernel headers which preciselly uses this definitition. But, once again, adding always_inline to functions does not completelly solve the problem (please read the thread !) or at least does not solves it with all gcc versions. The inline growth limits tweaking seems needed too. -- J. Mayer [EMAIL PROTECTED] Never organized
[Qemu-devel] RFC: fix for random Qemu crashes
Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o I found some workaround to avoid this behavior and I'd like to get opinions about it. The first idea is to change all occurences of 'inline' with 'always_inline' in all headers included in op.c. It then appeared to me that always_inline is not globally declared and that the definition is duplicated in vl.h and exec-all.h. But it's not declared in darwin-user/qemu.h and linux-user/qemu.h, which is not consistent with the declaration in vl.h. Further investigations showed me that the osdep.h header is the one that is actually included everywhere. Even if those are more compiler than OS dependent, I decided to move the definitions for glue, tostring, likely, unlikely, always_inline and REGPARM to this header so they can be globally used. I also changed the include orders in darwin-user/qemu.h to be sure this header will be included first. This patch is attached in common_defs.diff. Giving this patch, I've been able to replace all occurence of 'inline' with 'always_inline' in all headers included from op.c (given the generated .d file). Some would say I'd better add a #define inline always_inline somewhere. I personnally dislike this solution as this kind of macro as it tends to expand recursivally (always_inline definition contains the inline word) and this may lead to compilation warnings or errors in some context; one could do tests using the linux kernel headers to get convinced that it can happen. Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file.Unfortunatelly, there is no way to disable those limits (I checked in the source code), then I put them to an arbitrary high level. I also added the -funit-at-a-time switch, as this kind of optimisation would not be relevant in op.o context. The diff is attached in gcc_inline_limits.diff. Please comment. -- J. Mayer [EMAIL PROTECTED] Never organized Index: exec-all.h === RCS file: /sources/qemu/qemu/exec-all.h,v retrieving revision 1.70 diff -u -d -d -p -r1.70 exec-all.h --- exec-all.h 4 Nov 2007 02:24:57 - 1.70 +++ exec-all.h 15 Nov 2007 22:58:45 - @@ -21,36 +21,6 @@ /* allow to see translation results - the slowdown should be negligible, so we leave it */ #define DEBUG_DISAS -#ifndef glue -#define xglue(x, y) x ## y -#define glue(x, y) xglue(x, y) -#define stringify(s) tostring(s) -#define tostring(s) #s -#endif - -#ifndef likely -#if __GNUC__ 3 -#define __builtin_expect(x, n) (x) -#endif - -#define likely(x) __builtin_expect(!!(x), 1) -#define unlikely(x) __builtin_expect(!!(x), 0) -#endif - -#ifndef always_inline -#if (__GNUC__ 3) || defined(__APPLE__) -#define always_inline inline -#else -#define always_inline __attribute__ (( always_inline )) inline -#endif -#endif - -#ifdef __i386__ -#define REGPARM(n) __attribute((regparm(n))) -#else -#define REGPARM(n) -#endif - /* is_jmp field values */ #define DISAS_NEXT0 /* next instruction can be analyzed */ #define DISAS_JUMP1 /* only pc was modified dynamically */ Index: osdep.h === RCS file: /sources/qemu/qemu/osdep.h,v retrieving revision 1.10 diff -u -d -d -p -r1.10 osdep.h --- osdep.h 7 Jun 2007 23:09:47 - 1.10 +++ osdep.h 15 Nov 2007 22:58:45 - @@ -3,6 +3,43 @@ #include stdarg.h +#ifndef glue +#define xglue(x, y) x ## y +#define glue(x, y) xglue(x, y) +#define stringify(s) tostring(s) +#define tostring(s) #s +#endif + +#ifndef likely +#if __GNUC__ 3 +#define __builtin_expect(x, n) (x) +#endif + +#define likely(x) __builtin_expect(!!(x), 1) +#define unlikely(x) __builtin_expect(!!(x), 0) +#endif + +#ifndef MIN +#define MIN(a, b) (((a) (b)) ? (a) : (b)) +#endif +#ifndef MAX +#define MAX(a, b) (((a) (b)) ? (a) : (b)) +#endif + +#ifndef always_inline +#if (__GNUC__ 3) || defined(__APPLE__) +#define always_inline inline +#else +#define always_inline __attribute__ (( always_inline )) inline +#endif +#endif + +#ifdef __i386__ +#define REGPARM(n) __attribute((regparm(n))) +#else +#define REGPARM(n) +#endif + #define qemu_printf printf void *qemu_malloc(size_t size); Index: vl.h
Re: [Qemu-devel] RFC: fix for random Qemu crashes
Hi, On 16/11/2007, J. Mayer [EMAIL PROTECTED] wrote: Some may have experienced of having some Qemu builds crashing, apparently at random places, but in a reproducable way. I found one reason for this crashes: it appears that with the growth of the op.c file, there may be cases where we could reach the inlining limits of gcc. In such a case, gcc would not inline some declared inline function but would emit a call and provide a separate function. Unfortunately, this is not acceptable in op.o context as it will slowdown the emulation and because the call is likely to break the specific compilation rules (ie reserved registers) used while compiling op.o I found some workaround to avoid this behavior and I'd like to get opinions about it. The first idea is to change all occurences of 'inline' with 'always_inline' in all headers included in op.c. It then appeared to me that always_inline is not globally declared and that the definition is duplicated in vl.h and exec-all.h. But it's not declared in darwin-user/qemu.h and linux-user/qemu.h, which is not consistent with the declaration in vl.h. Further investigations showed me that the osdep.h header is the one that is actually included everywhere. Even if those are more compiler than OS dependent, I decided to move the definitions for glue, tostring, likely, unlikely, always_inline and REGPARM to this header so they can be globally used. I also changed the include orders in darwin-user/qemu.h to be sure this header will be included first. This patch is attached in common_defs.diff. I had also noticed that glue and tostring definitions were present in three places in qemu and it seemed wrong to me, so I'm in favour of this patch. I can't say much about the other patches. After armv6/armv7 support was merged on Sunday, I had a consistent segfaults in the generated code and I tracked it down to cpsr_write function not being inlined properly because it grew in size. Changing the inline to always_inline helped but we decided to instead move the function to target-arm/helper.c. I don't know which approach is better, the performance hit shouldn't be notable (in case of ARM cpsr). Giving this patch, I've been able to replace all occurence of 'inline' with 'always_inline' in all headers included from op.c (given the generated .d file). Some would say I'd better add a #define inline always_inline somewhere. I personnally dislike this solution as this kind of macro as it tends to expand recursivally (always_inline definition contains the inline word) and this may lead to compilation warnings or errors in some context; one could do tests using the linux kernel headers to get convinced that it can happen. Then, I choosed to replace 'inline' by 'always_inline', which is more invasive but have less risks of side effects. The diff is attached in always_inline.diff. The last thing that helps solve the problem is to change the inlining limits of gcc, at least to compile the op.o file.Unfortunatelly, there is no way to disable those limits (I checked in the source code), then I put them to an arbitrary high level. I also added the -funit-at-a-time switch, as this kind of optimisation would not be relevant in op.o context. The diff is attached in gcc_inline_limits.diff. Please comment. -- J. Mayer [EMAIL PROTECTED] Never organized Regards
