[PATCH] gdb: convert nat/x86-dregs.c macros to functions

[PATCH] gdb: convert nat/x86-dregs.c macros to functions

[Simon Marchi]

I'm debugging why GDB crashes on OpenBSD/amd64, turns out it's because
x86_dr_low.get_status is nullptr.  It would have been useful to be able
to break on x86_dr_low_get_status, so I thought it would be a good
reason to convert these function-like macros into functions.

Change-Id: Ic200b50ef8455b4697bc518da0fa2bb704cf4721
---
 gdb/nat/x86-dregs.c | 59 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 11 deletions(-)

diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
index b4eb7bf6d27b..cf8e517eb0d6 100644
--- a/gdb/nat/x86-dregs.c
+++ b/gdb/nat/x86-dregs.c
@@ -35,31 +35,68 @@
 /* Accessor macros for low-level function vector.  */
 
 /* Can we update the inferior's debug registers?  */
-#define x86_dr_low_can_set_addr() (x86_dr_low.set_addr != NULL)
+
+static bool
+x86_dr_low_can_set_addr ()
+{
+  return x86_dr_low.set_addr != nullptr;
+}
 
 /* Update the inferior's debug register REGNUM from STATE.  */
-#define x86_dr_low_set_addr(new_state, i) \
-  (x86_dr_low.set_addr ((i), (new_state)->dr_mirror[(i)]))
+
+static void
+x86_dr_low_set_addr (struct x86_debug_reg_state *new_state, int i)
+{
+  x86_dr_low.set_addr (i, new_state->dr_mirror[i]);
+}
 
 /* Return the inferior's debug register REGNUM.  */
-#define x86_dr_low_get_addr(i) (x86_dr_low.get_addr ((i)))
+
+static unsigned long
+x86_dr_low_get_addr (int i)
+{
+  return x86_dr_low.get_addr (i);
+}
 
 /* Can we update the inferior's DR7 control register?  */
-#define x86_dr_low_can_set_control() (x86_dr_low.set_control != NULL)
+
+static bool
+x86_dr_low_can_set_control ()
+{
+  return x86_dr_low.set_control != nullptr;
+}
 
 /* Update the inferior's DR7 debug control register from STATE.  */
-#define x86_dr_low_set_control(new_state) \
-  (x86_dr_low.set_control ((new_state)->dr_control_mirror))
+
+static void
+x86_dr_low_set_control (struct x86_debug_reg_state *new_state)
+{
+  x86_dr_low.set_control (new_state->dr_control_mirror);
+}
 
 /* Return the value of the inferior's DR7 debug control register.  */
-#define x86_dr_low_get_control() (x86_dr_low.get_control ())
+
+static unsigned long
+x86_dr_low_get_control ()
+{
+  return x86_dr_low.get_control ();
+}
 
 /* Return the value of the inferior's DR6 debug status register.  */
-#define x86_dr_low_get_status() (x86_dr_low.get_status ())
+
+static unsigned long
+x86_dr_low_get_status ()
+{
+  return x86_dr_low.get_status ();
+}
 
 /* Return the debug register size, in bytes.  */
-#define x86_get_debug_register_length() \
-  (x86_dr_low.debug_register_length)
+
+static int
+x86_get_debug_register_length ()
+{
+  return x86_dr_low.debug_register_length;
+}
 
 /* Support for 8-byte wide hw watchpoints.  */
 #define TARGET_HAS_DR_LEN_8 (x86_get_debug_register_length () == 8)
-- 
2.32.0

Re: [PATCH] gdb: convert nat/x86-dregs.c macros to functions

[Joel Brobecker]

Hi Simon,

On Thu, Jul 15, 2021 at 03:01:58PM -0400, Simon Marchi via Gdb-patches wrote:
> I'm debugging why GDB crashes on OpenBSD/amd64, turns out it's because
> x86_dr_low.get_status is nullptr.  It would have been useful to be able
> to break on x86_dr_low_get_status, so I thought it would be a good
> reason to convert these function-like macros into functions.
> 
> Change-Id: Ic200b50ef8455b4697bc518da0fa2bb704cf4721

I like the conversion to functions. Not only can we break into
that code as funtions, now, but it also documents the types of
the parameters better.

