[v2] gdb: remove SYMBOL_*_OPS macros

Message ID 20240119203246.83805-1-simon.marchi@efficios.com
State New
Headers
Series [v2] gdb: remove SYMBOL_*_OPS macros |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
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_check--master-aarch64 warning Patch is already merged

Commit Message

Simon Marchi Jan. 19, 2024, 8:32 p.m. UTC
  New in v2:

 - remove one use of auto in findvar.c
 - fix formatting in frame.c

Remove SYMBOL_BLOCK_OPS, SYMBOL_COMPUTED_OPS and SYMBOL_REGISTER_OPS, in
favor of methods on struct symbol.  More changes could be done here to
improve the design and make things safer, but I just wanted to do a
straightforward change to remove the macros for now.

Change-Id: I27adb74a28ea3c0dc9a85c2953413437cd95ad21
---
 gdb/ax-gdb.c                    | 14 +++++---------
 gdb/compile/compile-c-symbols.c | 11 +++++------
 gdb/dwarf2/loc.c                | 23 +++++++++++------------
 gdb/dwarf2/read.c               |  2 +-
 gdb/eval.c                      |  7 ++++---
 gdb/findvar.c                   | 13 +++++++------
 gdb/frame.c                     | 15 +++++++++------
 gdb/printcmd.c                  | 10 +++++-----
 gdb/stack.c                     | 16 +++++++---------
 gdb/symtab.h                    | 25 +++++++++++++++++++------
 gdb/tracepoint.c                | 16 +++++++---------
 11 files changed, 80 insertions(+), 72 deletions(-)


base-commit: 74d8fa2df7e7844f9b1b8fac27fe7d0b82100ab0
  

Comments

Kevin Buettner Jan. 19, 2024, 10:45 p.m. UTC | #1
On Fri, 19 Jan 2024 15:32:32 -0500
Simon Marchi <simon.marchi@efficios.com> wrote:

> New in v2:
> 
>  - remove one use of auto in findvar.c
>  - fix formatting in frame.c
> 
> Remove SYMBOL_BLOCK_OPS, SYMBOL_COMPUTED_OPS and SYMBOL_REGISTER_OPS, in
> favor of methods on struct symbol.  More changes could be done here to
> improve the design and make things safer, but I just wanted to do a
> straightforward change to remove the macros for now.

LGTM.

Reviewed-by: Kevin Buettner <kevinb@redhat.com>
  
Simon Marchi Jan. 20, 2024, 3:15 a.m. UTC | #2
On 2024-01-19 17:45, Kevin Buettner wrote:
> On Fri, 19 Jan 2024 15:32:32 -0500
> Simon Marchi <simon.marchi@efficios.com> wrote:
> 
>> New in v2:
>>
>>  - remove one use of auto in findvar.c
>>  - fix formatting in frame.c
>>
>> Remove SYMBOL_BLOCK_OPS, SYMBOL_COMPUTED_OPS and SYMBOL_REGISTER_OPS, in
>> favor of methods on struct symbol.  More changes could be done here to
>> improve the design and make things safer, but I just wanted to do a
>> straightforward change to remove the macros for now.
> 
> LGTM.
> 
> Reviewed-by: Kevin Buettner <kevinb@redhat.com>
> 

Thanks, pushed.

Simon
  

Patch

diff --git a/gdb/ax-gdb.c b/gdb/ax-gdb.c
index deadbc1bc022..c56d48683105 100644
--- a/gdb/ax-gdb.c
+++ b/gdb/ax-gdb.c
@@ -521,11 +521,9 @@  gen_var_ref (struct agent_expr *ax, struct axs_value *value, struct symbol *var)
   value->type = check_typedef (var->type ());
   value->optimized_out = 0;
 
-  if (SYMBOL_COMPUTED_OPS (var) != NULL)
-    {
-      SYMBOL_COMPUTED_OPS (var)->tracepoint_var_ref (var, ax, value);
-      return;
-    }
+  if (const symbol_computed_ops *computed_ops = var->computed_ops ();
+      computed_ops != nullptr)
+    return computed_ops->tracepoint_var_ref (var, ax, value);
 
   /* I'm imitating the code in read_var_value.  */
   switch (var->aclass ())
