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

[Simon Marchi <simon.marchi@polymtl.ca>]

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