I looked at the patch, and it looked good to me (including the one
slight change where you replaced a NULL by nullptr).

> ---
>  gdb/nat/x86-dregs.c | 59 ++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c
> index b4eb7bf6d27b..cf8e517eb0d6 100644
> --- a/gdb/nat/x86-dregs.c
> +++ b/gdb/nat/x86-dregs.c
> @@ -35,31 +35,68 @@
>  /* Accessor macros for low-level function vector.  */
>  
>  /* Can we update the inferior's debug registers?  */
> -#define x86_dr_low_can_set_addr() (x86_dr_low.set_addr != NULL)
> +
> +static bool
> +x86_dr_low_can_set_addr ()
> +{
> +  return x86_dr_low.set_addr != nullptr;
> +}
>  
>  /* Update the inferior's debug register REGNUM from STATE.  */
> -#define x86_dr_low_set_addr(new_state, i) \
> -  (x86_dr_low.set_addr ((i), (new_state)->dr_mirror[(i)]))
> +
> +static void
> +x86_dr_low_set_addr (struct x86_debug_reg_state *new_state, int i)
> +{
> +  x86_dr_low.set_addr (i, new_state->dr_mirror[i]);
> +}
>  
>  /* Return the inferior's debug register REGNUM.  */
> -#define x86_dr_low_get_addr(i) (x86_dr_low.get_addr ((i)))
> +
> +static unsigned long
> +x86_dr_low_get_addr (int i)
> +{
> +  return x86_dr_low.get_addr (i);
> +}
>  
>  /* Can we update the inferior's DR7 control register?  */
> -#define x86_dr_low_can_set_control() (x86_dr_low.set_control != NULL)
> +
> +static bool
> +x86_dr_low_can_set_control ()
> +{
> +  return x86_dr_low.set_control != nullptr;
> +}
>  
>  /* Update the inferior's DR7 debug control register from STATE.  */
> -#define x86_dr_low_set_control(new_state) \
> -  (x86_dr_low.set_control ((new_state)->dr_control_mirror))
> +
> +static void
> +x86_dr_low_set_control (struct x86_debug_reg_state *new_state)
> +{
> +  x86_dr_low.set_control (new_state->dr_control_mirror);
> +}
>  
>  /* Return the value of the inferior's DR7 debug control register.  */
> -#define x86_dr_low_get_control() (x86_dr_low.get_control ())
> +
> +static unsigned long
> +x86_dr_low_get_control ()
> +{
> +  return x86_dr_low.get_control ();
> +}
>  
>  /* Return the value of the inferior's DR6 debug status register.  */
> -#define x86_dr_low_get_status() (x86_dr_low.get_status ())
> +
> +static unsigned long
> +x86_dr_low_get_status ()
> +{
> +  return x86_dr_low.get_status ();
> +}
>  
>  /* Return the debug register size, in bytes.  */
> -#define x86_get_debug_register_length() \
> -  (x86_dr_low.debug_register_length)
> +
> +static int
> +x86_get_debug_register_length ()
> +{
> +  return x86_dr_low.debug_register_length;
> +}
>  
>  /* Support for 8-byte wide hw watchpoints.  */
>  #define TARGET_HAS_DR_LEN_8 (x86_get_debug_register_length () == 8)
> -- 
> 2.32.0
> 

-- 
Joel

Re: [PATCH] gdb: convert nat/x86-dregs.c macros to functions

[Simon Marchi]

On 2021-07-17 2:41 p.m., Joel Brobecker wrote:
> Hi Simon,
> 
> On Thu, Jul 15, 2021 at 03:01:58PM -0400, Simon Marchi via Gdb-patches wrote:
>> I'm debugging why GDB crashes on OpenBSD/amd64, turns out it's because
>> x86_dr_low.get_status is nullptr.  It would have been useful to be able
>> to break on x86_dr_low_get_status, so I thought it would be a good
>> reason to convert these function-like macros into functions.
>>
>> Change-Id: Ic200b50ef8455b4697bc518da0fa2bb704cf4721
> 
> I like the conversion to functions. Not only can we break into
> that code as funtions, now, but it also documents the types of
> the parameters better.
> 
> I looked at the patch, and it looked good to me (including the one
> slight change where you replaced a NULL by nullptr).
> 

Thanks for the review, I pushed it.

Simon