[v3,2/3] target-i386:make hw_breakpoint_enabled return bool type

Submitted by liguang on Dec. 7, 2012, 1:25 a.m.

Details

Message ID 1354843535-10912-2-git-send-email-lig.fnst@cn.fujitsu.com
State New
Headers show

Commit Message

liguang Dec. 7, 2012, 1:25 a.m.
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
---
 target-i386/cpu.h |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)

Comments

Peter Maydell Dec. 7, 2012, 10:24 a.m.
On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  target-i386/cpu.h |   15 +++++++++++++--
>  1 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 29245d1..3646128 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
>
> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
>  {
> -    return (dr7 >> (index * 2)) & 3;
> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);

This is pretty confusing and I'm pretty sure the function is
misnamed too. If we're checking "is local breakpoint enabled"
then we only want to check one of the two enable bits, not both.


> +}
> +
> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> +{
> +    return !!((dr7 >> (index * 2)) & 2);
> +}
> +
> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
> +{
> +    return (hw_global_breakpoint_enabled(dr7, index) ||
> +            hw_local_breakpoint_enabled(dr7, index));
>  }

-- PMM
Jan Kiszka Dec. 7, 2012, 10:29 a.m.
On 2012-12-07 11:24, Peter Maydell wrote:
> On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>> ---
>>  target-i386/cpu.h |   15 +++++++++++++--
>>  1 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
>> index 29245d1..3646128 100644
>> --- a/target-i386/cpu.h
>> +++ b/target-i386/cpu.h
>> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
>>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
>>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
>>
>> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
>> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
>>  {
>> -    return (dr7 >> (index * 2)) & 3;
>> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);
> 
> This is pretty confusing and I'm pretty sure the function is
> misnamed too. If we're checking "is local breakpoint enabled"
> then we only want to check one of the two enable bits, not both.
> 

Yes, and I already asked to define the proper constants that allow
checking for local vs. global enable bit. They have to be used here
instead of all the magic & 3 or ^ 1 stuff.

BTW, there is no need for "converting" ("!!") the result of the (value &
mask) to bool, the compiler will do this already.

Jan

> 
>> +}
>> +
>> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
>> +{
>> +    return !!((dr7 >> (index * 2)) & 2);
>> +}
>> +
>> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
>> +{
>> +    return (hw_global_breakpoint_enabled(dr7, index) ||
>> +            hw_local_breakpoint_enabled(dr7, index));
>>  }
> 
> -- PMM
>
liguang Dec. 10, 2012, 2:24 a.m.
在 2012-12-07五的 11:29 +0100,Jan Kiszka写道:
> On 2012-12-07 11:24, Peter Maydell wrote:
> > On 7 December 2012 01:25, liguang <lig.fnst@cn.fujitsu.com> wrote:
> >> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> >> ---
> >>  target-i386/cpu.h |   15 +++++++++++++--
> >>  1 files changed, 13 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> >> index 29245d1..3646128 100644
> >> --- a/target-i386/cpu.h
> >> +++ b/target-i386/cpu.h
> >> @@ -996,9 +996,20 @@ int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
> >>  #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
> >>  void cpu_x86_set_a20(CPUX86State *env, int a20_state);
> >>
> >> -static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
> >> +static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
> >>  {
> >> -    return (dr7 >> (index * 2)) & 3;
> >> +    return !(((dr7 >> (index * 2)) ^ 1) & 3);
> > 
> > This is pretty confusing and I'm pretty sure the function is
> > misnamed too. If we're checking "is local breakpoint enabled"
> > then we only want to check one of the two enable bits, not both.
> > 
> 
> Yes, and I already asked to define the proper constants that allow
> checking for local vs. global enable bit. They have to be used here
> instead of all the magic & 3 or ^ 1 stuff.
> 
> BTW, there is no need for "converting" ("!!") the result of the (value &
> mask) to bool, the compiler will do this already.
> 
> Jan

OK, as both of you commented, I'll fix for them.
Thanks!

> 
> > 
> >> +}
> >> +
> >> +static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
> >> +{
> >> +    return !!((dr7 >> (index * 2)) & 2);
> >> +}
> >> +
> >> +static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
> >> +{
> >> +    return (hw_global_breakpoint_enabled(dr7, index) ||
> >> +            hw_local_breakpoint_enabled(dr7, index));
> >>  }
> > 
> > -- PMM
> > 
>

Patch hide | download patch | download mbox

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 29245d1..3646128 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -996,9 +996,20 @@  int cpu_x86_handle_mmu_fault(CPUX86State *env, target_ulong addr,
 #define cpu_handle_mmu_fault cpu_x86_handle_mmu_fault
 void cpu_x86_set_a20(CPUX86State *env, int a20_state);
 
-static inline int hw_breakpoint_enabled(unsigned long dr7, int index)
+static inline bool hw_local_breakpoint_enabled(unsigned long dr7, int index)
 {
-    return (dr7 >> (index * 2)) & 3;
+    return !(((dr7 >> (index * 2)) ^ 1) & 3);
+}
+
+static inline bool hw_global_breakpoint_enabled(unsigned long dr7, int index)
+{
+    return !!((dr7 >> (index * 2)) & 2);
+}
+
+static inline bool hw_breakpoint_enabled(unsigned long dr7, int index)
+{
+    return (hw_global_breakpoint_enabled(dr7, index) ||
+            hw_local_breakpoint_enabled(dr7, index));
 }
 
 static inline int hw_breakpoint_type(unsigned long dr7, int index)