* [PATCH] Use function view when iterating over block symbols @ 2022-03-04 23:40 Tom Tromey 2022-03-06 16:27 ` Simon Marchi 2022-03-15 14:54 ` Simon Marchi 0 siblings, 2 replies; 5+ messages in thread From: Tom Tromey @ 2022-03-04 23:40 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use function view when iterating over block symbols 2022-03-04 23:40 [PATCH] Use function view when iterating over block symbols Tom Tromey @ 2022-03-06 16:27 ` Simon Marchi 2022-03-15 14:54 ` Simon Marchi 1 sibling, 0 replies; 5+ messages in thread From: Simon Marchi @ 2022-03-06 16:27 UTC (permalink / raw) To: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use function view when iterating over block symbols 2022-03-04 23:40 [PATCH] Use function view when iterating over block symbols Tom Tromey 2022-03-06 16:27 ` Simon Marchi @ 2022-03-15 14:54 ` Simon Marchi 2022-03-15 22:17 ` Tom Tromey 2022-03-15 22:17 ` Tom Tromey 1 sibling, 2 replies; 5+ messages in thread From: Simon Marchi @ 2022-03-15 14:54 UTC (permalink / raw) To: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use function view when iterating over block symbols 2022-03-15 14:54 ` Simon Marchi @ 2022-03-15 22:17 ` Tom Tromey 2022-03-15 22:17 ` Tom Tromey 1 sibling, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-03-15 22:17 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use function view when iterating over block symbols 2022-03-15 14:54 ` Simon Marchi 2022-03-15 22:17 ` Tom Tromey @ 2022-03-15 22:17 ` Tom Tromey 1 sibling, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-03-15 22:17 UTC (permalink / raw) To: Simon Marchi; +Cc: Tom Tromey, gdb-patches ... 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.")); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-03-15 22:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-04 23:40 [PATCH] Use function view when iterating over block symbols Tom Tromey 2022-03-06 16:27 ` Simon Marchi 2022-03-15 14:54 ` Simon Marchi 2022-03-15 22:17 ` Tom Tromey 2022-03-15 22:17 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).