[PATCH 0/9] Some register value cleanups

[PATCH 0/9] Some register value cleanups

[Simon Marchi]

I'm currently investigating some things related to register read and
spotted some improvements that could be made to make the code a bit
simpler or more robust, here they are.

Simon Marchi (9):
  gdb: don't set frame id after calling cooked_read_value
  gdb: pass frame_info_ptr to gdbarch_value_from_register
  gdb: pass non-nullptr frame to gdbarch_value_from_register in
    address_from_register
  gdb: add type parameter to value::allocate_register and add
    value::allocate_register_lazy
  gdb: remove read_frame_register_value's frame parameter
  gdb: implement address_from_register using value_from_register
  gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id
  gdb: remove VALUE_REGNUM, add value::regnum
  gdb: make value::allocate_register_lazy store id of next non-inline
    frame

 gdb/findvar.c             | 126 +++++++++-----------------------------
 gdb/frame.c               |   7 +--
 gdb/gdbarch-gen.h         |   6 +-
 gdb/gdbarch.c             |   4 +-
 gdb/gdbarch_components.py |   4 +-
 gdb/python/py-unwind.c    |   2 +-
 gdb/rs6000-tdep.c         |  26 +++-----
 gdb/s390-tdep.c           |  10 +--
 gdb/sentinel-frame.c      |   1 -
 gdb/stack.c               |   3 +-
 gdb/valops.c              |  22 +++----
 gdb/value.c               |  84 +++++++++++++++----------
 gdb/value.h               |  50 ++++++++-------
 13 files changed, 138 insertions(+), 207 deletions(-)


base-commit: 333a6b1a6399992cc98ac34727acf38136b770e6
-- 
2.43.0

[PATCH 1/9] gdb: don't set frame id after calling cooked_read_value

[Simon Marchi]

I don't think that setting the next frame id is needed there, all code
paths in cooked_read_value do set it properly, AFAIK.

Change-Id: Idb9d9e6f89c2c95c5ebfeec2a63fde89ed84cf3d
---
 gdb/sentinel-frame.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gdb/sentinel-frame.c b/gdb/sentinel-frame.c
index 3e5b9be2edf6..9a9e4c29198e 100644
--- a/gdb/sentinel-frame.c
+++ b/gdb/sentinel-frame.c
@@ -54,7 +54,6 @@ sentinel_frame_prev_register (frame_info_ptr this_frame,
   gdb_assert (is_sentinel_frame_id (this_frame_id));
 
   value = cache->regcache->cooked_read_value (regnum);
-  VALUE_NEXT_FRAME_ID (value) = this_frame_id;
 
   return value;
 }
-- 
2.43.0

[PATCH 2/9] gdb: pass frame_info_ptr to gdbarch_value_from_register

[Simon Marchi]

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(-)

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);
-- 
2.43.0

Re: [PATCH 2/9] gdb: pass frame_info_ptr to gdbarch_value_from_register

[Tom Tromey]

>>>>> "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

Re: [PATCH 2/9] gdb: pass frame_info_ptr to gdbarch_value_from_register

[Simon Marchi]

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

Re: [PATCH 2/9] gdb: pass frame_info_ptr to gdbarch_value_from_register

[Tom Tromey]

>>>>> "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

[PATCH 3/9] gdb: pass non-nullptr frame to gdbarch_value_from_register in address_from_register

[Simon Marchi]

address_from_register used to pass null_frame_id to
gdbarch_value_from_register as "this frame"'s id, because it's possible
for it to be called during unwind, when "this frame"'s id is not yet
known.  This create an oddity where those register struct values are
created without a valid next frame id.  I would much prefer for things
to be consistent and have all register struct values to have a valid
next frame id.

Since gdbarch_value_from_register takes a frame_info_ptr now, rather
than a frame_id, we can pass down "this frame", even if it doesn't have
a valid id.  gdbarch_value_from_register implementations can obtain the
next frame from it.

However, it's possible for the "this frame"'s next frame to be an
inline frame, inlined in "this frame", in which case that next frame's
id is also not known.  So, loop until we get to the next non-inline
frame (which is actually the frame where registers for "this frame" are
unwound from).  This is the same thing that we do in
value_of_register_lazy, for the same reason.  A later patch will factor
out this "while next frame is inline" loop to apply it to all register
struct values, so this is somewhat temporary.

Change-Id: If487c82620cc5a4a4ea5807f0a0bad80ab984078
---
 gdb/findvar.c     | 19 +++++--------------
 gdb/rs6000-tdep.c | 10 ++++------
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 25737c4e3159..852f42dc176d 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -755,13 +755,11 @@ default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
   struct value *value = value::allocate (type);
   value->set_lval (lval_register);
 
-  frame_id next_frame_id;
-  if (this_frame == nullptr)
-    next_frame_id = null_frame_id;
-  else
-    next_frame_id = get_frame_id (get_next_frame_sentinel_okay (this_frame));
+  frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
+  while (get_frame_type (next_frame) == INLINE_FRAME)
+    next_frame = get_next_frame_sentinel_okay (next_frame);
 
-  VALUE_NEXT_FRAME_ID (value) = next_frame_id;
+  VALUE_NEXT_FRAME_ID (value) = get_frame_id (next_frame);
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
@@ -887,13 +885,6 @@ address_from_register (int regnum, frame_info_ptr frame)
     error (_("Invalid register #%d, expecting 0 <= # < %d"), regnum,
 	   regnum_max_excl);
 
-  /* This routine may be called during early unwinding, at a time
-     where the ID of FRAME is not yet known.  Calling value_from_register
-     would therefore abort in get_frame_id.  However, since we only need
-     a temporary value that is never used as lvalue, we actually do not
-     really need to set its VALUE_NEXT_FRAME_ID.  Therefore, we re-implement
-     the core of value_from_register, but use the null_frame_id.  */
-
   /* Some targets require a special conversion routine even for plain
      pointer types.  Avoid constructing a value object in those cases.  */
   if (gdbarch_convert_register_p (gdbarch, regnum, type))