@@ -587,8 +585,7 @@  gen_var_ref (struct agent_expr *ax, struct axs_value *value, struct symbol *var)
 	 this as an lvalue or rvalue, the caller will generate the
 	 right code.  */
       value->kind = axs_lvalue_register;
-      value->u.reg
-	= SYMBOL_REGISTER_OPS (var)->register_number (var, ax->gdbarch);
+      value->u.reg = var->register_ops ()->register_number (var, ax->gdbarch);
       break;
 
       /* A lot like LOC_REF_ARG, but the pointer lives directly in a
@@ -596,8 +593,7 @@  gen_var_ref (struct agent_expr *ax, struct axs_value *value, struct symbol *var)
 	 because it's just like any other case where the thing
 	 has a real address.  */
     case LOC_REGPARM_ADDR:
-      ax_reg (ax,
-	      SYMBOL_REGISTER_OPS (var)->register_number (var, ax->gdbarch));
+      ax_reg (ax, var->register_ops ()->register_number (var, ax->gdbarch));
       value->kind = axs_lvalue_memory;
       break;
 
diff --git a/gdb/compile/compile-c-symbols.c b/gdb/compile/compile-c-symbols.c
index f55438d9ccbd..9e15709c4e54 100644
--- a/gdb/compile/compile-c-symbols.c
+++ b/gdb/compile/compile-c-symbols.c
@@ -563,7 +563,8 @@  generate_c_for_for_one_variable (compile_instance *compiler,
 	  stream->write (local_file.c_str (), local_file.size ());
 	}
 
