diff mbox series

[2/9] gdb: pass frame_info_ptr to gdbarch_value_from_register

Message ID 20231221191716.257256-3-simon.marchi@efficios.com
State New
Headers
Series Some register value cleanups |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed

Commit Message

Simon Marchi Dec. 21, 2023, 7:16 p.m. UTC 
  Pass a frame_info_ptr rather than a frame_id.  This avoids having to do
a frame lookup on the callee side, when we can just pass the frame down
directly.

I think this fixes a bug in rs6000-tdep.c where the id of the wrong
frame was set to `VALUE_NEXT_FRAME_ID (v)`.

Change-Id: I77039bc87ea8fc5262f16d0e1446515efa21c565
---
 gdb/findvar.c             | 24 ++++++++++--------------
 gdb/gdbarch-gen.h         |  6 +++---
 gdb/gdbarch.c             |  4 ++--
 gdb/gdbarch_components.py |  4 ++--
 gdb/rs6000-tdep.c         | 16 ++++++++--------
 gdb/s390-tdep.c           | 10 +++++-----
 gdb/value.h               |  7 +++----
 7 files changed, 33 insertions(+), 38 deletions(-)

Comments

Tom Tromey Dec. 22, 2023, 4:40 p.m. UTC | #1 
>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Pass a frame_info_ptr rather than a frame_id.  This avoids having to do
Simon> a frame lookup on the callee side, when we can just pass the frame down
Simon> directly.

Simon> I think this fixes a bug in rs6000-tdep.c where the id of the wrong
Simon> frame was set to `VALUE_NEXT_FRAME_ID (v)`.

Simon> +value *
Simon> +default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
Simon> +			     frame_info_ptr this_frame)

FWIW I tend to think new code should use 'const frame_info_ptr &' in
most places.  It's more efficient and the design of frame_info_ptr also
makes it harmless, as the underlying frame_info member is mutable.

Tom
Simon Marchi Dec. 22, 2023, 4:53 p.m. UTC | #2 
On 2023-12-22 11:40, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Pass a frame_info_ptr rather than a frame_id.  This avoids having to do
> Simon> a frame lookup on the callee side, when we can just pass the frame down
> Simon> directly.
> 
> Simon> I think this fixes a bug in rs6000-tdep.c where the id of the wrong
> Simon> frame was set to `VALUE_NEXT_FRAME_ID (v)`.
> 
> Simon> +value *
> Simon> +default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
> Simon> +			     frame_info_ptr this_frame)
> 
> FWIW I tend to think new code should use 'const frame_info_ptr &' in
> most places.  It's more efficient and the design of frame_info_ptr also
> makes it harmless, as the underlying frame_info member is mutable.

I initially thought that frame_info_ptr would be a thin enough wrapper
that it would be better to pass it by value, but I see that it's 64
bytes long, so I guess you're right.  I'll change my patches, but we
should probably change the existing code too.