@@ -915,7 +906,7 @@ address_from_register (int regnum, frame_info_ptr frame)
       return unpack_long (type, buf);
     }
 
-  value *value = gdbarch_value_from_register (gdbarch, type, regnum, nullptr);
+  value *value = gdbarch_value_from_register (gdbarch, type, regnum, frame);
   read_frame_register_value (value, frame);
 
   if (value->optimized_out ())
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index a0f3d94f3a11..478e9078de6f 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2760,13 +2760,11 @@ rs6000_value_from_register (gdbarch *gdbarch, type *type, int regnum,
 
   value->set_lval (lval_register);
 
-  frame_id next_frame_id;
-  if (this_frame == nullptr)
-    next_frame_id = null_frame_id;
-  else
-    next_frame_id = get_frame_id (get_next_frame_sentinel_okay (this_frame));
+  frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
+  while (get_frame_type (next_frame) == INLINE_FRAME)
+    next_frame = get_next_frame_sentinel_okay (next_frame);
 
-  VALUE_NEXT_FRAME_ID (value) = next_frame_id;
+  VALUE_NEXT_FRAME_ID (value) = get_frame_id (next_frame);
   VALUE_REGNUM (value) = regnum;
 
   /* Any structure stored in more than one register will always be
-- 
2.43.0

[PATCH 4/9] gdb: add type parameter to value::allocate_register and add value::allocate_register_lazy

[Simon Marchi]

Some places that create register struct values don't use register_type
to obtain the value type.  This prevents them from using the current
version of value::allocate_register.  One spot (value_of_register_lazy)
also creates a lazy register value.

Add a value::allocate_register_lazy method.  Add some type parameters
to value::allocate_register and value::allocate_register_lazy, to let
the caller specify the type to use for the value.  The parameters
default to nullptr, in which case we use register_type to obtain the
type.

Change-Id: I640ec0a5a0f4a55eba12d515dbfd25933229f8ec
---
 gdb/findvar.c     | 24 ++++++------------------
 gdb/rs6000-tdep.c | 13 ++++---------
 gdb/value.c       | 22 ++++++++++++++++++----
 gdb/value.h       | 13 +++++++++----
 4 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 852f42dc176d..7c6c837ae12e 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -281,12 +281,7 @@ value_of_register_lazy (frame_info_ptr next_frame, int regnum)
   /* We should have a valid next frame.  */
   gdb_assert (frame_id_p (get_frame_id (next_frame)));
 
-  value *reg_val = value::allocate_lazy (register_type (gdbarch, regnum));
-  reg_val->set_lval (lval_register);
-  VALUE_REGNUM (reg_val) = regnum;
-  VALUE_NEXT_FRAME_ID (reg_val) = get_frame_id (next_frame);
-
-  return reg_val;
+  return value::allocate_register_lazy (next_frame, regnum);
 }
 
 /* Given a pointer of type TYPE in target form in BUF, return the
@@ -751,25 +746,20 @@ 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);
-  value->set_lval (lval_register);
-
   frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
   while (get_frame_type (next_frame) == INLINE_FRAME)
     next_frame = get_next_frame_sentinel_okay (next_frame);
 
-  VALUE_NEXT_FRAME_ID (value) = get_frame_id (next_frame);
-  VALUE_REGNUM (value) = regnum;
+  value *value = value::allocate_register (next_frame, regnum, type);
 
   /* Any structure stored in more than one register will always be
      an integral number of registers.  Otherwise, you need to do
      some fiddling with the last register copied here for little
      endian machines.  */
   if (type_byte_order (type) == BFD_ENDIAN_BIG
-      && len < register_size (gdbarch, regnum))
+      && type->length () < register_size (gdbarch, regnum))
     /* Big-endian, and we want less than full size.  */
-    value->set_offset (register_size (gdbarch, regnum) - len);
+    value->set_offset (register_size (gdbarch, regnum) - type->length ());
   else
     value->set_offset (0);
 
@@ -842,10 +832,8 @@ value_from_register (struct type *type, int regnum, frame_info_ptr frame)
 	 the corresponding [integer] type (see Alpha).  The assumption
 	 is that gdbarch_register_to_value populates the entire value
 	 including the location.  */
-      v = value::allocate (type);
-      v->set_lval (lval_register);
-      VALUE_NEXT_FRAME_ID (v) = get_frame_id (get_next_frame_sentinel_okay (frame));
-      VALUE_REGNUM (v) = regnum;
+      v = value::allocate_register (get_next_frame_sentinel_okay (frame),
+				    regnum, type);
       ok = gdbarch_register_to_value (gdbarch, frame, regnum, type1,
 				      v->contents_raw ().data (), &optim,
 				      &unavail);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 478e9078de6f..edf776853e20 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2751,30 +2751,25 @@ 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);
-
   /* We have an IEEE 128-bit float -- need to change regnum mapping from
      fpr to vsr.  */
   regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
 
-  value->set_lval (lval_register);
-
   frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
   while (get_frame_type (next_frame) == INLINE_FRAME)
     next_frame = get_next_frame_sentinel_okay (next_frame);
 
-  VALUE_NEXT_FRAME_ID (value) = get_frame_id (next_frame);
-  VALUE_REGNUM (value) = regnum;
+  value *value
+    = value::allocate_register (next_frame, regnum, type);
 
   /* Any structure stored in more than one register will always be
      an integral number of registers.  Otherwise, you need to do
      some fiddling with the last register copied here for little
      endian machines.  */
   if (type_byte_order (type) == BFD_ENDIAN_BIG
-      && len < register_size (gdbarch, regnum))
+      && type->length () < register_size (gdbarch, regnum))
     /* Big-endian, and we want less than full size.  */
-    value->set_offset (register_size (gdbarch, regnum) - len);
+    value->set_offset (register_size (gdbarch, regnum) - type->length ());
   else
     value->set_offset (0);
 