-      if (SYMBOL_COMPUTED_OPS (sym) != NULL)
+      if (const symbol_computed_ops *computed_ops = sym->computed_ops ();
+	  computed_ops != nullptr)
 	{
 	  gdb::unique_xmalloc_ptr<char> generated_name
 	    = c_symbol_substitution_name (sym);
@@ -571,11 +572,9 @@  generate_c_for_for_one_variable (compile_instance *compiler,
 	     occurs in the middle.  */
 	  string_file local_file;
 
-	  SYMBOL_COMPUTED_OPS (sym)->generate_c_location (sym, &local_file,
-							  gdbarch,
-							  registers_used,
-							  pc,
-							  generated_name.get ());
+	  computed_ops->generate_c_location (sym, &local_file, gdbarch,
+					     registers_used, pc,
+					     generated_name.get ());
 	  stream->write (local_file.c_str (), local_file.size ());
 	}
       else
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 062bd12909fa..8a350b402587 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -515,14 +515,15 @@  locexpr_get_frame_base (struct symbol *framefunc, frame_info_ptr frame)
   /* If this method is called, then FRAMEFUNC is supposed to be a DWARF block.
      Thus, it's supposed to provide the find_frame_base_location method as
      well.  */
-  gdb_assert (SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location != NULL);
+  gdb_assert (framefunc->block_ops ()->find_frame_base_location != nullptr);
 
   gdbarch = get_frame_arch (frame);
   type = builtin_type (gdbarch)->builtin_data_ptr;
   dlbaton = (struct dwarf2_locexpr_baton *) SYMBOL_LOCATION_BATON (framefunc);
 
-  SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location
-    (framefunc, get_frame_pc (frame), &start, &length);
+  framefunc->block_ops ()->find_frame_base_location (framefunc,
+						     get_frame_pc (frame),
+						     &start, &length);
   result = dwarf2_evaluate_loc_desc (type, frame, start, length,
 				     dlbaton->per_cu, dlbaton->per_objfile);
 
@@ -572,14 +573,15 @@  loclist_get_frame_base (struct symbol *framefunc, frame_info_ptr frame)
   /* If this method is called, then FRAMEFUNC is supposed to be a DWARF block.
      Thus, it's supposed to provide the find_frame_base_location method as
      well.  */
-  gdb_assert (SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location != NULL);
+  gdb_assert (framefunc->block_ops ()->find_frame_base_location != nullptr);
 
   gdbarch = get_frame_arch (frame);
   type = builtin_type (gdbarch)->builtin_data_ptr;
   dlbaton = (struct dwarf2_loclist_baton *) SYMBOL_LOCATION_BATON (framefunc);
 
-  SYMBOL_BLOCK_OPS (framefunc)->find_frame_base_location
-    (framefunc, get_frame_pc (frame), &start, &length);
+  framefunc->block_ops ()->find_frame_base_location (framefunc,
+						     get_frame_pc (frame),
+						     &start, &length);
   result = dwarf2_evaluate_loc_desc (type, frame, start, length,
 				     dlbaton->per_cu, dlbaton->per_objfile);
 
@@ -606,12 +608,9 @@  void
 func_get_frame_base_dwarf_block (struct symbol *framefunc, CORE_ADDR pc,
 				 const gdb_byte **start, size_t *length)
 {
-  if (SYMBOL_BLOCK_OPS (framefunc) != NULL)
-    {
-      const struct symbol_block_ops *ops_block = SYMBOL_BLOCK_OPS (framefunc);
-
-      ops_block->find_frame_base_location (framefunc, pc, start, length);
-    }
+  if (const symbol_block_ops *block_ops = framefunc->block_ops ();
+      block_ops != nullptr)
+    block_ops->find_frame_base_location (framefunc, pc, start, length);
   else
     *length = 0;
 
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 7691fe0050b6..69dbeae7ea1a 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18693,7 +18693,7 @@  var_decode_location (struct attribute *attr, struct symbol *sym,
 
   dwarf2_symbol_mark_computed (attr, sym, cu, 0);
 
-  if (SYMBOL_COMPUTED_OPS (sym)->location_has_loclist)
+  if (sym->computed_ops ()->location_has_loclist)
     cu->has_loclist = true;
 }
 
diff --git a/gdb/eval.c b/gdb/eval.c
index 495effe2d039..1529063b7a99 100644
--- a/gdb/eval.c
+++ b/gdb/eval.c
@@ -1070,13 +1070,14 @@  eval_op_var_entry_value (struct type *expect_type, struct expression *exp,
   if (noside == EVAL_AVOID_SIDE_EFFECTS)
     return value::zero (sym->type (), not_lval);
 
-  if (SYMBOL_COMPUTED_OPS (sym) == NULL
-      || SYMBOL_COMPUTED_OPS (sym)->read_variable_at_entry == NULL)
+  const symbol_computed_ops *computed_ops = sym->computed_ops ();
+  if (computed_ops == nullptr
+      || computed_ops->read_variable_at_entry == nullptr)
     error (_("Symbol \"%s\" does not have any specific entry value"),
 	   sym->print_name ());
 
   frame_info_ptr frame = get_selected_frame (NULL);
-  return SYMBOL_COMPUTED_OPS (sym)->read_variable_at_entry (sym, frame);
+  return computed_ops->read_variable_at_entry (sym, frame);
 }
 
 /* Helper function that implements the body of OP_VAR_MSYM_VALUE.  */
diff --git a/gdb/findvar.c b/gdb/findvar.c
index 37b859c2347d..2fcfccda1c7a 100644
--- a/gdb/findvar.c
+++ b/gdb/findvar.c
@@ -318,8 +318,9 @@  address_to_signed_pointer (struct gdbarch *gdbarch, struct type *type,
 enum symbol_needs_kind
 symbol_read_needs (struct symbol *sym)
 {
-  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
-    return SYMBOL_COMPUTED_OPS (sym)->get_symbol_read_needs (sym);
+  if (const symbol_computed_ops *computed_ops = sym->computed_ops ();
+      computed_ops != nullptr)
+    return computed_ops->get_symbol_read_needs (sym);
 
   switch (sym->aclass ())
     {
@@ -498,8 +499,8 @@  language_defn::read_var_value (struct symbol *var,
   if (frame != NULL)
     frame = get_hosting_frame (var, var_block, frame);
 
-  if (SYMBOL_COMPUTED_OPS (var) != NULL)
-    return SYMBOL_COMPUTED_OPS (var)->read_variable (var, frame);
+  if (const symbol_computed_ops *computed_ops = var->computed_ops ())
+    return computed_ops->read_variable (var, frame);
 
   switch (var->aclass ())
     {
@@ -621,8 +622,8 @@  language_defn::read_var_value (struct symbol *var,
     case LOC_REGISTER:
     case LOC_REGPARM_ADDR:
       {
-	int regno = SYMBOL_REGISTER_OPS (var)
-		      ->register_number (var, get_frame_arch (frame));
+	const symbol_register_ops *reg_ops = var->register_ops ();
+	int regno = reg_ops->register_number (var, get_frame_arch (frame));
 
 	if (var->aclass () == LOC_REGPARM_ADDR)
 	  addr = value_as_address
diff --git a/gdb/frame.c b/gdb/frame.c
index d95d63eb0f60..29cda7da7495 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -3146,12 +3146,15 @@  frame_follow_static_link (frame_info_ptr frame)
 
       /* If we don't know how to compute FRAME's base address, don't give up:
 	 maybe the frame we are looking for is upper in the stack frame.  */
-      if (framefunc != NULL
-	  && SYMBOL_BLOCK_OPS (framefunc) != NULL
-	  && SYMBOL_BLOCK_OPS (framefunc)->get_frame_base != NULL
-	  && (SYMBOL_BLOCK_OPS (framefunc)->get_frame_base (framefunc, frame)
-	      == upper_frame_base))
-	break;
+      if (framefunc != nullptr)
+	{
+	  if (const symbol_block_ops *block_ops = framefunc->block_ops ();
+	      (block_ops != nullptr
+	       && block_ops->get_frame_base != nullptr
+	       && (block_ops->get_frame_base (framefunc, frame)
+		   == upper_frame_base)))
+	    break;
+	}
     }
 
   return frame;
diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 7a638ab8b894..f4c79ed6d247 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1643,10 +1643,10 @@  info_address_command (const char *exp, int from_tty)
     section = NULL;
   gdbarch = sym->arch ();
 
-  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
+  if (const symbol_computed_ops *computed_ops = sym->computed_ops ();
+      computed_ops != nullptr)
     {
-      SYMBOL_COMPUTED_OPS (sym)->describe_location (sym, context_pc,
-						    gdb_stdout);
+      computed_ops->describe_location (sym, context_pc, gdb_stdout);
       gdb_printf (".\n");
       return;
     }
@@ -1684,7 +1684,7 @@  info_address_command (const char *exp, int from_tty)
 	 architecture at this point.  We assume the objfile architecture
 	 will contain all the standard registers that occur in debug info
 	 in that objfile.  */
-      regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym, gdbarch);
+      regno = sym->register_ops ()->register_number (sym, gdbarch);
 
       if (sym->is_argument ())
 	gdb_printf (_("an argument in register %s"),
@@ -1712,7 +1712,7 @@  info_address_command (const char *exp, int from_tty)
 
     case LOC_REGPARM_ADDR:
       /* Note comment at LOC_REGISTER.  */
-      regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym, gdbarch);
+      regno = sym->register_ops ()->register_number (sym, gdbarch);
       gdb_printf (_("address of an argument in register %s"),
 		  gdbarch_register_name (gdbarch, regno));
       break;
diff --git a/gdb/stack.c b/gdb/stack.c
index 96a9cd480ac5..31b89342bd18 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -547,18 +547,16 @@  read_frame_arg (const frame_print_options &fp_opts,
 	}
     }
 
-  if (SYMBOL_COMPUTED_OPS (sym) != NULL
-      && SYMBOL_COMPUTED_OPS (sym)->read_variable_at_entry != NULL
-      && fp_opts.print_entry_values != print_entry_values_no
-      && (fp_opts.print_entry_values != print_entry_values_if_needed
-	  || !val || val->optimized_out ()))
+  if (const symbol_computed_ops *computed_ops = sym->computed_ops ();
+      (computed_ops != nullptr
+       && computed_ops->read_variable_at_entry != nullptr
+       && fp_opts.print_entry_values != print_entry_values_no
+       && (fp_opts.print_entry_values != print_entry_values_if_needed || !val
+	   || val->optimized_out ())))
     {
       try
 	{
-	  const struct symbol_computed_ops *ops;
-
-	  ops = SYMBOL_COMPUTED_OPS (sym);
-	  entryval = ops->read_variable_at_entry (sym, frame);
+	  entryval = computed_ops->read_variable_at_entry (sym, frame);
 	}
       catch (const gdb_exception_error &except)
 	{
diff --git a/gdb/symtab.h b/gdb/symtab.h
index eecd999b7e6f..729c003f1c10 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1267,6 +1267,21 @@  struct symbol : public general_symbol_info, public allocate_on_obstack
     return symbol_impls[this->m_aclass_index];
   }
 
+  const symbol_block_ops *block_ops () const
+  {
+    return this->impl ().ops_block;
+  }
+
+  const symbol_computed_ops *computed_ops () const
+  {
+    return this->impl ().ops_computed;
+  }
+
+  const symbol_register_ops *register_ops () const
+  {
+    return this->impl ().ops_register;
+  }
+
   address_class aclass () const
   {
     return this->impl ().aclass;
@@ -1543,17 +1558,15 @@  struct block_symbol
 /* Note: There is no accessor macro for symbol.owner because it is
    "private".  */
 
-#define SYMBOL_COMPUTED_OPS(symbol)	((symbol)->impl ().ops_computed)
-#define SYMBOL_BLOCK_OPS(symbol)	((symbol)->impl ().ops_block)
-#define SYMBOL_REGISTER_OPS(symbol)	((symbol)->impl ().ops_register)
 #define SYMBOL_LOCATION_BATON(symbol)   (symbol)->aux_value
 
 inline const block *
 symbol::value_block () const
 {
-  if (SYMBOL_BLOCK_OPS (this) != nullptr
-      && SYMBOL_BLOCK_OPS (this)->get_block_value != nullptr)
-    return SYMBOL_BLOCK_OPS (this)->get_block_value (this);
+  if (const symbol_block_ops *block_ops = this->block_ops ();
+      block_ops != nullptr && block_ops->get_block_value != nullptr)
+    return block_ops->get_block_value (this);
+
   return m_value.block;
 }
 
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 8f04f1a95018..abe8d2b45f44 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -943,7 +943,7 @@  collection_list::collect_symbol (struct symbol *sym,
 	add_memrange (gdbarch, memrange_absolute, offset, len, scope);
       break;
     case LOC_REGISTER:
-      reg = SYMBOL_REGISTER_OPS (sym)->register_number (sym, gdbarch);
+      reg = sym->register_ops ()->register_number (sym, gdbarch);
       if (info_verbose)
 	gdb_printf ("LOC_REG[parm] %s: ", sym->print_name ());
       add_local_register (gdbarch, reg, scope);
@@ -2502,10 +2502,10 @@  info_scope_command (const char *args_in, int from_tty)
 
 	  gdb_printf ("Symbol %s is ", symname);
 
-	  if (SYMBOL_COMPUTED_OPS (sym) != NULL)
-	    SYMBOL_COMPUTED_OPS (sym)->describe_location (sym,
-							  block->entry_pc (),
-							  gdb_stdout);
+	  if (const symbol_computed_ops *computed_ops = sym->computed_ops ();
+	      computed_ops != nullptr)
+	    computed_ops->describe_location (sym, block->entry_pc (),
+					     gdb_stdout);
 	  else
 	    {
 	      switch (sym->aclass ())
@@ -2539,8 +2539,7 @@  info_scope_command (const char *args_in, int from_tty)
 		     We assume the objfile architecture will contain all the
 		     standard registers that occur in debug info in that
 		     objfile.  */
-		  regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
-								      gdbarch);
+		  regno = sym->register_ops ()->register_number (sym, gdbarch);
 
 		  if (sym->is_argument ())
 		    gdb_printf ("an argument in register $%s",
@@ -2563,8 +2562,7 @@  info_scope_command (const char *args_in, int from_tty)
 		  break;
 		case LOC_REGPARM_ADDR:
 		  /* Note comment at LOC_REGISTER.  */
-		  regno = SYMBOL_REGISTER_OPS (sym)->register_number (sym,
-								      gdbarch);
+		  regno = sym->register_ops ()->register_number (sym, gdbarch);
 		  gdb_printf ("the address of an argument, in register $%s",
 			      gdbarch_register_name (gdbarch, regno));
 		  break;