It will also appease a frustration I had, where passing a frame_info_ptr
down to a function would call the copy constructor.  So, trying to step
into that function would first step into the frame_info_ptr constructor
first.  I tried to add some skips for that in gdb-gdb.gdb, but I wasn't
very successful (I didn't try very hard though).

Simon
Tom Tromey Dec. 22, 2023, 4:56 p.m. UTC | #3 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> FWIW I tend to think new code should use 'const frame_info_ptr &' in
>> most places.  It's more efficient and the design of frame_info_ptr also
>> makes it harmless, as the underlying frame_info member is mutable.

Simon> I initially thought that frame_info_ptr would be a thin enough wrapper
Simon> that it would be better to pass it by value, but I see that it's 64
Simon> bytes long, so I guess you're right.  I'll change my patches, but we
Simon> should probably change the existing code too.

Yeah, and it also registers/deregisters itself on a global list...

Simon> It will also appease a frustration I had, where passing a frame_info_ptr
Simon> down to a function would call the copy constructor.  So, trying to step
Simon> into that function would first step into the frame_info_ptr constructor
Simon> first.

I also find this annoying, though not yet to the point where I'm tempted
to add a gdb feature to make it nicer.

I think we have a bug open about stepping into the "last" function call
on a line, which is pretty much this idea: skip over all the copy
constructors and into the call you really care about.  Dunno if this is
even implementable with current debuginfo.

Tom
diff mbox series

Patch

diff --git a/gdb/findvar.c b/gdb/findvar.c
index e6dedf0aadf6..25737c4e3159 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -747,23 +747,21 @@  read_var_value (struct symbol *var, const struct block *var_block,
 
 /* Install default attributes for register values.  */
 
-struct value *
-default_value_from_register (struct gdbarch *gdbarch, struct type *type,
-			     int regnum, struct frame_id frame_id)
+value *
+default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
+			     frame_info_ptr this_frame)
 {
   int len = type->length ();
   struct value *value = value::allocate (type);
-  frame_info_ptr frame;
-
   value->set_lval (lval_register);
-  frame = frame_find_by_id (frame_id);
 
-  if (frame == NULL)
-    frame_id = null_frame_id;
+  frame_id next_frame_id;
+  if (this_frame == nullptr)
+    next_frame_id = null_frame_id;
   else
-    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+    next_frame_id = get_frame_id (get_next_frame_sentinel_okay (this_frame));
 
-  VALUE_NEXT_FRAME_ID (value) = frame_id;
+  VALUE_NEXT_FRAME_ID (value) = next_frame_id;
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
@@ -865,8 +863,7 @@  value_from_register (struct type *type, int regnum, frame_info_ptr frame)
   else
     {
       /* Construct the value.  */
-      v = gdbarch_value_from_register (gdbarch, type,
-				       regnum, get_frame_id (frame));
+      v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
 
       /* Get the data.  */
       read_frame_register_value (v, frame);
@@ -883,7 +880,6 @@  address_from_register (int regnum, frame_info_ptr frame)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
   struct type *type = builtin_type (gdbarch)->builtin_data_ptr;
-  struct value *value;
   CORE_ADDR result;
   int regnum_max_excl = gdbarch_num_cooked_regs (gdbarch);
 
@@ -919,7 +915,7 @@  address_from_register (int regnum, frame_info_ptr frame)
       return unpack_long (type, buf);
     }
 
-  value = gdbarch_value_from_register (gdbarch, type, regnum, null_frame_id);
+  value *value = gdbarch_value_from_register (gdbarch, type, regnum, nullptr);
   read_frame_register_value (value, frame);
 
   if (value->optimized_out ())
diff --git a/gdb/gdbarch-gen.h b/gdb/gdbarch-gen.h
index 80d40136c379..a7761616e032 100644
--- a/gdb/gdbarch-gen.h
+++ b/gdb/gdbarch-gen.h
@@ -422,12 +422,12 @@  extern void gdbarch_value_to_register (struct gdbarch *gdbarch, frame_info_ptr f
 extern void set_gdbarch_value_to_register (struct gdbarch *gdbarch, gdbarch_value_to_register_ftype *value_to_register);
 
 /* Construct a value representing the contents of register REGNUM in
-   frame FRAME_ID, interpreted as type TYPE.  The routine needs to
+   frame THIS_FRAME, interpreted as type TYPE.  The routine needs to
    allocate and return a struct value with all value attributes
    (but not the value contents) filled in. */
 
-typedef struct value * (gdbarch_value_from_register_ftype) (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
-extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id);
+typedef struct value * (gdbarch_value_from_register_ftype) (struct gdbarch *gdbarch, struct type *type, int regnum, frame_info_ptr this_frame);
+extern struct value * gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, frame_info_ptr this_frame);
 extern void set_gdbarch_value_from_register (struct gdbarch *gdbarch, gdbarch_value_from_register_ftype *value_from_register);
 
 typedef CORE_ADDR (gdbarch_pointer_to_address_ftype) (struct gdbarch *gdbarch, struct type *type, const gdb_byte *buf);
diff --git a/gdb/gdbarch.c b/gdb/gdbarch.c
index d584305fefb2..6d83e17160e6 100644
--- a/gdb/gdbarch.c
+++ b/gdb/gdbarch.c
@@ -2557,13 +2557,13 @@  set_gdbarch_value_to_register (struct gdbarch *gdbarch,
 }
 
 struct value *
-gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, struct frame_id frame_id)
+gdbarch_value_from_register (struct gdbarch *gdbarch, struct type *type, int regnum, frame_info_ptr this_frame)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->value_from_register != NULL);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_value_from_register called\n");
-  return gdbarch->value_from_register (gdbarch, type, regnum, frame_id);
+  return gdbarch->value_from_register (gdbarch, type, regnum, this_frame);
 }
 
 void