diff --git a/gdb/value.c b/gdb/value.c
index 7523af142348..7d51396a0e3a 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -961,11 +961,14 @@ value::allocate (struct type *type)
 
 /* See value.h  */
 
-struct value *
-value::allocate_register (frame_info_ptr next_frame, int regnum)
+value *
+value::allocate_register_lazy (frame_info_ptr next_frame, int regnum,
+			       struct type *type)
 {
-  value *result
-    = value::allocate (register_type (frame_unwind_arch (next_frame), regnum));
+  if (type == nullptr)
+    type = register_type (frame_unwind_arch (next_frame), regnum);
+
+  value *result = value::allocate_lazy (type);
 
   result->set_lval (lval_register);
   VALUE_REGNUM (result) = regnum;
@@ -974,6 +977,17 @@ value::allocate_register (frame_info_ptr next_frame, int regnum)
   return result;
 }
 
+/* See value.h  */
+
+value *
+value::allocate_register (frame_info_ptr next_frame, int regnum,
+			  struct type *type)
+{
+  value *result = value::allocate_register_lazy (next_frame, regnum, type);
+  result->set_lazy (false);
+  return result;
+}
+
 /* Allocate a  value  that has the correct length
    for COUNT repetitions of type TYPE.  */
 
diff --git a/gdb/value.h b/gdb/value.h
index d4244dadb91a..78abcac7e739 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -159,12 +159,17 @@ struct value
   /* Allocate a value and its contents for type TYPE.  */
   static struct value *allocate (struct type *type);
 
-  /* Allocate a non-lazy value representing register RENUM in the frame previous
-     to NEXT_FRAME.  The type of the value is found using `register_type`.
-
+  /* Allocate a lazy value representing register REGNUM in the frame previous
+     to NEXT_FRAME.  If TYPE is non-nullptr, use it as the value type.
+     Otherwise, use `register_type` to obtain the type.  */
+  static struct value *allocate_register_lazy (frame_info_ptr next_frame,
+					  int regnum, type *type = nullptr);
+
+  /* Same as `allocate_register_lazy`, but make the value non-lazy.
+  
      The caller is responsible for filling the value's contents.  */
   static struct value *allocate_register (frame_info_ptr next_frame,
-					  int regnum);
+					  int regnum, type *type = nullptr);
 
   /* Create a computed lvalue, with type TYPE, function pointers
      FUNCS, and closure CLOSURE.  */
-- 
2.43.0

[PATCH 5/9] gdb: remove read_frame_register_value's frame parameter

[Simon Marchi]

By now, all register struct values should have a valid next frame id
(assuming they are created using value::allocate_register or
value::allocate_register_lazy), so there should be no need to pass a
frame alongside the value to read_frame_register_value.  Remove the
frame parameter and adjust read_frame_register_value accordingly.

While at it, make read_frame_register_value static, it's only used in
findvar.c.

Change-Id: I118959ef8c628499297c67810916e8ba9934bfac
---
 gdb/findvar.c | 23 +++++++++++++----------
 gdb/value.h   |  3 ---
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 7c6c837ae12e..838d850e821d 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -767,24 +767,27 @@ default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
 }
 
 /* VALUE must be an lval_register value.  If regnum is the value's
-   associated register number, and len the length of the values type,
-   read one or more registers in FRAME, starting with register REGNUM,
+   associated register number, and len the length of the value's type,
+   read one or more registers in VALUE's frame, starting with register REGNUM,
    until we've read LEN bytes.
 
    If any of the registers we try to read are optimized out, then mark the
    complete resulting value as optimized out.  */
 
-void
-read_frame_register_value (struct value *value, frame_info_ptr frame)
+static void
+read_frame_register_value (value *value)
 {
-  struct gdbarch *gdbarch = get_frame_arch (frame);
+  gdb_assert (value->lval () == lval_register);
+
+  frame_info_ptr next_frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (value));
+  gdb_assert (next_frame != nullptr);
+
+  gdbarch *gdbarch = frame_unwind_arch (next_frame);
   LONGEST offset = 0;
   LONGEST reg_offset = value->offset ();
   int regnum = VALUE_REGNUM (value);
   int len = type_length_units (check_typedef (value->type ()));
 
-  gdb_assert (value->lval () == lval_register);
-
   /* Skip registers wholly inside of REG_OFFSET.  */
   while (reg_offset >= register_size (gdbarch, regnum))
     {
@@ -795,7 +798,7 @@ read_frame_register_value (struct value *value, frame_info_ptr frame)
   /* Copy the data.  */
   while (len > 0)
     {
-      struct value *regval = get_frame_register_value (frame, regnum);
+      struct value *regval = frame_unwind_register_value (next_frame, regnum);
       int reg_len = type_length_units (regval->type ()) - reg_offset;
 
       /* If the register length is larger than the number of bytes
@@ -852,7 +855,7 @@ value_from_register (struct type *type, int regnum, frame_info_ptr frame)
       v = gdbarch_value_from_register (gdbarch, type, regnum, frame);
 
       /* Get the data.  */
-      read_frame_register_value (v, frame);
+      read_frame_register_value (v);
     }
 
   return v;
@@ -895,7 +898,7 @@ address_from_register (int regnum, frame_info_ptr frame)
     }
 
   value *value = gdbarch_value_from_register (gdbarch, type, regnum, frame);
-  read_frame_register_value (value, frame);
+  read_frame_register_value (value);
 
   if (value->optimized_out ())
     {
diff --git a/gdb/value.h b/gdb/value.h
index 78abcac7e739..9d7630ef07b3 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -1119,9 +1119,6 @@ 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);
-
 extern struct value *value_from_register (struct type *type, int regnum,
 					  frame_info_ptr frame);
 
-- 
2.43.0

[PATCH 6/9] gdb: implement address_from_register using value_from_register

[Simon Marchi]

