[PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Tom Tromey]

On a Windows build locally, watchpoints started failing.  I tracked
this down to x86_dr_low_get_addr returning an 'unsigned long'... in
this particular build, this is a 32-bit type, but the inferior is a
64-bit program.

This patch fixes the problem by changing the return type.  No other
change is really required, because this matches the function pointer
in struct x86_dr_low_type.

However, I found a spot in windows-nat.c that is a bit clearer if it
uses CORE_ADDR as well, so I've also made this change.
---
 gdb/nat/x86-dregs.c | 2 +-
 gdb/windows-nat.c   | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index cf8e517eb0d..24751647005 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -52,7 +52,7 @@ x86_dr_low_set_addr (struct x86_debug_reg_state *new_state, int i)
 
 /* Return the inferior's debug register REGNUM.  */
 
-static unsigned long
+static CORE_ADDR
 x86_dr_low_get_addr (int i)
 {
   return x86_dr_low.get_addr (i);
diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
index 0c2d55ef67b..b45de3cab7a 100644
--- a/gdb/windows-nat.c
+++ b/gdb/windows-nat.c
@@ -122,7 +122,7 @@ enum
 	| CONTEXT_SEGMENTS | CONTEXT_DEBUG_REGISTERS \
 	| CONTEXT_EXTENDED_REGISTERS
 
-static uintptr_t dr[8];
+static CORE_ADDR dr[8];
 static int debug_registers_changed;
 static int debug_registers_used;
 
@@ -3221,7 +3221,7 @@ cygwin_set_dr (int i, CORE_ADDR addr)
 static void
 cygwin_set_dr7 (unsigned long val)
 {
-  dr[7] = (CORE_ADDR) val;
+  dr[7] = val;
   debug_registers_changed = 1;
   debug_registers_used = 1;
 }
-- 
2.31.1

Re: [PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Simon Marchi via Gdb-patches]

On 2021-09-03 12:32 p.m., Tom Tromey wrote:
> On a Windows build locally, watchpoints started failing.  I tracked
> this down to x86_dr_low_get_addr returning an 'unsigned long'... in
> this particular build, this is a 32-bit type, but the inferior is a
> 64-bit program.
> 
> This patch fixes the problem by changing the return type.  No other
> change is really required, because this matches the function pointer
> in struct x86_dr_low_type.
> 
> However, I found a spot in windows-nat.c that is a bit clearer if it
> uses CORE_ADDR as well, so I've also made this change.
> ---
>  gdb/nat/x86-dregs.c | 2 +-
>  gdb/windows-nat.c   | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
> index cf8e517eb0d..24751647005 100644
> --- a/gdb/nat/x86-dregs.c
> +++ b/gdb/nat/x86-dregs.c
> @@ -52,7 +52,7 @@ x86_dr_low_set_addr (struct x86_debug_reg_state *new_state, int i)
>  
>  /* Return the inferior's debug register REGNUM.  */
>  
> -static unsigned long
> +static CORE_ADDR
>  x86_dr_low_get_addr (int i)
>  {

This bit looks good to me.

>    return x86_dr_low.get_addr (i);
> diff --git a/gdb/windows-nat.c b/gdb/windows-nat.c
> index 0c2d55ef67b..b45de3cab7a 100644
> --- a/gdb/windows-nat.c
> +++ b/gdb/windows-nat.c
> @@ -122,7 +122,7 @@ enum
>  	| CONTEXT_SEGMENTS | CONTEXT_DEBUG_REGISTERS \
>  	| CONTEXT_EXTENDED_REGISTERS
>  
> -static uintptr_t dr[8];
> +static CORE_ADDR dr[8];

I don't really mind the change, but I don't know if that's really any
more clear, because not all DR registers contain addresses, which is
what using CORE_ADDR suggests.  I guess the point of using uintptr_t is
that they are 32 bits when GDB is built as 32-bit (and it can't debug
64-bit programs then) and 64 bits when GDB is built as 64-bit (and the
top 32 bits are unused when debugging a 32-bit program).  What prompted
you to change this part?

Simon

Re: [PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:

>> -static uintptr_t dr[8];
>> +static CORE_ADDR dr[8];

Simon> I don't really mind the change, but I don't know if that's really any
Simon> more clear, because not all DR registers contain addresses, which is
Simon> what using CORE_ADDR suggests.  I guess the point of using uintptr_t is
Simon> that they are 32 bits when GDB is built as 32-bit (and it can't debug
Simon> 64-bit programs then) and 64 bits when GDB is built as 64-bit (and the
Simon> top 32 bits are unused when debugging a 32-bit program).  What prompted
Simon> you to change this part?

It just seemed to me that, most of the time, the value really is a
CORE_ADDR, and that anyway a CORE_ADDR would definitely be large enough.

Tom

Re: [PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Tom Tromey]

Tom> It just seemed to me that, most of the time, the value really is a
Tom> CORE_ADDR, and that anyway a CORE_ADDR would definitely be large enough.

Thinking about this a bit more -- it seems wrong to me to use uintptr_t
(a host type) to represent a register value (a target type).  So from
this perspective, CORE_ADDR is more correct.  Now, it's not completely
correct in the sense that not every DR register holds an address.  But,
it's at least harmless in those cases.

Tom

Re: [PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Simon Marchi via Gdb-patches]

On 2021-09-03 1:00 p.m., Tom Tromey wrote:
> Tom> It just seemed to me that, most of the time, the value really is a
> Tom> CORE_ADDR, and that anyway a CORE_ADDR would definitely be large enough.
> 
> Thinking about this a bit more -- it seems wrong to me to use uintptr_t
> (a host type) to represent a register value (a target type).  So from
> this perspective, CORE_ADDR is more correct.  Now, it's not completely
> correct in the sense that not every DR register holds an address.  But,
> it's at least harmless in those cases.

This is in windows-nat.c, where host == target, so I think uintptr_t is
somewhat correct given the uses cases we want to support (GDB 32
debugging inferior 32, GDB 64 debugging inferior 32 and GDB 64 debugging
inferior 64).  If it were in windows-tdep.c, it would definitely be
wrong.

But uintptr_t isn't any more correct than CORE_ADDR, so I'm not against
the change either.

Simon

Re: [PATCH] Use CORE_ADDR as return type from x86_dr_low_get_addr

[Tom Tromey]

Simon> This is in windows-nat.c, where host == target, so I think uintptr_t is
Simon> somewhat correct given the uses cases we want to support (GDB 32
Simon> debugging inferior 32, GDB 64 debugging inferior 32 and GDB 64 debugging
Simon> inferior 64).

I'll drop it, but if 32-bit gdb debugging 64-bit executables ever works,
then it will have to change at that time.

Tom