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

Message ID 20231221191716.257256-8-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_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm 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
  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(-)
  

Comments

Tom Tromey Dec. 22, 2023, 4:51 p.m. UTC | #1
>>>>> "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
  
Simon Marchi Dec. 22, 2023, 4:56 p.m. UTC | #2
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
  
Tom Tromey Dec. 22, 2023, 5:02 p.m. UTC | #3
>>>>> "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
  
Simon Marchi Dec. 22, 2023, 5:06 p.m. UTC | #4
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
  
Simon Marchi Dec. 24, 2023, 3:35 p.m. UTC | #5
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

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