As explained in the comment removed by the previous commit "gdb: pass
non-nullptr frame to gdbarch_value_from_register in
address_from_register", address_from_register copies some implementation
bits from value_from_register:

   /* This routine may be called during early unwinding, at a time
      where the ID of FRAME is not yet known.  Calling value_from_register
      would therefore abort in get_frame_id.  However, since we only need
      a temporary value that is never used as lvalue, we actually do not
      really need to set its VALUE_NEXT_FRAME_ID.  Therefore, we re-implement
      the core of value_from_register, but use the null_frame_id.  */

This is no longer relevant, since we now create a value with a valid next
frame id, so change address_from_register to use value_from_register.

Change-Id: I189bd96f28735ed9f47750ffd73764c459ec6f43
---
 gdb/findvar.c | 41 ++++-------------------------------------
 1 file changed, 4 insertions(+), 37 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 838d850e821d..fa014d60291d 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -867,40 +867,10 @@ value_from_register (struct type *type, int regnum, frame_info_ptr frame)
 CORE_ADDR
 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;
-  CORE_ADDR result;
-  int regnum_max_excl = gdbarch_num_cooked_regs (gdbarch);
-
-  if (regnum < 0 || regnum >= regnum_max_excl)
-    error (_("Invalid register #%d, expecting 0 <= # < %d"), regnum,
-	   regnum_max_excl);
+  type *type = builtin_type (get_frame_arch (frame))->builtin_data_ptr;
+  value_ref_ptr v = release_value (value_from_register (type, regnum, frame));
 
-  /* Some targets require a special conversion routine even for plain
-     pointer types.  Avoid constructing a value object in those cases.  */
-  if (gdbarch_convert_register_p (gdbarch, regnum, type))
-    {
-      gdb_byte *buf = (gdb_byte *) alloca (type->length ());
-      int optim, unavail, ok;
-
-      ok = gdbarch_register_to_value (gdbarch, frame, regnum, type,
-				      buf, &optim, &unavail);
-      if (!ok)
-	{
-	  /* This function is used while computing a location expression.
-	     Complain about the value being optimized out, rather than
-	     letting value_as_address complain about some random register
-	     the expression depends on not being saved.  */
-	  error_value_optimized_out ();
-	}
-
-      return unpack_long (type, buf);
-    }
-
-  value *value = gdbarch_value_from_register (gdbarch, type, regnum, frame);
-  read_frame_register_value (value);
-
-  if (value->optimized_out ())
+  if (v->optimized_out ())
     {
       /* This function is used while computing a location expression.
 	 Complain about the value being optimized out, rather than
@@ -909,10 +879,7 @@ address_from_register (int regnum, frame_info_ptr frame)
       error_value_optimized_out ();
     }
 
-  result = value_as_address (value);
-  release_value (value);
-
-  return result;
+  return value_as_address (v.get ());
 }
 
 #if GDB_SELF_TEST
-- 
2.43.0

[PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Simon Marchi]

Remove VALUE_NEXT_FRAME_ID, replace it with a method on struct value.  Set
`m_location.reg.next_frame_id` directly from value::allocate_register_lazy,
which is fine because allocate_register_lazy is a static creation
function for struct value.

Change-Id: Ic9f0f239c166a88dccfee836f9f51871e67548e6
---
 gdb/findvar.c |  2 +-
 gdb/valops.c  |  7 +++----
 gdb/value.c   | 32 ++++++++++++--------------------
 gdb/value.h   | 11 ++++-------
 4 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index fa014d60291d..ef7129dab331 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -779,7 +779,7 @@ read_frame_register_value (value *value)
 {
   gdb_assert (value->lval () == lval_register);
 
-  frame_info_ptr next_frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (value));
+  frame_info_ptr next_frame = frame_find_by_id (value->next_frame_id ());
   gdb_assert (next_frame != nullptr);
 
   gdbarch *gdbarch = frame_unwind_arch (next_frame);
diff --git a/gdb/valops.c b/gdb/valops.c
index 049314cf7db5..5a5b3f14ad44 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1193,7 +1193,7 @@ value_assign (struct value *toval, struct value *fromval)
 
     case lval_register:
       {
-	frame_info_ptr next_frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (toval));
+	frame_info_ptr next_frame = frame_find_by_id (toval->next_frame_id ());
 
 	int value_reg = VALUE_REGNUM (toval);
 
@@ -1410,11 +1410,10 @@ address_of_variable (struct symbol *var, const struct block *b)
     {
     case lval_register:
       {
-	frame_info_ptr frame;
 	const char *regname;
 
-	frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (val));
-	gdb_assert (frame);
+	frame_info_ptr frame = frame_find_by_id (val->next_frame_id ());
+	gdb_assert (frame != nullptr);
 
 	regname = gdbarch_register_name (get_frame_arch (frame),
 					 VALUE_REGNUM (val));
diff --git a/gdb/value.c b/gdb/value.c
index 7d51396a0e3a..a16ba2fb5d6b 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -972,7 +972,7 @@ value::allocate_register_lazy (frame_info_ptr next_frame, int regnum,
 
   result->set_lval (lval_register);
   VALUE_REGNUM (result) = regnum;
-  VALUE_NEXT_FRAME_ID (result) = get_frame_id (next_frame);
+  result->m_location.reg.next_frame_id = get_frame_id (next_frame);
 
   return result;
 }
@@ -1421,11 +1421,12 @@ value::set_address (CORE_ADDR addr)
   m_location.address = addr;
 }
 
-struct frame_id *
-value::deprecated_next_frame_id_hack ()
+frame_id
+value::next_frame_id ()
 {
   gdb_assert (m_lval == lval_register);
-  return &m_location.reg.next_frame_id;
+
+  return m_location.reg.next_frame_id;
 }
 
 int *
@@ -3928,7 +3929,7 @@ value::fetch_lazy_memory ()
 void
 value::fetch_lazy_register ()
 {
-  frame_info_ptr next_frame;
+ 
   int regnum;
   struct type *type = check_typedef (this->type ());
   struct value *new_val = this;
@@ -3941,13 +3942,12 @@ value::fetch_lazy_register ()
 
   while (new_val->lval () == lval_register && new_val->lazy ())
     {
-      struct frame_id next_frame_id = VALUE_NEXT_FRAME_ID (new_val);
+      frame_id next_frame_id = new_val->next_frame_id ();
+      frame_info_ptr next_frame = frame_find_by_id (next_frame_id);
+      gdb_assert (next_frame != NULL);
 
-      next_frame = frame_find_by_id (next_frame_id);
       regnum = VALUE_REGNUM (new_val);
 
-      gdb_assert (next_frame != NULL);
-
       /* Convertible register routines are used for multi-register
 	 values and for interpretation in different types
 	 (e.g. float or int from a double register).  Lazy
@@ -3956,12 +3956,6 @@ value::fetch_lazy_register ()
       gdb_assert (!gdbarch_convert_register_p (get_frame_arch (next_frame),
 					       regnum, type));
 
-      /* FRAME was obtained, above, via VALUE_NEXT_FRAME_ID.
-	 Since a "->next" operation was performed when setting
-	 this field, we do not need to perform a "next" operation
-	 again when unwinding the register.  That's why
-	 frame_unwind_register_value() is called here instead of
-	 get_frame_register_value().  */
       new_val = frame_unwind_register_value (next_frame, regnum);
 
       /* If we get another lazy lval_register value, it means the
@@ -3976,7 +3970,7 @@ value::fetch_lazy_register ()
 	 in this situation.  */
       if (new_val->lval () == lval_register
 	  && new_val->lazy ()
-	  && VALUE_NEXT_FRAME_ID (new_val) == next_frame_id)
+	  && new_val->next_frame_id () == next_frame_id)
 	internal_error (_("infinite loop while fetching a register"));
     }
 
