[PATCH] target/i386: Use cpu_breakpoint_test in breakpoint_handler

[PATCH] target/i386: Use cpu_breakpoint_test in breakpoint_handler

[Richard Henderson]

The loop is performing a simple boolean test for the existence
of a BP_CPU breakpoint at EIP.  Plus it gets the iteration wrong,
if we happen to have a BP_GDB breakpoint at the same address.

We have a function for this: cpu_breakpoint_test.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/tcg/sysemu/bpt_helper.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/target/i386/tcg/sysemu/bpt_helper.c b/target/i386/tcg/sysemu/bpt_helper.c
index 9bdf7e170b..f1fb479ad9 100644
--- a/target/i386/tcg/sysemu/bpt_helper.c
+++ b/target/i386/tcg/sysemu/bpt_helper.c
@@ -210,7 +210,6 @@ void breakpoint_handler(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
-    CPUBreakpoint *bp;
 
     if (cs->watchpoint_hit) {
         if (cs->watchpoint_hit->flags & BP_CPU) {
@@ -222,14 +221,9 @@ void breakpoint_handler(CPUState *cs)
             }
         }
     } else {
-        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
-            if (bp->pc == env->eip) {
-                if (bp->flags & BP_CPU) {
-                    check_hw_breakpoints(env, true);
-                    raise_exception(env, EXCP01_DB);
-                }
-                break;
-            }
+        if (cpu_breakpoint_test(cs, env->eip, BP_CPU)) {
+            check_hw_breakpoints(env, true);
+            raise_exception(env, EXCP01_DB);
         }
     }
 }
-- 
2.25.1

Re: [PATCH] target/i386: Use cpu_breakpoint_test in breakpoint_handler

[Richard Henderson]

Ping.

On 6/19/21 11:23 PM, Richard Henderson wrote:
> The loop is performing a simple boolean test for the existence
> of a BP_CPU breakpoint at EIP.  Plus it gets the iteration wrong,
> if we happen to have a BP_GDB breakpoint at the same address.
> 
> We have a function for this: cpu_breakpoint_test.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/i386/tcg/sysemu/bpt_helper.c | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/target/i386/tcg/sysemu/bpt_helper.c b/target/i386/tcg/sysemu/bpt_helper.c
> index 9bdf7e170b..f1fb479ad9 100644
> --- a/target/i386/tcg/sysemu/bpt_helper.c
> +++ b/target/i386/tcg/sysemu/bpt_helper.c
> @@ -210,7 +210,6 @@ void breakpoint_handler(CPUState *cs)
>   {
>       X86CPU *cpu = X86_CPU(cs);
>       CPUX86State *env = &cpu->env;
> -    CPUBreakpoint *bp;
>   
>       if (cs->watchpoint_hit) {
>           if (cs->watchpoint_hit->flags & BP_CPU) {
> @@ -222,14 +221,9 @@ void breakpoint_handler(CPUState *cs)
>               }
>           }
>       } else {
> -        QTAILQ_FOREACH(bp, &cs->breakpoints, entry) {
> -            if (bp->pc == env->eip) {
> -                if (bp->flags & BP_CPU) {
> -                    check_hw_breakpoints(env, true);
> -                    raise_exception(env, EXCP01_DB);
> -                }
> -                break;
> -            }
> +        if (cpu_breakpoint_test(cs, env->eip, BP_CPU)) {
> +            check_hw_breakpoints(env, true);
> +            raise_exception(env, EXCP01_DB);
>           }
>       }
>   }
> 

Re: [PATCH] target/i386: Use cpu_breakpoint_test in breakpoint_handler

[Eduardo Habkost]

On Fri, Jul 09, 2021 at 09:00:30AM -0700, Richard Henderson wrote:
> Ping.
> 
> On 6/19/21 11:23 PM, Richard Henderson wrote:
> > The loop is performing a simple boolean test for the existence
> > of a BP_CPU breakpoint at EIP.  Plus it gets the iteration wrong,
> > if we happen to have a BP_GDB breakpoint at the same address.
> > 
> > We have a function for this: cpu_breakpoint_test.
> > 
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

cpu_breakpoint_test() logic matches the existing code being
replaced, so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I wouldn't have objected if you simply merged this without
waiting for review from others, though.  I believe you know this
code better than anybody else.

-- 
Eduardo