This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PROTOTYPE] Make "info breakpoints" show breakpoint's specs (Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler)


* Pedro Alves <palves@redhat.com> [2019-06-24 20:54:43 +0100]:

> On 6/24/19 8:16 PM, Pedro Alves wrote:
> > 
> > I've thought for a few years that "info breakpoints" should show
> > BOTH the canonical spec behind each breakpoint, and the actual
> > location(s) where the breakpoint is inserted.  It wouldn't
> > be that hard, even.  I cooked up a prototype patch for that.
> > I'll post it as a follow up.
> 
> Here's what I meant.
> 
> Here are for example 3 breakpoints that I set while debugging gdb:
> 
>  (top-gdb) info breakpoints
>  1       breakpoint     keep y   0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
>  2       breakpoint     keep y   0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
>  3       breakpoint     keep y   <MULTIPLE>         
>  3.1                         y   0x0000000000575127 in internal_error(char const*, int, char const*, ...) at src/gdb/common/errors.c:54
>  3.2                         y   0x00007ffff6d50410 in PyErr_SetObject at /usr/src/debug/python2-2.7.15-4.fc27.x86_64/Python/errors.c:54
>  (top-gdb)
> 
> From looking at those, you have no idea how I created the breakpoints in
> the first place, which specs I used.  The first two look like the same,
> but really aren't.  I don't recall which was which though.
> And what's with the third, showing seemingly unrelated functions?
> 
> GDB of course knows the breakpoints were set differently.  It needs to
> remember the spec in order to re_set the locations properly.  And it
> needs to remember the spec in order to save the breakpoints, like:
> 
>  (top-gdb) save breakpoints bpts.cmd
>  Saved to file 'bpts.cmd'.
> 
> Let's look at the file, see how the breakpoint had been created:
> 
>  (top-gdb) shell cat bpts.cmd
>  break internal_error
>  break -qualified internal_error
>  break errors.c:54
>  (top-gdb) 
> 
> Ah, see how the "-qualified" information was lost from
> "info breakpoints" output?  And how now it's obvious
> why we have two locations for breakpoint 3?
> 
> Why not show this in "info breakpoints" too?
> 
> A way to do that would be always show separate lines for
> breakpoint and locations, as if all breakpoints had multiple
> locations, even if they only have one location.  The "What"
> column for the owner breakpoint could then show the canonical
> location string:
> 
>  (top-gdb) info breakpoints 
>  Num     Type           Disp Enb Address            What
>  1       breakpoint     keep y                      internal_error
>  1.1                         y   0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
>  2       breakpoint     keep y                      -qualified internal_error
>  2.1                         y   0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
>  3       breakpoint     keep y                      errors.c:54
>  3.1                         y   0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
>  3.2                         y   0x00007ffff6d50410 in PyErr_SetObject at /usr/src/debug/python2-2.7.15-4.fc27.x86_64/Python/errors.c:54

I think this would be a good change.

Thanks,
Andrew