@@ -3994,12 +3988,10 @@ value::fetch_lazy_register ()
 
   if (frame_debug)
     {
-      struct gdbarch *gdbarch;
-      frame_info_ptr frame;
-      frame = frame_find_by_id (VALUE_NEXT_FRAME_ID (this));
+      frame_info_ptr frame = frame_find_by_id (this->next_frame_id ());
       frame = get_prev_frame_always (frame);
       regnum = VALUE_REGNUM (this);
-      gdbarch = get_frame_arch (frame);
+      gdbarch *gdbarch = get_frame_arch (frame);
 
       string_file debug_file;
       gdb_printf (&debug_file,
diff --git a/gdb/value.h b/gdb/value.h
index 9d7630ef07b3..c33d2d8f0cd1 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -373,7 +373,10 @@ struct value
   struct internalvar **deprecated_internalvar_hack ()
   { return &m_location.internalvar; }
 
-  struct frame_id *deprecated_next_frame_id_hack ();
+  /* Return this value's next frame id.
+
+     The value must be of lval == lval_register.  */
+  frame_id next_frame_id ();
 
   int *deprecated_regnum_hack ();
 
@@ -964,12 +967,6 @@ extern void error_value_optimized_out (void);
 /* Pointer to internal variable.  */
 #define VALUE_INTERNALVAR(val) (*((val)->deprecated_internalvar_hack ()))
 
-/* Frame ID of "next" frame to which a register value is relative.  A
-   register value is indicated by VALUE_LVAL being set to lval_register.
-   So, if the register value is found relative to frame F, then the
-   frame id of F->next will be stored in VALUE_NEXT_FRAME_ID.  */
-#define VALUE_NEXT_FRAME_ID(val) (*((val)->deprecated_next_frame_id_hack ()))
-
 /* Register number if the value is from a register.  */
 #define VALUE_REGNUM(val) (*((val)->deprecated_regnum_hack ()))
 
-- 
2.43.0

Re: [PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Remove VALUE_NEXT_FRAME_ID, replace it with a method on struct value.  Set
Simon> `m_location.reg.next_frame_id` directly from value::allocate_register_lazy,
Simon> which is fine because allocate_register_lazy is a static creation
Simon> function for struct value.

I looks like every user except value::fetch_lazy_register actually
immediatley calls find_frame_by_id with the result; so I wonder if this
should just return a frame_info_ptr instead.

BTW thank you for doing this, I'm happy to see VALUE_NEXT_FRAME_ID go away.

Tom

Re: [PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Simon Marchi]

On 2023-12-22 11:51, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Remove VALUE_NEXT_FRAME_ID, replace it with a method on struct value.  Set
> Simon> `m_location.reg.next_frame_id` directly from value::allocate_register_lazy,
> Simon> which is fine because allocate_register_lazy is a static creation
> Simon> function for struct value.
> 
> I looks like every user except value::fetch_lazy_register actually
> immediatley calls find_frame_by_id with the result; so I wonder if this
> should just return a frame_info_ptr instead.

I'm not sure I understand, which function do you think should return a
frame_info_ptr?

Simon

Re: [PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> I'm not sure I understand, which function do you think should return a
Simon> frame_info_ptr?

value::next_frame_id (though also with a new name).

value::fetch_lazy_register can just access the frame_id directly if it
really needs to.

I don't feel super strongly about this.

Tom

Re: [PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Simon Marchi]

On 2023-12-22 12:02, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> I'm not sure I understand, which function do you think should return a
> Simon> frame_info_ptr?
> 
> value::next_frame_id (though also with a new name).
> 
> value::fetch_lazy_register can just access the frame_id directly if it
> really needs to.
> 
> I don't feel super strongly about this.

Ah ok I see.  Well, even value::fetch_lazy_register does call
frame_find_by_id, so I guess it would make sense, it wouldn't be a
pessimization.

Simon

Re: [PATCH 7/9] gdb: remove VALUE_NEXT_FRAME_ID, add value::next_frame_id

[Simon Marchi]

On 2023-12-22 12:06, Simon Marchi wrote:
> 
> 
> On 2023-12-22 12:02, Tom Tromey wrote:
>>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
>>
>> Simon> I'm not sure I understand, which function do you think should return a
>> Simon> frame_info_ptr?
>>
>> value::next_frame_id (though also with a new name).
>>
>> value::fetch_lazy_register can just access the frame_id directly if it
>> really needs to.
>>
>> I don't feel super strongly about this.
> 
> Ah ok I see.  Well, even value::fetch_lazy_register does call
> frame_find_by_id, so I guess it would make sense, it wouldn't be a
> pessimization.

So, there's one call in fetch_lazy_register that just wants the frame
id, the call on `new_val`.  So I will leave it as is for now.

I had the idea of making it so that value::m_location stores a
frame_info_ptr to the next frame, instead of a frame id.  Now that frame
infos are reinflatable, it should be safe to do so.  And then we could
return a frame_info_ptr at no additional cost.  But a first step
would probably be to convert the m_location union to a variant, because
we need the frame_info_ptr ctor and dtor to be called.

Simon

[PATCH 8/9] gdb: remove VALUE_REGNUM, add value::regnum

[Simon Marchi]

Remove VALUE_REGNUM, replace it with a method on struct value.  Set
`m_location.reg.regnum` directly from value::allocate_register_lazy,
which is fine because allocate_register_lazy is a static creation
function for struct value.

Change-Id: Id632502357da971617d9dce1e2eab9b56dbcf52d
---
 gdb/findvar.c          |  2 +-
 gdb/frame.c            |  7 +++----
 gdb/python/py-unwind.c |  2 +-
 gdb/stack.c            |  3 +--
 gdb/valops.c           | 15 ++++++---------
 gdb/value.c            | 20 ++++++++++----------
 gdb/value.h            |  8 ++++----
 7 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index ef7129dab331..7c360eb37ff9 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -785,7 +785,7 @@ read_frame_register_value (value *value)
   gdbarch *gdbarch = frame_unwind_arch (next_frame);
   LONGEST offset = 0;
   LONGEST reg_offset = value->offset ();
-  int regnum = VALUE_REGNUM (value);
+  int regnum = value->regnum ();
   int len = type_length_units (check_typedef (value->type ()));
 
   /* Skip registers wholly inside of REG_OFFSET.  */
diff --git a/gdb/frame.c b/gdb/frame.c
index 5f4c8c621a04..41003cc7771e 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1196,7 +1196,7 @@ frame_register_unwind (frame_info_ptr next_frame, int regnum,
   *lvalp = value->lval ();
   *addrp = value->address ();
   if (*lvalp == lval_register)
-    *realnump = VALUE_REGNUM (value);
+    *realnump = value->regnum ();
   else
     *realnump = -1;
 
@@ -1304,8 +1304,7 @@ frame_unwind_register_value (frame_info_ptr next_frame, int regnum)
       else
 	{
 	  if (value->lval () == lval_register)
-	    gdb_printf (&debug_file, " register=%d",
-			VALUE_REGNUM (value));
+	    gdb_printf (&debug_file, " register=%d", value->regnum ());
 	  else if (value->lval () == lval_memory)
 	    gdb_printf (&debug_file, " address=%s",
 			paddress (gdbarch,
@@ -1417,7 +1416,7 @@ read_frame_register_unsigned (frame_info_ptr frame, int regnum,
     {
       struct gdbarch *gdbarch = get_frame_arch (frame);
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
-      int size = register_size (gdbarch, VALUE_REGNUM (regval));
+      int size = register_size (gdbarch, regval->regnum ());
 
       *val = extract_unsigned_integer (regval->contents ().data (), size,
 				       byte_order);
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 8fed55beadca..f12485c22b77 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -336,7 +336,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args, PyObject *kw)
       struct value *user_reg_value
 	= value_of_user_reg (regnum, pending_frame->frame_info);
       if (user_reg_value->lval () == lval_register)
-	regnum = VALUE_REGNUM (user_reg_value);
+	regnum = user_reg_value->regnum ();
       if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
 	{
 	  PyErr_SetString (PyExc_ValueError, "Bad register");
diff --git a/gdb/stack.c b/gdb/stack.c
index 20bb85efd19e..fad4b62c6f72 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -1724,8 +1724,7 @@ info_frame_command_core (frame_info_ptr fi, bool selected_frame_p)
 	    else if (value->lval () == lval_register)
 	      {
 		gdb_printf (" Previous frame's sp in %s\n",
-			    gdbarch_register_name (gdbarch,
-						   VALUE_REGNUM (value)));
+			    gdbarch_register_name (gdbarch, value->regnum ()));
 	      }
 
 	    release_value (value);
diff --git a/gdb/valops.c b/gdb/valops.c
index 5a5b3f14ad44..16cdf1f45530 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -1194,8 +1194,7 @@ value_assign (struct value *toval, struct value *fromval)
     case lval_register:
       {
 	frame_info_ptr next_frame = frame_find_by_id (toval->next_frame_id ());
-
-	int value_reg = VALUE_REGNUM (toval);
+	int value_reg = toval->regnum ();
 
 	if (next_frame == nullptr)
 	  error (_("Value being assigned to is no longer active."));
@@ -1240,15 +1239,13 @@ value_assign (struct value *toval, struct value *fromval)
 	  }
 	else
 	  {
-	    if (gdbarch_convert_register_p (gdbarch, VALUE_REGNUM (toval),
-					    type))
+	    if (gdbarch_convert_register_p (gdbarch, toval->regnum (), type))
 	      {
 		/* If TOVAL is a special machine register requiring
 		   conversion of program values to a special raw
 		   format.  */
-		gdbarch_value_to_register (gdbarch,
-					   get_prev_frame_always (next_frame),
-					   VALUE_REGNUM (toval), type,
+		gdbarch_value_to_register (gdbarch, next_frame,
+					   toval->regnum (), type,
 					   fromval->contents ().data ());
 	      }
 	    else
@@ -1415,8 +1412,8 @@ address_of_variable (struct symbol *var, const struct block *b)
 	frame_info_ptr frame = frame_find_by_id (val->next_frame_id ());
 	gdb_assert (frame != nullptr);
 
-	regname = gdbarch_register_name (get_frame_arch (frame),
-					 VALUE_REGNUM (val));
+	regname
+	  = gdbarch_register_name (get_frame_arch (frame), val->regnum ());
 	gdb_assert (regname != nullptr && *regname != '\0');
 
 	error (_("Address requested for identifier "
diff --git a/gdb/value.c b/gdb/value.c
index a16ba2fb5d6b..a69bc348167a 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -971,7 +971,7 @@ value::allocate_register_lazy (frame_info_ptr next_frame, int regnum,
   value *result = value::allocate_lazy (type);
 
   result->set_lval (lval_register);
-  VALUE_REGNUM (result) = regnum;
+  result->m_location.reg.regnum = regnum;
   result->m_location.reg.next_frame_id = get_frame_id (next_frame);
 
   return result;
@@ -1429,11 +1429,14 @@ value::next_frame_id ()
   return m_location.reg.next_frame_id;
 }
 
-int *
-value::deprecated_regnum_hack ()
+/* See value.h.  */
+
+int
+value::regnum ()
 {
   gdb_assert (m_lval == lval_register);
-  return &m_location.reg.regnum;
+
+  return m_location.reg.regnum;
 }
 
 \f
@@ -3929,8 +3932,6 @@ value::fetch_lazy_memory ()
 void
 value::fetch_lazy_register ()
 {
- 
-  int regnum;
   struct type *type = check_typedef (this->type ());
   struct value *new_val = this;
 
@@ -3946,7 +3947,7 @@ value::fetch_lazy_register ()
       frame_info_ptr next_frame = frame_find_by_id (next_frame_id);
       gdb_assert (next_frame != NULL);
 
-      regnum = VALUE_REGNUM (new_val);
+      int regnum = new_val->regnum ();
 
       /* Convertible register routines are used for multi-register
 	 values and for interpretation in different types
@@ -3990,7 +3991,7 @@ value::fetch_lazy_register ()
     {
       frame_info_ptr frame = frame_find_by_id (this->next_frame_id ());
       frame = get_prev_frame_always (frame);
-      regnum = VALUE_REGNUM (this);
+      int regnum = this->regnum ();
       gdbarch *gdbarch = get_frame_arch (frame);
 
       string_file debug_file;
@@ -4011,8 +4012,7 @@ value::fetch_lazy_register ()
 	  gdb::array_view<const gdb_byte> buf = new_val->contents ();
 
 	  if (new_val->lval () == lval_register)
-	    gdb_printf (&debug_file, " register=%d",
-			VALUE_REGNUM (new_val));
+	    gdb_printf (&debug_file, " register=%d", new_val->regnum ());
 	  else if (new_val->lval () == lval_memory)
 	    gdb_printf (&debug_file, " address=%s",
 			paddress (gdbarch,
diff --git a/gdb/value.h b/gdb/value.h
index c33d2d8f0cd1..f1202007bb4f 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -378,7 +378,10 @@ struct value
      The value must be of lval == lval_register.  */
   frame_id next_frame_id ();
 
-  int *deprecated_regnum_hack ();
+  /* Return this value's register number.
+
+     The value must be of lval == lval_register.  */
+  int regnum ();
 
   /* contents() and contents_raw() both return the address of the gdb
      buffer used to hold a copy of the contents of the lval.
@@ -967,9 +970,6 @@ extern void error_value_optimized_out (void);
 /* Pointer to internal variable.  */
 #define VALUE_INTERNALVAR(val) (*((val)->deprecated_internalvar_hack ()))
 
-/* Register number if the value is from a register.  */
-#define VALUE_REGNUM(val) (*((val)->deprecated_regnum_hack ()))
-
 /* Return value after lval_funcs->coerce_ref (after check_typedef).  Return
    NULL if lval_funcs->coerce_ref is not applicable for whatever reason.  */
 
-- 
2.43.0

Re: [PATCH 8/9] gdb: remove VALUE_REGNUM, add value::regnum

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> +/* See value.h.  */
Simon> +
Simon> +int
Simon> +value::regnum ()
Simon>  {
Simon>    gdb_assert (m_lval == lval_register);
Simon> -  return &m_location.reg.regnum;
Simon> +
Simon> +  return m_location.reg.regnum;

I wonder if this could just be inlined in the header.

Tom

Re: [PATCH 8/9] gdb: remove VALUE_REGNUM, add value::regnum

[Simon Marchi]

On 2023-12-22 11:52, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> +/* See value.h.  */
> Simon> +
> Simon> +int
> Simon> +value::regnum ()
> Simon>  {
> Simon>    gdb_assert (m_lval == lval_register);
> Simon> -  return &m_location.reg.regnum;
> Simon> +
> Simon> +  return m_location.reg.regnum;
> 
> I wonder if this could just be inlined in the header.
> 
> Tom

Yes, I think it's a good candidate to be in the header, I'll do that.

Simon

[PATCH 9/9] gdb: make value::allocate_register_lazy store id of next non-inline frame

[Simon Marchi]

Some spots loop on the frame chain to find the first next non-inline
frame, and pass that as the "next frame" to
value::allocate_register_lazy / value::allocate_register.  This is
necessary if the value is used in the process of computing the id of
"this frame".  If the frame next to "this frame" is inlined into "this
frame", then you that next frame won't have a computed id yet.  You have
to go past that to find the next non-inline frame, which will have a
computed id.

In other cases, it's fine to store the id of an inline frame as the
"next frame id" in a register struct value.  When trying to unwind a
register from it, it will just call inline_frame_prev_register, which
will forward the request to the next next frame, until we hit the next
physical frame.

I think it would make things simpler to just never store the id of an
inline frame as the next frame id of register struct values, and go with
the first next non-inline frame directly.  This way, we don't have to
wonder which code paths have to skip inline frames when creating
register values and which don't.

So, change value::allocate_register_lazy to do that work, and remove the
loops for the callers that did it.

Change-Id: Ic88115dac49dc14e3053c95f92050062b24b7310
---
 gdb/findvar.c     | 19 +++----------------
 gdb/rs6000-tdep.c |  7 ++-----
 gdb/value.c       | 12 ++++++++++++
 gdb/value.h       |  8 ++++----
 4 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/gdb/findvar.c b/gdb/findvar.c
index 7c360eb37ff9..90d3385b36d8 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -270,17 +270,6 @@ value_of_register_lazy (frame_info_ptr next_frame, int regnum)
   gdb_assert (regnum < gdbarch_num_cooked_regs (gdbarch));
   gdb_assert (next_frame != nullptr);
 
-  /* In some cases NEXT_FRAME may not have a valid frame-id yet.  This can
-     happen if we end up trying to unwind a register as part of the frame
-     sniffer.  The only time that we get here without a valid frame-id is
-     if NEXT_FRAME is an inline frame.  If this is the case then we can
-     avoid getting into trouble here by skipping past the inline frames.  */
-  while (get_frame_type (next_frame) == INLINE_FRAME)
-    next_frame = get_next_frame_sentinel_okay (next_frame);
-
-  /* We should have a valid next frame.  */
-  gdb_assert (frame_id_p (get_frame_id (next_frame)));
-
   return value::allocate_register_lazy (next_frame, regnum);
 }
 
@@ -746,11 +735,9 @@ value *
 default_value_from_register (gdbarch *gdbarch, type *type, int regnum,
 			     frame_info_ptr this_frame)
 {
-  frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
-  while (get_frame_type (next_frame) == INLINE_FRAME)
-    next_frame = get_next_frame_sentinel_okay (next_frame);
-
-  value *value = value::allocate_register (next_frame, regnum, type);
+  value *value
+    = value::allocate_register (get_next_frame_sentinel_okay (this_frame),
+				regnum, type);
 
   /* Any structure stored in more than one register will always be
      an integral number of registers.  Otherwise, you need to do
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index edf776853e20..1c9bb3e5f04f 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -2755,12 +2755,9 @@ rs6000_value_from_register (gdbarch *gdbarch, type *type, int regnum,
      fpr to vsr.  */
   regnum = ieee_128_float_regnum_adjust (gdbarch, type, regnum);
 
-  frame_info_ptr next_frame = get_next_frame_sentinel_okay (this_frame);
-  while (get_frame_type (next_frame) == INLINE_FRAME)
-    next_frame = get_next_frame_sentinel_okay (next_frame);
-
   value *value
-    = value::allocate_register (next_frame, regnum, type);
+    = value::allocate_register (get_next_frame_sentinel_okay (this_frame),
+				regnum, type);
 
   /* Any structure stored in more than one register will always be
      an integral number of registers.  Otherwise, you need to do
diff --git a/gdb/value.c b/gdb/value.c
index a69bc348167a..92e62d655d0c 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -972,8 +972,20 @@ value::allocate_register_lazy (frame_info_ptr next_frame, int regnum,
 
   result->set_lval (lval_register);
   result->m_location.reg.regnum = regnum;
+
+  /* If this register value is created during unwind (while computing a frame
+     id), and NEXT_FRAME is a frame inlined in the frame being unwound, then
+     NEXT_FRAME will not have a valid frame id yet.  Find the next non-inline
+     frame (possibly the sentinel frame).  This is where registers are unwound
+     from anyway.  */
+  while (get_frame_type (next_frame) == INLINE_FRAME)
+    next_frame = get_next_frame_sentinel_okay (next_frame);
+
   result->m_location.reg.next_frame_id = get_frame_id (next_frame);
 
+  /* We should have a next frame with a valid id.  */
+  gdb_assert (frame_id_p (result->m_location.reg.next_frame_id));
+
   return result;
 }
 
diff --git a/gdb/value.h b/gdb/value.h
index f1202007bb4f..22b3a09e5a5b 100644
--- a/gdb/value.h
+++ b/gdb/value.h
@@ -680,10 +680,10 @@ struct value
     {
       /* Register number.  */
       int regnum;
-      /* Frame ID of "next" frame to which a register value is relative.
-	 If the register value is found relative to frame F, then the
-	 frame id of F->next will be stored in next_frame_id.  */
-      struct frame_id next_frame_id;
+
+      /* Frame ID of the next physical (non-inline) frame to which a register
+	 value is relative.  */
+      frame_id next_frame_id;
     } reg;
 
     /* Pointer to internal variable.  */
-- 
2.43.0

Re: [PATCH 0/9] Some register value cleanups

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> I'm currently investigating some things related to register read and
Simon> spotted some improvements that could be made to make the code a bit
Simon> simpler or more robust, here they are.

Thank you for doing this.  I sent a few minor notes, but overall I'm
very happy to see this series.

Tom

Re: [PATCH 0/9] Some register value cleanups

[Simon Marchi]

On 2023-12-22 11:53, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> I'm currently investigating some things related to register read and
> Simon> spotted some improvements that could be made to make the code a bit
> Simon> simpler or more robust, here they are.
> 
> Thank you for doing this.  I sent a few minor notes, but overall I'm
> very happy to see this series.
> 
> Tom

Thanks, I'll apply your comments, and I'll push it if there are no
significant changes that need re-review.

Simon

Re: [PATCH 0/9] Some register value cleanups

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:

Simon> Thanks, I'll apply your comments, and I'll push it if there are no
Simon> significant changes that need re-review.

Feel free to stick this on:
Approved-By: Tom Tromey <tom@tromey.com>

Tom

Re: [PATCH 0/9] Some register value cleanups

[Simon Marchi]

On 2023-12-22 12:02, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@efficios.com> writes:
> 
> Simon> Thanks, I'll apply your comments, and I'll push it if there are no
> Simon> significant changes that need re-review.
> 
> Feel free to stick this on:
> Approved-By: Tom Tromey <tom@tromey.com>
> 
> Tom

I pushed the series with the small comments addressed, I left out the
one to change value::next_frame_id to return a frame_info_ptr.  I forgot
to add the Approved-By, sorry :(.

Simon