[PATCH] Use function view when iterating over block symbols

[PATCH] Use function view when iterating over block symbols

[Tom Tromey]

This changes iterate_over_block_local_vars and
iterate_over_block_arg_vars to take a gdb::function_view rather than a
function pointer and a user-data.  In one spot, this allows us to
remove a helper structure and helper function.  In another spot, this
looked more complicated, so I changed the helper function to be an
"operator()" -- also a simplification, just not as big.
---
 gdb/stack.c      | 47 ++++++++++++++++++-----------------------
 gdb/stack.h      | 11 ++++------
 gdb/tracepoint.c | 55 ++++++++++++------------------------------------
 3 files changed, 37 insertions(+), 76 deletions(-)

diff --git a/gdb/stack.c b/gdb/stack.c
index fe243b4310e..d373b9fa93d 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2240,8 +2240,7 @@ backtrace_command_completer (struct cmd_list_element *ignore,
 
 static void
 iterate_over_block_locals (const struct block *b,
-			   iterate_over_block_arg_local_vars_cb cb,
-			   void *cb_data)
+			   iterate_over_block_arg_local_vars_cb cb)
 {
   struct block_iterator iter;
   struct symbol *sym;
@@ -2260,7 +2259,7 @@ iterate_over_block_locals (const struct block *b,
 	    break;
 	  if (sym->domain () == COMMON_BLOCK_DOMAIN)
 	    break;
-	  (*cb) (sym->print_name (), sym, cb_data);
+	  cb (sym->print_name (), sym);
 	  break;
 
 	default:
@@ -2275,12 +2274,11 @@ iterate_over_block_locals (const struct block *b,
 
 void
 iterate_over_block_local_vars (const struct block *block,
-			       iterate_over_block_arg_local_vars_cb cb,
-			       void *cb_data)
+			       iterate_over_block_arg_local_vars_cb cb)
 {
   while (block)
     {
-      iterate_over_block_locals (block, cb, cb_data);
+      iterate_over_block_locals (block, cb);
       /* After handling the function's top-level block, stop.  Don't
 	 continue to its superblock, the block of per-file
 	 symbols.  */
@@ -2301,41 +2299,40 @@ struct print_variable_and_value_data
   int num_tabs;
   struct ui_file *stream;
   int values_printed;
+
+  void operator() (const char *print_name, struct symbol *sym);
 };
 
 /* The callback for the locals and args iterators.  */
 
-static void
-do_print_variable_and_value (const char *print_name,
-			     struct symbol *sym,
-			     void *cb_data)
+void
+print_variable_and_value_data::operator() (const char *print_name,
+					   struct symbol *sym)
 {
-  struct print_variable_and_value_data *p
-    = (struct print_variable_and_value_data *) cb_data;
   struct frame_info *frame;
 
-  if (p->preg.has_value ()
-      && p->preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
+  if (preg.has_value ()
+      && preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
     return;
-  if (p->treg.has_value ()
-      && !treg_matches_sym_type_name (*p->treg, sym))
+  if (treg.has_value ()
+      && !treg_matches_sym_type_name (*treg, sym))
     return;
   if (language_def (sym->language ())->symbol_printing_suppressed (sym))
     return;
 
-  frame = frame_find_by_id (p->frame_id);
+  frame = frame_find_by_id (frame_id);
   if (frame == NULL)
     {
       warning (_("Unable to restore previously selected frame."));
       return;
     }
 
-  print_variable_and_value (print_name, sym, frame, p->stream, p->num_tabs);
+  print_variable_and_value (print_name, sym, frame, stream, num_tabs);
 
   /* print_variable_and_value invalidates FRAME.  */
   frame = NULL;
 
-  p->values_printed = 1;
+  values_printed = 1;
 }
 
 /* Prepares the regular expression REG from REGEXP.
@@ -2404,9 +2401,7 @@ print_frame_local_vars (struct frame_info *frame,
   scoped_restore_selected_frame restore_selected_frame;
   select_frame (frame);
 
-  iterate_over_block_local_vars (block,
-				 do_print_variable_and_value,
-				 &cb_data);
+  iterate_over_block_local_vars (block, cb_data);
 
   if (!cb_data.values_printed && !quiet)
     {
@@ -2494,8 +2489,7 @@ info_locals_command (const char *args, int from_tty)
 
 void
 iterate_over_block_arg_vars (const struct block *b,
-			     iterate_over_block_arg_local_vars_cb cb,
-			     void *cb_data)
+			     iterate_over_block_arg_local_vars_cb cb)
 {
   struct block_iterator iter;
   struct symbol *sym, *sym2;
@@ -2518,7 +2512,7 @@ iterate_over_block_arg_vars (const struct block *b,
 
 	  sym2 = lookup_symbol_search_name (sym->search_name (),
 					    b, VAR_DOMAIN).symbol;
-	  (*cb) (sym->print_name (), sym2, cb_data);
+	  cb (sym->print_name (), sym2);
 	}
     }
 }
@@ -2569,8 +2563,7 @@ print_frame_arg_vars (struct frame_info *frame,
   cb_data.stream = stream;
   cb_data.values_printed = 0;
 
-  iterate_over_block_arg_vars (SYMBOL_BLOCK_VALUE (func),
-			       do_print_variable_and_value, &cb_data);
+  iterate_over_block_arg_vars (SYMBOL_BLOCK_VALUE (func), cb_data);
 
   /* do_print_variable_and_value invalidates FRAME.  */
   frame = NULL;
diff --git a/gdb/stack.h b/gdb/stack.h
index 97cf30cb677..c49d2e2cbef 100644
--- a/gdb/stack.h
+++ b/gdb/stack.h
@@ -30,17 +30,14 @@ gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
 						  enum language *funlang,
 						  struct symbol **funcp);
 
-typedef void (*iterate_over_block_arg_local_vars_cb) (const char *print_name,
-						      struct symbol *sym,
-						      void *cb_data);
+typedef gdb::function_view<void (const char *print_name, struct symbol *sym)>
+     iterate_over_block_arg_local_vars_cb;
 
 void iterate_over_block_arg_vars (const struct block *block,
-				  iterate_over_block_arg_local_vars_cb cb,
-				  void *cb_data);
+				  iterate_over_block_arg_local_vars_cb cb);
 
 void iterate_over_block_local_vars (const struct block *block,
-				    iterate_over_block_arg_local_vars_cb cb,
-				    void *cb_data);
+				    iterate_over_block_arg_local_vars_cb cb);
 
 /* Initialize *WHAT to be a copy of the user desired print what frame info.
    If !WHAT.has_value (), the printing function chooses a default set of
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 73ebc38eb80..7b2244d8a54 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1041,36 +1041,6 @@ collection_list::collect_symbol (struct symbol *sym,
     }
 }
 
-/* Data to be passed around in the calls to the locals and args
-   iterators.  */
-
-struct add_local_symbols_data
-{
-  struct collection_list *collect;
-  struct gdbarch *gdbarch;
-  CORE_ADDR pc;
-  long frame_regno;
-  long frame_offset;
-  int count;
-  int trace_string;
-};
-
-/* The callback for the locals and args iterators.  */
-
-static void
-do_collect_symbol (const char *print_name,
-		   struct symbol *sym,
-		   void *cb_data)
-{
-  struct add_local_symbols_data *p = (struct add_local_symbols_data *) cb_data;
-
-  p->collect->collect_symbol (sym, p->gdbarch, p->frame_regno,
-			      p->frame_offset, p->pc, p->trace_string);
-  p->count++;
-
-  p->collect->add_wholly_collected (print_name);
-}
-
 void
 collection_list::add_wholly_collected (const char *print_name)
 {
@@ -1085,15 +1055,16 @@ collection_list::add_local_symbols (struct gdbarch *gdbarch, CORE_ADDR pc,
 				    int trace_string)
 {
   const struct block *block;
-  struct add_local_symbols_data cb_data;
+  int count = 0;
 
-  cb_data.collect = this;
-  cb_data.gdbarch = gdbarch;
-  cb_data.pc = pc;
-  cb_data.frame_regno = frame_regno;
-  cb_data.frame_offset = frame_offset;
-  cb_data.count = 0;
-  cb_data.trace_string = trace_string;
+  auto do_collect_symbol = [&] (const char *print_name,
+				struct symbol *sym)
+    {
+      collect_symbol (sym, gdbarch, frame_regno,
+		      frame_offset, pc, trace_string);
+      count++;
+      add_wholly_collected (print_name);
+    };
 
   if (type == 'L')
     {
@@ -1105,8 +1076,8 @@ collection_list::add_local_symbols (struct gdbarch *gdbarch, CORE_ADDR pc,
 	  return;
 	}
 
-      iterate_over_block_local_vars (block, do_collect_symbol, &cb_data);
-      if (cb_data.count == 0)
+      iterate_over_block_local_vars (block, do_collect_symbol);
+      if (count == 0)
 	warning (_("No locals found in scope."));
     }
   else
@@ -1119,8 +1090,8 @@ collection_list::add_local_symbols (struct gdbarch *gdbarch, CORE_ADDR pc,
 	  return;
 	}
 
-      iterate_over_block_arg_vars (block, do_collect_symbol, &cb_data);
-      if (cb_data.count == 0)
+      iterate_over_block_arg_vars (block, do_collect_symbol);
+      if (count == 0)
 	warning (_("No args found in scope."));
     }
 }
-- 
2.34.1

Re: [PATCH] Use function view when iterating over block symbols

[Simon Marchi]

On 2022-03-04 18:40, Tom Tromey wrote:
> This changes iterate_over_block_local_vars and
> iterate_over_block_arg_vars to take a gdb::function_view rather than a
> function pointer and a user-data.  In one spot, this allows us to
> remove a helper structure and helper function.  In another spot, this
> looked more complicated, so I changed the helper function to be an
> "operator()" -- also a simplification, just not as big.
> ---
>  gdb/stack.c      | 47 ++++++++++++++++++-----------------------
>  gdb/stack.h      | 11 ++++------
>  gdb/tracepoint.c | 55 ++++++++++++------------------------------------
>  3 files changed, 37 insertions(+), 76 deletions(-)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index fe243b4310e..d373b9fa93d 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2240,8 +2240,7 @@ backtrace_command_completer (struct cmd_list_element *ignore,
>  
>  static void
>  iterate_over_block_locals (const struct block *b,

The comment above this needs to be updated.

Otherwise, LGTM.

Simon

Re: [PATCH] Use function view when iterating over block symbols

[Simon Marchi]

On 2022-03-04 18:40, Tom Tromey wrote:
> This changes iterate_over_block_local_vars and
> iterate_over_block_arg_vars to take a gdb::function_view rather than a
> function pointer and a user-data.  In one spot, this allows us to
> remove a helper structure and helper function.  In another spot, this
> looked more complicated, so I changed the helper function to be an
> "operator()" -- also a simplification, just not as big.
> ---
>  gdb/stack.c      | 47 ++++++++++++++++++-----------------------
>  gdb/stack.h      | 11 ++++------
>  gdb/tracepoint.c | 55 ++++++++++++------------------------------------
>  3 files changed, 37 insertions(+), 76 deletions(-)
> 
> diff --git a/gdb/stack.c b/gdb/stack.c
> index fe243b4310e..d373b9fa93d 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2240,8 +2240,7 @@ backtrace_command_completer (struct cmd_list_element *ignore,
>  
>  static void
>  iterate_over_block_locals (const struct block *b,
> -			   iterate_over_block_arg_local_vars_cb cb,
> -			   void *cb_data)
> +			   iterate_over_block_arg_local_vars_cb cb)
>  {
>    struct block_iterator iter;
>    struct symbol *sym;
> @@ -2260,7 +2259,7 @@ iterate_over_block_locals (const struct block *b,
>  	    break;
>  	  if (sym->domain () == COMMON_BLOCK_DOMAIN)
>  	    break;
> -	  (*cb) (sym->print_name (), sym, cb_data);
> +	  cb (sym->print_name (), sym);
>  	  break;
>  
>  	default:
> @@ -2275,12 +2274,11 @@ iterate_over_block_locals (const struct block *b,
>  
>  void
>  iterate_over_block_local_vars (const struct block *block,
> -			       iterate_over_block_arg_local_vars_cb cb,
> -			       void *cb_data)
> +			       iterate_over_block_arg_local_vars_cb cb)
>  {
>    while (block)
>      {
> -      iterate_over_block_locals (block, cb, cb_data);
> +      iterate_over_block_locals (block, cb);
>        /* After handling the function's top-level block, stop.  Don't
>  	 continue to its superblock, the block of per-file
>  	 symbols.  */
> @@ -2301,41 +2299,40 @@ struct print_variable_and_value_data
>    int num_tabs;
>    struct ui_file *stream;
>    int values_printed;
> +
> +  void operator() (const char *print_name, struct symbol *sym);
>  };
>  
>  /* The callback for the locals and args iterators.  */
>  
> -static void
> -do_print_variable_and_value (const char *print_name,
> -			     struct symbol *sym,
> -			     void *cb_data)
> +void
> +print_variable_and_value_data::operator() (const char *print_name,
> +					   struct symbol *sym)
>  {
> -  struct print_variable_and_value_data *p
> -    = (struct print_variable_and_value_data *) cb_data;
>    struct frame_info *frame;
>  
> -  if (p->preg.has_value ()
> -      && p->preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
> +  if (preg.has_value ()
> +      && preg->exec (sym->natural_name (), 0, NULL, 0) != 0)
>      return;
> -  if (p->treg.has_value ()
> -      && !treg_matches_sym_type_name (*p->treg, sym))
> +  if (treg.has_value ()
> +      && !treg_matches_sym_type_name (*treg, sym))
>      return;
>    if (language_def (sym->language ())->symbol_printing_suppressed (sym))
>      return;
>  
> -  frame = frame_find_by_id (p->frame_id);
> +  frame = frame_find_by_id (frame_id);
>    if (frame == NULL)
>      {
>        warning (_("Unable to restore previously selected frame."));
>        return;
>      }
>  
> -  print_variable_and_value (print_name, sym, frame, p->stream, p->num_tabs);
> +  print_variable_and_value (print_name, sym, frame, stream, num_tabs);
>  
>    /* print_variable_and_value invalidates FRAME.  */
>    frame = NULL;
>  
> -  p->values_printed = 1;
> +  values_printed = 1;
>  }
>  
>  /* Prepares the regular expression REG from REGEXP.
> @@ -2404,9 +2401,7 @@ print_frame_local_vars (struct frame_info *frame,
>    scoped_restore_selected_frame restore_selected_frame;
>    select_frame (frame);
>  
> -  iterate_over_block_local_vars (block,
> -				 do_print_variable_and_value,
> -				 &cb_data);
> +  iterate_over_block_local_vars (block, cb_data);
>  
>    if (!cb_data.values_printed && !quiet)
>      {
> @@ -2494,8 +2489,7 @@ info_locals_command (const char *args, int from_tty)
>  
>  void
>  iterate_over_block_arg_vars (const struct block *b,
> -			     iterate_over_block_arg_local_vars_cb cb,
> -			     void *cb_data)
> +			     iterate_over_block_arg_local_vars_cb cb)
>  {
>    struct block_iterator iter;
>    struct symbol *sym, *sym2;
> @@ -2518,7 +2512,7 @@ iterate_over_block_arg_vars (const struct block *b,
>  
>  	  sym2 = lookup_symbol_search_name (sym->search_name (),
>  					    b, VAR_DOMAIN).symbol;
> -	  (*cb) (sym->print_name (), sym2, cb_data);
> +	  cb (sym->print_name (), sym2);
>  	}
>      }
>  }
> @@ -2569,8 +2563,7 @@ print_frame_arg_vars (struct frame_info *frame,
>    cb_data.stream = stream;
>    cb_data.values_printed = 0;
>  
> -  iterate_over_block_arg_vars (SYMBOL_BLOCK_VALUE (func),
> -			       do_print_variable_and_value, &cb_data);
> +  iterate_over_block_arg_vars (SYMBOL_BLOCK_VALUE (func), cb_data);
>  
>    /* do_print_variable_and_value invalidates FRAME.  */
>    frame = NULL;
> diff --git a/gdb/stack.h b/gdb/stack.h
> index 97cf30cb677..c49d2e2cbef 100644
> --- a/gdb/stack.h
> +++ b/gdb/stack.h
> @@ -30,17 +30,14 @@ gdb::unique_xmalloc_ptr<char> find_frame_funname (struct frame_info *frame,
>  						  enum language *funlang,
>  						  struct symbol **funcp);
>  
> -typedef void (*iterate_over_block_arg_local_vars_cb) (const char *print_name,
> -						      struct symbol *sym,
> -						      void *cb_data);
> +typedef gdb::function_view<void (const char *print_name, struct symbol *sym)>
> +     iterate_over_block_arg_local_vars_cb;
>  
>  void iterate_over_block_arg_vars (const struct block *block,
> -				  iterate_over_block_arg_local_vars_cb cb,
> -				  void *cb_data);
> +				  iterate_over_block_arg_local_vars_cb cb);
>  
>  void iterate_over_block_local_vars (const struct block *block,
> -				    iterate_over_block_arg_local_vars_cb cb,
> -				    void *cb_data);
> +				    iterate_over_block_arg_local_vars_cb cb);
>  
>  /* Initialize *WHAT to be a copy of the user desired print what frame info.
>     If !WHAT.has_value (), the printing function chooses a default set of
> diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
> index 73ebc38eb80..7b2244d8a54 100644
> --- a/gdb/tracepoint.c
> +++ b/gdb/tracepoint.c
> @@ -1041,36 +1041,6 @@ collection_list::collect_symbol (struct symbol *sym,
>      }
>  }
>  
> -/* Data to be passed around in the calls to the locals and args
> -   iterators.  */
> -
> -struct add_local_symbols_data
> -{
> -  struct collection_list *collect;
> -  struct gdbarch *gdbarch;
> -  CORE_ADDR pc;
> -  long frame_regno;
> -  long frame_offset;
> -  int count;
> -  int trace_string;
> -};
> -
> -/* The callback for the locals and args iterators.  */
> -
> -static void
> -do_collect_symbol (const char *print_name,
> -		   struct symbol *sym,
> -		   void *cb_data)
> -{
> -  struct add_local_symbols_data *p = (struct add_local_symbols_data *) cb_data;
> -
> -  p->collect->collect_symbol (sym, p->gdbarch, p->frame_regno,
> -			      p->frame_offset, p->pc, p->trace_string);
> -  p->count++;
> -
> -  p->collect->add_wholly_collected (print_name);
> -}
> -
>  void
>  collection_list::add_wholly_collected (const char *print_name)
>  {
> @@ -1085,15 +1055,16 @@ collection_list::add_local_symbols (struct gdbarch *gdbarch, CORE_ADDR pc,
>  				    int trace_string)
>  {
>    const struct block *block;
> -  struct add_local_symbols_data cb_data;
> +  int count = 0;
>  
> -  cb_data.collect = this;
> -  cb_data.gdbarch = gdbarch;
> -  cb_data.pc = pc;
> -  cb_data.frame_regno = frame_regno;
> -  cb_data.frame_offset = frame_offset;
> -  cb_data.count = 0;
> -  cb_data.trace_string = trace_string;
> +  auto do_collect_symbol = [&] (const char *print_name,
> +				struct symbol *sym)
> +    {
> +      collect_symbol (sym, gdbarch, frame_regno,
> +		      frame_offset, pc, trace_string);
> +      count++;
> +      add_wholly_collected (print_name);
> +    };

I see some tests failing in gdb.trace since this patch.  I can't really
explain it, but I tracked it down to this capture by reference.

Amongst others:

    $ make check TESTS="gdb.trace/backtrace.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
    FAIL: gdb.trace/backtrace.exp: 8.6: Backtrace, depth == 1, collect args and locals
    FAIL: gdb.trace/backtrace.exp: 8.6: Backtrace, depth == 2, collect args and locals
    FAIL: gdb.trace/backtrace.exp: 8.6: Backtrace, depth == 3, collect args and locals
    FAIL: gdb.trace/backtrace.exp: 8.6: Backtrace, depth == 4, collect args and locals

If I capture all variables by value (except count, which needs to be by
reference since it's updated in the lambda):

     auto do_collect_symbol = [gdbarch, frame_regno, frame_offset, pc, trace_string, this, &count]
         (const char *print_name, struct symbol *sym)

The test passes.  If I capture `pc` by reference, the test fails:

     auto do_collect_symbol = [gdbarch, frame_regno, frame_offset, &pc, trace_string, this, &count]
         (const char *print_name, struct symbol *sym)

I don't understand why it changes anything,  since `pc` is only passed
to the collect_symbol method by value, so the fact that it's captured by
reference or by value shouldn't change anything, at least I don't see
how.

Simon

Re: [PATCH] Use function view when iterating over block symbols

[Tom Tromey]

Simon> I see some tests failing in gdb.trace since this patch.

Sorry about that.

Simon> I can't really
Simon> explain it, but I tracked it down to this capture by reference.

'pc' is assigned to later in the function.  The new closure uses the
updated value, but the old code did not.

I'm checking in the appended.  I verified that the test failed before
the patch and passes after.  I didn't see this in the original patch
because I didn't run gdbserver tests.

Tom

Re: [PATCH] Use function view when iterating over block symbols

[Tom Tromey]

... forgot to actually append the patch.

Tom

commit 8f50b4b574b9f34c4f23ce6d6508f72e9c2f5a59
Author: Tom Tromey <tom@tromey.com>
Date:   Tue Mar 15 16:13:57 2022 -0600

    Do not capture updated 'pc' in add_local_symbols
    
    Simon pointed out that commit 13835d88 ("Use function view when
    iterating over block symbols") caused a regression.  The bug is that
    the new closure captures 'pc' by reference, but later code updates
    this variable -- but the earlier code did not update the callback
    structure with the new value.
    
    This patch restores the old behavior by using a new varible name in an
    inner scope.

diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 67b31182880..c45a9611abd 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -1082,8 +1082,8 @@ collection_list::add_local_symbols (struct gdbarch *gdbarch, CORE_ADDR pc,
     }
   else
     {
-      pc = get_pc_function_start (pc);
-      block = block_for_pc (pc);
+      CORE_ADDR fn_pc = get_pc_function_start (pc);
+      block = block_for_pc (fn_pc);
       if (block == NULL)
 	{
 	  warning (_("Can't collect args; no symbol table info available."));