Re: [Qemu-devel] [PATCH qom-next 01/12] store prev_debug_excp_handler globaly and not per target

[Igor Mammedov]

On 05/30/2012 12:51 PM, Jan Kiszka wrote:
> On 2012-05-30 00:10, Igor Mammedov wrote:
> > current callers all do the same thing, storing in prev_debug_excp_handler
> > previous handler and then calling it in breakpoint_handler.
> > Move prev_debug_excp_handler from local scope to global and make
> > cpu_set_debug_excp_handler() always to store previous handler.
>
> NAK, this is not a beautiful interface, exposing the previous handler
> via global variable. And it prevents chaining of handlers.
>
> Is there a technical reason for this refactoring?

current 2 users use prev_debug_excp_handler in the same manner i.e. doing
        prev_debug_excp_handler =
            cpu_set_debug_excp_handler(breakpoint_handler);
so consolidating it in one place looks better than writing the same code 
per-target.
In addition when chaining more then current and previous handlers would be 
needed,
the global var prev_debug_excp_handler could be replaced by global call
prev_debug_excp_handler() without affecting per-target code and it could be
implemented in cpu-exec.c or even doing chaining completely internally in 
cpu-exec.c
without breakpoint_handler() being aware of it.

PS:
For moving tcg initialization from helper.c into cpu.c I would need to make
prev_debug_excp_handler global per target any way for above assignment to
compile and it would be the same ugliness but without possible future.

PS2:
alternative with defining tcg_x86_init() call inside helper.c and calling
from cpu.c is acceptable too, so if you still don't like idea of this patch
I can go this way.


> Jan
>
> > Signed-off-by: Igor Mammedov<address@hidden>
> > ---
> >   cpu-exec.c |    7 +++----
> >   exec-all.h |    3 ++-
> >   2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/cpu-exec.c b/cpu-exec.c
> > index 83cac93..44c83d9 100644
> > --- a/cpu-exec.c
> > +++ b/cpu-exec.c
> > @@ -155,13 +155,12 @@ static inline TranslationBlock *tb_find_fast(CPUArchState 
> > *env)
> >   }
> >
> >   static CPUDebugExcpHandler *debug_excp_handler;
> > +CPUDebugExcpHandler *prev_debug_excp_handler;
> >
> > -CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
> > +void cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler)
> >   {
> > -    CPUDebugExcpHandler *old_handler = debug_excp_handler;
> > -
> > +    prev_debug_excp_handler = debug_excp_handler;
> >       debug_excp_handler = handler;
> > -    return old_handler;
> >   }
> >
> >   static void cpu_handle_debug_exception(CPUArchState *env)
> > diff --git a/exec-all.h b/exec-all.h
> > index 9bda7f7..f3c3a8a 100644
> > --- a/exec-all.h
> > +++ b/exec-all.h
> > @@ -357,7 +357,8 @@ tb_page_addr_t get_page_addr_code(CPUArchState *env1, 
> > target_ulong addr);
> >
> >   typedef void (CPUDebugExcpHandler)(CPUArchState *env);
> >
> > -CPUDebugExcpHandler *cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
> > +extern CPUDebugExcpHandler *prev_debug_excp_handler;
> > +void cpu_set_debug_excp_handler(CPUDebugExcpHandler *handler);
> >
> >   /* vl.c */
> >   extern int singlestep;

--
-----
 Igor