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

Message ID 20210715190158.2089522-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • gdb: convert nat/x86-dregs.c macros to functions
Related show

Commit Message

Simon Marchi via Gdb-patches July 15, 2021, 7:01 p.m.
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(-)

-- 
2.32.0

Comments

Joel Brobecker July 17, 2021, 6:41 p.m. | #1
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
Simon Marchi via Gdb-patches July 18, 2021, 2:52 a.m. | #2
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

Patch

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)