Re: [Qemu-devel] RFC: fix for random Qemu crashes

[andrzej zaborowski]

Hi,

On 16/11/2007, J. Mayer <address@hidden> 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 <address@hidden>
> Never organized
>
>

Regards