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

[Jan Kiszka]

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?

Jan

 
 Signed-off-by: Igor Mammedov imamm...@redhat.com
 ---
  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;

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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 Mammedovimamm...@redhat.com
---
  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

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

[Jan Kiszka]

On 2012-05-30 15:35, Igor Mammedov wrote:
 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.

As pointed out, this changes the API in disallowing chains of exception
handlers. Either drop this feature (it's currently unused) or leave the
API as it is. Introducing a global variable is not an acceptable API.

 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.

Users would be different subsystems, and they would use different
variables to store and call the previous callback in the chain. That was
the idea of this API.

However, it is probably simpler to make it a non-chained callback for
now and rethink the design once a use case shows up.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Igor Mammedov]

On 05/30/2012 05:07 PM, Jan Kiszka wrote:

On 2012-05-30 15:35, Igor Mammedov wrote:

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.


As pointed out, this changes the API in disallowing chains of exception
handlers. Either drop this feature (it's currently unused) or leave the
API as it is. Introducing a global variable is not an acceptable API.


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.


Users would be different subsystems, and they would use different
variables to store and call the previous callback in the chain. That was
the idea of this API.

However, it is probably simpler to make it a non-chained callback for
now and rethink the design once a use case shows up.


Ok, then I'll drop prev_debug_excp_handler handling for target-i386.
Thanks for patience and explanation!



Jan



--
-
 Igor

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

[Igor Mammedov]

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.

Signed-off-by: Igor Mammedov imamm...@redhat.com
---
 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;
-- 
1.7.7.6