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

[J. Mayer]

On Sat, 2007-11-17 at 11:57 +0100, andrzej zaborowski wrote:
> On 17/11/2007, J. Mayer <address@hidden> wrote:
> >
> > On Fri, 2007-11-16 at 21:32 +0100, andrzej zaborowski wrote:
> > > On 16/11/2007, Jocelyn Mayer <address@hidden> wrote:
> > > >
> > > > On Fri, 2007-11-16 at 15:52 +0000, 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 <address@hidden>
Never organized