Re: [Qemu-devel] RFC: fix for random Qemu crashes
On Sat, 2007-11-17 at 11:57 +0100, andrzej zaborowski wrote: > On 17/11/2007, J. Mayer <[EMAIL PROTECTED]> wrote: > > > > 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. > > My point here is that you can use dyngen-exec.h for the macro so that > the functions are only always_inline'd when used in op.c, not in other > files, I think that's what pbrook mean too. For example cpsr_write > from target-arm/exec.h was used in op.c as well as in vl.c. There's no > problem if it isn't inlined in vl.c, the fix should only affect op.c > which is a special case, for other files let gcc decide in the way it > was designed by gcc authors. The problem in op.c is that not inlining lead to crashes. But not inlining functions in many other places would lead to great performances issues. dyngen-exec.h is included only in cpu-exec.c, host-utils.c, op.c and op_helpers.c (from what I see in the .d files). But, for example, not inlining code fetch or gen_op_xxx in translate.c is a very bad news for code translation efficiency. Not inlining in exec.c also have a great performance impact. It seems to me that there are much more cases we want always_inline rule to be applied than cases for which it's not so important. And it never hurts if the inlining is not done in the later case but it can have a great impact if inlining is not done for most functions that are declared inline in Qemu. [...] -- J. Mayer <[EMAIL PROTECTED]> Never organized
Re: [Qemu-devel] RFC: fix for random Qemu crashes
On 17/11/2007, J. Mayer <[EMAIL PROTECTED]> wrote: > > 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. My point here is that you can use dyngen-exec.h for the macro so that the functions are only always_inline'd when used in op.c, not in other files, I think that's what pbrook mean too. For example cpsr_write from target-arm/exec.h was used in op.c as well as in vl.c. There's no problem if it isn't inlined in vl.c, the fix should only affect op.c which is a special case, for other files let gcc decide in the way it was designed by gcc authors. > 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. Yes, I'm not saying anything about the other part. 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
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 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 Fri, 2007-11-16 at 17:42 +0200, Heikki Lindholm wrote: > 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. yes, sure. > 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 This is not sufficient. You will still have problems if triggering the growth limit of a single function or the whole compilation unit. Furthermore, 4000 is not a lot, I think it should be set to a very high value we could never reach in real cases. I did a build with current CVS and the -Winline switch set, using gcc-3.4.6 on a x86_64 host. This showed me more problems than I expected: while compiling slirp code, I got a lot of warnings from slirp headers: warning: inlining failed in call to 'slirp_remque': function body not available Those seem to be real bugs to be fixed. from linux-user/syscall.c, I got a lot of warnings "--param large-function-growth limit reached" in tswap32/tswap64 functions. This does not seem to be critical, as syscall is actually a slow operation. from target-i386/helper.c, I got a gew warnings "--param max-inline-insns-single limit reached" from target-arm/translate.c, got a few "--param max-inline-insns-single limit reached" from target-sparc/translate.c, got a few "--param max-inline-insns-single limit reached after inlining into the callee" from target-mips/translate.c: got a lot of "--param inline-unit-growth limit reached" from gen-op.h, which can be quite a problem from target-mips/op_helper.c: a lot of " --param inline-unit-growth limit reached", mostly for load/store functions, which may be problematic. from target-alpha/translate.c: got a lot of "--param inline-unit-growth limit reached" More warnings are issued for full system emulation: from target-i386/translate.c: a lot of "--param large-function-growth limit reached" from softmmu_header.h, which means code fetch is done via function, which is not great. from hw/pxa2xx.c: "--param large-function-growth limit reached". Should not be problematic. from target-ppc/helper.c: lots of "--param inline-unit-growth limit reached" for load/store functions, which may be problematic I noticed no warnings from target-ppc/op.c. But I've seen the bug triggered with the build I done before commiting wy latest patches (with those patches applied to a clean repository...) for the PowerPC 64 target, resulting in an emulator that would crash while entering user mode (with the CDROM images I tested). I'm not sure if there could be some cases gcc won't warn even when not inlining an declared inline function, or if the bug what not triggered this time, for an unknown reason as I just did a build from a checkout with just the -Winline flag added (then the build should be quite equivalent to the one I did this morning before commiting). -- Jocelyn Mayer <[EMAIL PROTECTED]>
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
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
> 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
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
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 00:49 +0100, andrzej zaborowski wrote: > 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). When the problem appears in rarely used or large functions, moving them to helpers might be the proper solution. When it appears in load/store functions (which is the case I experienced), this would not be acceptable, as those functions are used very often and are really time critical. Furthermore, this problem is likely to be encountered more often as the targets features increase and then need to be solved, even if using more helpers might be a work-around to temporary hide the problem. [...] -- J. Mayer <[EMAIL PROTECTED]> Never organized
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