> 
> Breakpoints would no longer move from single-location to multiple-location
> "display modes" as location were added/removed (e.g., when a shared library
> is loaded), they'd always be in multi-location display mode.
> 
> Note we currently have a special case when we show the location of
> a breakpoint with a single location separately, it's when a breakpoint
> has a single location that is disabled.  The above gets rid of the
> special case, by always applying the special case, you could say.  :-)
> 
> Here's what it would look like in that case we're discussing,
> where the user sets a breapoint on a comment or empty line:
> 
>  (top-gdb) b 27
>  Breakpoint 4 at 0x469aa6: file /home/pedro/gdb/mygit/src/gdb/gdb.c, line 28.
>  (top-gdb) info breakpoints 4
>  Num     Type           Disp Enb Address            What
>  4       breakpoint     keep y                      /home/pedro/gdb/mygit/src/gdb/gdb.c:27
>  4.1                         y   0x0000000000469aa6 in main(int, char**) at /home/pedro/gdb/mygit/src/gdb/gdb.c:28
> 
> Note how GDB remembers line 27, but we don't currently show it, unlike
> in the snippet above with patched gdb.
> 
> If we had this, then I think it would also make sense to
> add a way to list breakpoints and skip showing the locations, for
> a more condensed, higher level view of the breakpoints.
> E.g., try "b main" while debugging gdb.  You'll end up with
> 24 locations...:
> 
> (top-gdb) b main
> Breakpoint 3 at 0x469aa6: main. (24 locations)
> (top-gdb) info breakpoints 
> Num     Type           Disp Enb Address            What
> 1       breakpoint     keep y                      internal_error
> 1.1                         y   0x00000000005755a5 in internal_error(char const*, int, char const*, ...) at /home/pedro/gdb/mygit/src/gdb/common/errors.c:54
> 2       breakpoint     keep y                      info_command
>         silent
>         return
> 2.1                         y   0x0000000000545204 in info_command(char const*, int) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:201
> 3       breakpoint     keep y                      main
> 3.1                         y   0x0000000000469aa6 in main(int, char**) at /home/pedro/gdb/mygit/src/gdb/gdb.c:28
> 3.2                         y   0x00000000009875fa in selftests::string_view::capacity_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/capacity/1.cc:167
> 3.3                         y   0x00000000009879ac in selftests::string_view::cons_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/1.cc:62
> 3.4                         y   0x0000000000987a67 in selftests::string_view::cons_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/2.cc:41
> 3.5                         y   0x0000000000987aa2 in selftests::string_view::cons_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/cons/char/3.cc:34
> 3.6                         y   0x0000000000987c2b in selftests::string_view::element_access_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/1.cc:66
> 3.7                         y   0x0000000000987c3f in selftests::string_view::element_access_empty::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/empty.cc:27
> 3.8                         y   0x0000000000987de3 in selftests::string_view::element_access_front_back::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/element_access/char/front_back.cc:38
> 3.9                         y   0x000000000098819b in selftests::string_view::inserters_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/inserters/char/2.cc:84
> 3.10                        y   0x00000000009882cf in selftests::string_view::modifiers_remove_prefix::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/modifiers/remove_prefix/char/1.cc:58
> 3.11                        y   0x00000000009883e0 in selftests::string_view::modifiers_remove_suffix::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/modifiers/remove_suffix/char/1.cc:58
> 3.12                        y   0x0000000000988b99 in selftests::string_view::operations_compare_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/compare/char/1.cc:127
> 3.13                        y   0x0000000000988cc6 in selftests::string_view::operations_compare_13650::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/compare/char/13650.cc:45
> 3.14                        y   0x0000000000988d82 in selftests::string_view::operations_copy_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/copy/char/1.cc:41
> 3.15                        y   0x0000000000988e20 in selftests::string_view::operations_data_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/data/char/1.cc:39
> 3.16                        y   0x0000000000989370 in selftests::string_view::operations_find_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/1.cc:160
> 3.17                        y   0x0000000000989861 in selftests::string_view::operations_find_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/2.cc:158
> 3.18                        y   0x0000000000989e43 in selftests::string_view::operations_find_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/3.cc:158
> 3.19                        y   0x0000000000989ebb in selftests::string_view::operations_find_4::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/find/char/4.cc:40
> 3.20                        y   0x000000000098a41b in selftests::string_view::operations_rfind_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/1.cc:90
> 3.21                        y   0x000000000098a623 in selftests::string_view::operations_rfind_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/2.cc:48
> 3.22                        y   0x000000000098a970 in selftests::string_view::operations_rfind_3::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/rfind/char/3.cc:62
> 3.23                        y   0x000000000098ac52 in selftests::string_view::operations_substr_1::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operations/substr/char/1.cc:74
> 3.24                        y   0x000000000098cd7a in selftests::string_view::operators_2::main() at /home/pedro/gdb/mygit/src/gdb/unittests/basic_string_view/operators/char/2.cc:366
> 
> With a "-hide-locations" switch, a user could do this to get the locations
> out of the way:
> 
>  (top-gdb) info breakpoints -hide-locations 
>  Num     Type           Disp Enb Address            What
>  1       breakpoint     keep y                      internal_error
>  2       breakpoint     keep y                      info_command
>          silent
>          return
>  3       breakpoint     keep y                      main
> 
> 
> (We could have switches for only showing enabled or disabled breakpoints
> too, btw.)
> 
> Below's the prototype patch.
> 
> I didn't think about what to do with watchpoints, catchpoints,
> etc.  Currently they show like an unpatched gdb:
> 
>  3       catchpoint     keep y                      syscall "<any syscall>" 
>  4       hw watchpoint  keep y                      *main
> 
> I did notice that the patch currently crashes with catchpoints that
> are implemented via breakpoints, for example, and possibly
> other types:
> 
>  (top-gdb) catch throw 
>  Catchpoint 5 (throw)
>  (top-gdb) info breakpoints 
>  5       breakpoint     keep y   ../../gdb/breakpoint.c:6387: internal-error: void print_one_breakpoint_location(breakpoint*, bp_location*, int, bp_location**, int): Assertion `b->loc == NULL || b->loc->next == NULL' failed.
>  A problem internal to GDB has been detected,
>  further debugging may prove unreliable.
>  Quit this debugging session? (y or n) 
> 
> whoops.  :-)  I did say this was a prototype, right?  :-)
> 
> From 8087acd6926d1054979ed42076a9bb0b28c4551e Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Sat, 22 Jun 2019 21:10:58 +0100
> Subject: [PATCH] bkpts
> 
> ---
>  gdb/breakpoint.c     | 114 ++++++++++++++++++++++++++++++++++++++-------------
>  gdb/cli/cli-option.c |  12 +++++-
>  gdb/cli/cli-option.h |   6 +++
>  3 files changed, 102 insertions(+), 30 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 8422db8b571..d28c4736d43 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5973,6 +5973,33 @@ output_thread_groups (struct ui_out *uiout,
>      }
>  }
>  
> +/* The options for the "info breakpoints" command.  */
> +
> +struct info_breakpoints_opts
> +{
> +  /* For "-hide-locations".  */
> +  int hide_locations = 0;
> +};
> +
> +static const gdb::option::option_def info_breakpoints_option_defs[] = {
> +
> +  gdb::option::flag_option_def<info_breakpoints_opts> {
> +    "hide-locations",
> +    [] (info_breakpoints_opts *opts) { return &opts->hide_locations; },
> +    N_("Hide breakpoint locations."),
> +  },
> +
> +};
> +
> +/* Create an option_def_group for the "info breakpoints" options, with
> +   OPTS as context.  */
> +
> +static inline gdb::option::option_def_group
> +make_info_breakpoints_options_def_group (info_breakpoints_opts *opts)
> +{
> +  return {{info_breakpoints_option_defs}, opts};
> +}
> +
>  /* Print B to gdb_stdout.  */
>  
>  static void
> @@ -5993,14 +6020,11 @@ print_one_breakpoint_location (struct breakpoint *b,
>    get_user_print_options (&opts);
>  
>    gdb_assert (!loc || loc_number != 0);
> -  /* See comment in print_one_breakpoint concerning treatment of
> -     breakpoints with single disabled location.  */
> -  if (loc == NULL 
> -      && (b->loc != NULL 
> -	  && (b->loc->next != NULL || !b->loc->enabled)))
> -    header_of_multiple = 1;
>    if (loc == NULL)
> -    loc = b->loc;
> +    {
> +      header_of_multiple = 1;
> +      loc = b->loc;
> +    }
>  
>    annotate_record ();
>  
> @@ -6098,7 +6122,7 @@ print_one_breakpoint_location (struct breakpoint *b,
>  	  {
>  	    annotate_field (4);
>  	    if (header_of_multiple)
> -	      uiout->field_string ("addr", "<MULTIPLE>");
> +	      uiout->field_skip ("addr");
>  	    else if (b->loc == NULL || loc->shlib_disabled)
>  	      uiout->field_string ("addr", "<PENDING>");
>  	    else
> @@ -6106,7 +6130,9 @@ print_one_breakpoint_location (struct breakpoint *b,
>  				      loc->gdbarch, loc->address);
>  	  }
>  	annotate_field (5);
> -	if (!header_of_multiple)
> +	if (header_of_multiple)
> +	  uiout->field_string ("what", event_location_to_string (b->location.get ()));
> +	else
>  	  print_breakpoint_location (b, loc);
>  	if (b->loc)
>  	  *last_loc = b->loc;
> @@ -6333,7 +6359,8 @@ bool fix_multi_location_breakpoint_output_globally = false;
>  static void
>  print_one_breakpoint (struct breakpoint *b,
>  		      struct bp_location **last_loc, 
> -		      int allflag)
> +		      int allflag,
> +		      const info_breakpoints_opts &ib_opts)
>  {
>    struct ui_out *uiout = current_uiout;
>    bool use_fixed_output
> @@ -6348,21 +6375,15 @@ print_one_breakpoint (struct breakpoint *b,
>    if (!use_fixed_output)
>      bkpt_tuple_emitter.reset ();
>  
> -  /* If this breakpoint has custom print function,
> -     it's already printed.  Otherwise, print individual
> -     locations, if any.  */
> -  if (b->ops == NULL || b->ops->print_one == NULL)
> +  /* If this breakpoint has a custom print function, it's already
> +     printed.  Otherwise, print individual locations, if any, and if
> +     not explicitly disabled by the user.  */
> +  if ((b->ops == NULL || b->ops->print_one == NULL)
> +      && !ib_opts.hide_locations)
>      {
> -      /* If breakpoint has a single location that is disabled, we
> -	 print it as if it had several locations, since otherwise it's
> -	 hard to represent "breakpoint enabled, location disabled"
> -	 situation.
> -
> -	 Note that while hardware watchpoints have several locations
> -	 internally, that's not a property exposed to user.  */
> -      if (b->loc 
> -	  && !is_hardware_watchpoint (b)
> -	  && (b->loc->next || !b->loc->enabled))
> +      /* While hardware watchpoints have several locations internally,
> +	 that's not a property exposed to the user.  */
> +      if (!is_hardware_watchpoint (b))
>  	{
>  	  gdb::optional<ui_out_emit_list> locations_list;
>  
> @@ -6410,8 +6431,9 @@ breakpoint_address_bits (struct breakpoint *b)
>  void
>  print_breakpoint (breakpoint *b)
>  {
> +  info_breakpoints_opts ib_opts;
>    struct bp_location *dummy_loc = NULL;
> -  print_one_breakpoint (b, &dummy_loc, 0);
> +  print_one_breakpoint (b, &dummy_loc, 0, ib_opts);
>  }
>  
>  /* Return true if this breakpoint was set by the user, false if it is
> @@ -6452,6 +6474,17 @@ breakpoint_1 (const char *args, int allflag,
>  
>    get_user_print_options (&opts);
>  
> +  info_breakpoints_opts ib_opts;
> +
> +  auto grp = make_info_breakpoints_options_def_group (&ib_opts);
> +
> +  gdb::option::process_options
> +    (&args, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp);
> +
> +  if (args != nullptr
> +      && args[0] == '-' && (!allflag || !isdigit (args[1])))
> +    gdb::option::error_unrecognized_option_at (args);
> +
>    /* Compute the number of rows in the table, as well as the size
>       required for address fields.  */
>    nr_printable_breakpoints = 0;
> @@ -6549,7 +6582,7 @@ breakpoint_1 (const char *args, int allflag,
>  	/* We only print out user settable breakpoints unless the
>  	   allflag is set.  */
>  	if (allflag || user_breakpoint_p (b))
> -	  print_one_breakpoint (b, &last_loc, allflag);
> +	  print_one_breakpoint (b, &last_loc, allflag, ib_opts);
>        }
>    }
>  
> @@ -6579,6 +6612,29 @@ breakpoint_1 (const char *args, int allflag,
>    return nr_printable_breakpoints;
>  }
>  
> +/* Completer for the "info breakpoints" command.  */
> +
> +static void
> +info_breakpoints_command_completer (struct cmd_list_element *ignore,
> +				    completion_tracker &tracker,
> +				    const char *text, const char *word_ignored)
> +{
> +  const auto grp = make_info_breakpoints_options_def_group (nullptr);
> +
> +  if (gdb::option::complete_options
> +      (tracker, &text, gdb::option::PROCESS_OPTIONS_UNKNOWN_IS_OPERAND, grp))
> +    return;
> +
> +  /* Convenience to let the user know what the command can accept.  */
> +  if (*text == '\0')
> +    {
> +      gdb::option::complete_on_all_options (tracker, grp);
> +      /* Keep this "ID" in sync with what "help info breakpoints"
> +	 says.  */
> +      tracker.add_completion (make_unique_xstrdup ("ID"));
> +    }
> +}
> +
>  /* Display the value of default-collect in a way that is generally
>     compatible with the breakpoint list.  */
>  
> @@ -15611,7 +15667,7 @@ Convenience variable \"$bpnum\" contains the number of the last\n\
>  breakpoint set."));
>      }
>  
> -  add_info ("breakpoints", info_breakpoints_command, _("\
> +  c = add_info ("breakpoints", info_breakpoints_command, _("\
>  Status of specified breakpoints (all user-settable breakpoints if no argument).\n\
>  The \"Type\" column indicates one of:\n\
>  \tbreakpoint     - normal breakpoint\n\
> @@ -15626,10 +15682,11 @@ are set to the address of the last breakpoint listed unless the command\n\
>  is prefixed with \"server \".\n\n\
>  Convenience variable \"$bpnum\" contains the number of the last\n\
>  breakpoint set."));
> +  set_cmd_completer_handle_brkchars (c, info_breakpoints_command_completer);
>  
>    add_info_alias ("b", "breakpoints", 1);
>  
> -  add_cmd ("breakpoints", class_maintenance, maintenance_info_breakpoints, _("\
> +  c = add_cmd ("breakpoints", class_maintenance, maintenance_info_breakpoints, _("\
>  Status of all breakpoints, or breakpoint number NUMBER.\n\
>  The \"Type\" column indicates one of:\n\
>  \tbreakpoint     - normal breakpoint\n\
> @@ -15649,6 +15706,7 @@ is prefixed with \"server \".\n\n\
>  Convenience variable \"$bpnum\" contains the number of the last\n\
>  breakpoint set."),
>  	   &maintenanceinfolist);
> +  set_cmd_completer_handle_brkchars (c, info_breakpoints_command_completer);
>  
>    add_prefix_cmd ("catch", class_breakpoint, catch_command, _("\
>  Set catchpoints to catch events."),
> diff --git a/gdb/cli/cli-option.c b/gdb/cli/cli-option.c
> index 9a53ec0592d..d43edb78641 100644
> --- a/gdb/cli/cli-option.c
> +++ b/gdb/cli/cli-option.c
> @@ -121,6 +121,14 @@ complete_on_all_options (completion_tracker &tracker,
>    complete_on_options (options_group, tracker, opt + 1, opt);
>  }
>  
> +/* See cli-option.h.  */
> +
> +void
> +error_unrecognized_option_at (const char *at)
> +{
> +  error (_("Unrecognized option at: %s"), at);
> +}
> +
>  /* Parse ARGS, guided by OPTIONS_GROUP.  HAVE_DELIMITER is true if the
>     whole ARGS line included the "--" options-terminator delimiter.  */
>  
> @@ -136,7 +144,7 @@ parse_option (gdb::array_view<const option_def_group> options_group,
>    else if (**args != '-')
>      {
>        if (have_delimiter)
> -	error (_("Unrecognized option at: %s"), *args);
> +	error_unrecognized_option_at (*args);
>        return {};
>      }
>    else if (check_for_argument (args, "--"))
> @@ -182,7 +190,7 @@ parse_option (gdb::array_view<const option_def_group> options_group,
>    if (match == nullptr)
>      {
>        if (have_delimiter || mode != PROCESS_OPTIONS_UNKNOWN_IS_OPERAND)
> -	error (_("Unrecognized option at: %s"), *args);
> +	error_unrecognized_option_at (*args);
>  
>        return {};
>      }
> diff --git a/gdb/cli/cli-option.h b/gdb/cli/cli-option.h
> index 1bfbfce1ce5..06f12751c36 100644
> --- a/gdb/cli/cli-option.h
> +++ b/gdb/cli/cli-option.h
> @@ -314,6 +314,12 @@ extern void
>    complete_on_all_options (completion_tracker &tracker,
>  			   gdb::array_view<const option_def_group> options_group);
>  
> +/* Throw an error indicating an unrecognized option was detected at
> +   AT.  Use this in conjunction with UNKNOWN_IS_OPERAND instead of
> +   UNKNOWN_IS_ERROR when the operand may or may not begin with '-'
> +   depending on some condition determined at run time.  */
> +extern void error_unrecognized_option_at (const char *at);
> +
>  /* Return a string with the result of replacing %OPTIONS% in HELP_TMLP
>     with an auto-generated "help" string fragment for all the options
>     in OPTIONS_GROUP.  */
> -- 
> 2.14.5
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]