diff --git a/gdb/gdbarch_components.py b/gdb/gdbarch_components.py
index 4352f7066512..432e4b4ea267 100644
--- a/gdb/gdbarch_components.py
+++ b/gdb/gdbarch_components.py
@@ -814,7 +814,7 @@  Function(
 Method(
     comment="""
 Construct a value representing the contents of register REGNUM in
-frame FRAME_ID, interpreted as type TYPE.  The routine needs to
+frame THIS_FRAME, interpreted as type TYPE.  The routine needs to
 allocate and return a struct value with all value attributes
 (but not the value contents) filled in.
 """,
@@ -823,7 +823,7 @@  allocate and return a struct value with all value attributes
     params=[
         ("struct type *", "type"),
         ("int", "regnum"),
-        ("struct frame_id", "frame_id"),
+        ("frame_info_ptr", "this_frame"),
     ],
     predefault="default_value_from_register",
     invalid=False,
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index a7e0bf5305b5..a0f3d94f3a11 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2747,9 +2747,9 @@  rs6000_value_to_register (frame_info_ptr frame,
   put_frame_register (get_next_frame_sentinel_okay (frame), regnum, to_view);
 }
 
-static struct value *
-rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type,
-			    int regnum, struct frame_id frame_id)
+static value *
+rs6000_value_from_register (gdbarch *gdbarch, type *type, int regnum,
+			    frame_info_ptr this_frame)
 {
   int len = type->length ();
   struct value *value = value::allocate (type);
@@ -2759,14 +2759,14 @@  rs6000_value_from_register (struct gdbarch *gdbarch, struct type *type,
   regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
 
   value->set_lval (lval_register);
-  frame_info_ptr frame = frame_find_by_id (frame_id);
 
-  if (frame == NULL)
-    frame_id = null_frame_id;
+  frame_id next_frame_id;
+  if (this_frame == nullptr)
+    next_frame_id = null_frame_id;
   else
-    frame_id = get_frame_id (get_next_frame_sentinel_okay (frame));
+    next_frame_id = get_frame_id (get_next_frame_sentinel_okay (this_frame));
 
-  VALUE_NEXT_FRAME_ID (value) = frame_id;
+  VALUE_NEXT_FRAME_ID (value) = next_frame_id;
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
diff --git a/gdb/s390-tdep.c b/gdb/s390-tdep.c
index fcba7a1a5608..ebbcbc153fe1 100644
--- a/gdb/s390-tdep.c
+++ b/gdb/s390-tdep.c
@@ -1236,13 +1236,13 @@  regnum_is_vxr_full (s390_gdbarch_tdep *tdep, int regnum)
    registers, even though we are otherwise a big-endian platform.  The
    same applies to a 'float' value within a vector.  */
 
-static struct value *
-s390_value_from_register (struct gdbarch *gdbarch, struct type *type,
-			  int regnum, struct frame_id frame_id)
+static value *
+s390_value_from_register (gdbarch *gdbarch, type *type, int regnum,
+			  frame_info_ptr this_frame)
 {
   s390_gdbarch_tdep *tdep = gdbarch_tdep<s390_gdbarch_tdep> (gdbarch);
-  struct value *value = default_value_from_register (gdbarch, type,
-						     regnum, frame_id);
+  value *value
+    = default_value_from_register (gdbarch, type, regnum, this_frame);
   check_typedef (type);
 
   if ((regnum >= S390_F0_REGNUM && regnum <= S390_F15_REGNUM
diff --git a/gdb/value.h b/gdb/value.h
index d7bda1e8d2c9..d4244dadb91a 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1110,10 +1110,9 @@  extern struct value *value_from_contents_and_address
       frame_info_ptr frame = nullptr);
 extern struct value *value_from_contents (struct type *, const gdb_byte *);
 
-extern struct value *default_value_from_register (struct gdbarch *gdbarch,
-						  struct type *type,
-						  int regnum,
-						  struct frame_id frame_id);
+extern value *default_value_from_register (gdbarch *gdbarch, type *type,
+					   int regnum,
+					   frame_info_ptr this_frame);
 
 extern void read_frame_register_value (struct value *value,
 				       frame_info_ptr frame);