Re: [4/4] RFC: implement catch load and catch unload

[Pedro Alves <palves at redhat dot com>]

On 01/19/2012 08:35 PM, Tom Tromey wrote:
> This patch actually implements catch load and catch unload.  It also
> changes 'set stop-on-solib-events' to print information about what
> libraries were loaded or unloaded.
> 
> This patch has a few oddities.  I would appreciate comment on them.
> 
> * If you have a catchpoint and also set stop-on-solib-events, the latter
>   will always take precedence.  This is unfortunate but I think it is an
>   artifact of the bp_shlib_event breakpoint coming first in the
>   breakpoint list.
> 
>   I don't think this is a very big deal.  I'd like to consider
>   stop-on-solib-events as being essentially deprecated by the new
>   commands.

I think that's fine.

> * There is a change in bpstat_stop_status that is somewhat hacky.  We
>   have to do a pass here to handle an solib event, because we need to
>   load libraries at a point (1) after the catchpoint's breakpoint_hit
>   method is invoked, but (2) before the catchpoint's check_status method
>   is invoked.  This is needed so that we can properly collect the names
>   of added and removed solibs.

I'm okay with this.

> gdb
> 	PR symtab/12406:
> 	* solib.c (update_solib_list): Update the program space's
> 	added_solibs and deleted_solibs fields.
> 	* progspace.h (struct program_space) <added_solibs,
> 	deleted_solibs>: New fields.
> 	(clear_program_space_solib_cache): Declare.
> 	* progspace.c (release_program_space): Call
> 	clear_program_space_solib_cache.
> 	(clear_program_space_solib_cache): New function.
> 	* infrun.c (handle_inferior_event) <TARGET_WAITKIND_LOADED>: Call
> 	bpstat_stop_status.  Use handle_solib_event.
> 	* breakpoint.c: Include gdb_regex.h.
> 	(print_solib_event): New function.
> 	(bpstat_print): Use print_solib_event.
> 	(bpstat_stop_status): Add special case for bp_shlib_event.
> 	(handle_solib_event): New function.
> 	(bpstat_what): Use handle_solib_event.
> 	(struct solib_catchpoint): New.
> 	(dtor_catch_solib, insert_catch_solib, remove_catch_solib)
> 	(breakpoint_hit_catch_solib, check_status_catch_solib)
> 	(print_it_catch_solib, print_one_catch_solib)
> 	(print_mention_catch_solib, print_recreate_catch_solib): New
> 	functions.
> 	(catch_solib_breakpoint_ops): New global.
> 	(catch_load_or_unload, catch_load_command_1)
> 	(catch_unload_command_1): New functions.
> 	(internal_bkpt_check_status): Add special case for
> 	bp_shlib_event.
> 	(internal_bkpt_print_it): Use print_solib_event.
> 	(initialize_breakpoint_ops): Initialize
> 	catch_solib_breakpoint_ops.
> 	(_initialize_breakpoint): Register "catch load" and "catch
> 	unload".
> 	* breakpoint.h (handle_solib_event): Declare.
> 	* NEWS: Add entry for "catch load" and "catch unload".
> gdb/doc
> 	* gdb.texinfo (Set Catchpoints): Document "catch load" and "catch
> 	unload".
> 	(Files): Mention new catch commands.
> 	(GDB/MI Async Records): Likewise.
> gdb/testsuite
> 	* lib/mi-support.exp (mi_expect_stop): Add special case for
> 	solib-event.
> 	* gdb.base/catch-load-so.c: New file.
> 	* gdb.base/catch-load.exp: New file.
> 	* gdb.base/catch-load.c: New file.
> 	* gdb.base/break-interp.exp (reach_1): Update regexp.
> ---
>  gdb/NEWS                                |    5 +
>  gdb/breakpoint.c                        |  426 ++++++++++++++++++++++++++++--
>  gdb/breakpoint.h                        |    2 +
>  gdb/doc/gdb.texinfo                     |   21 ++-
>  gdb/infrun.c                            |   41 ++--
>  gdb/progspace.c                         |   17 ++
>  gdb/progspace.h                         |   17 ++
>  gdb/solib.c                             |    4 +
>  gdb/testsuite/gdb.base/break-interp.exp |    2 +-
>  gdb/testsuite/gdb.base/catch-load-so.c  |   22 ++
>  gdb/testsuite/gdb.base/catch-load.c     |   35 +++
>  gdb/testsuite/gdb.base/catch-load.exp   |  120 +++++++++
>  gdb/testsuite/lib/mi-support.exp        |    2 +
>  13 files changed, 660 insertions(+), 54 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/catch-load-so.c
>  create mode 100644 gdb/testsuite/gdb.base/catch-load.c
>  create mode 100644 gdb/testsuite/gdb.base/catch-load.exp
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index eaf0b9d..6d8f8f7 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -28,6 +28,11 @@
>    now set a breakpoint in build/gcc/expr.c, but not
>    build/libcpp/expr.c.
>  
> +* New commands
> +
> +  ** "catch load" and "catch unload" can be used to stop when a shared
> +     library is loaded or unloaded, respectively.
> +
>  *** Changes in GDB 7.4
>  
>  * GDB now handles ambiguous linespecs more consistently; the existing
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 1b43b43..f2710ee 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -65,6 +65,7 @@
>  #include "stack.h"
>  #include "skip.h"
>  #include "record.h"
> +#include "gdb_regex.h"
>  
>  /* readline include files */
>  #include "readline/readline.h"
> @@ -259,6 +260,11 @@ static int is_masked_watchpoint (const struct breakpoint *b);
>  
>  static int strace_marker_p (struct breakpoint *b);
>  
> +static void init_catchpoint (struct breakpoint *b,
> +			     struct gdbarch *gdbarch, int tempflag,
> +			     char *cond_string,
> +			     const struct breakpoint_ops *ops);
> +
>  /* The abstract base class all breakpoint_ops structures inherit
>     from.  */
>  static struct breakpoint_ops base_breakpoint_ops;
> @@ -3489,6 +3495,82 @@ print_bp_stop_message (bpstat bs)
>      }
>  }
>  
> +/* A helper function that prints a shared library stopped event.  */
> +
> +static void
> +print_solib_event (int is_catchpoint)
> +{
> +  int any_deleted
> +    = !VEC_empty (char_ptr, current_program_space->deleted_solibs);
> +  int any_added
> +    = !VEC_empty (so_list_ptr, current_program_space->added_solibs);
> +
> +  if (!is_catchpoint)
> +    {
> +      if (any_added || any_deleted)
> +	ui_out_text (current_uiout,
> +		     _("Stopped due to shared library event:\n"));
> +      else
> +	ui_out_text (current_uiout,
> +		     _("Stopped due to spurious shared library event (no "
> +		       "libraries added or removed)\n"));

I know Eclipse used to rely "set stop-on-solib-events 1".  Not sure they
still do.  So if it doesn't convert to the MI events, it'll break.  Maybe
just one more reason to make sure they do adjust.  :-)

> +    }
> +
> +  if (ui_out_is_mi_like_p (current_uiout))
> +    {
> +      ui_out_field_string (current_uiout, "reason",
> +			   async_reason_lookup (EXEC_ASYNC_SOLIB_EVENT));
> +      if (!any_added && !any_deleted)
> +	ui_out_field_int (current_uiout, "spurious", 1);

Do we need "spurious"?  We get the same info from neither "removed" nor "added"
being present.  I'm not super fond of using the word "spurious" because the stop
had some reason, and in my mind, something spurious is something that should
not have happened.  But in this case, the stop means something, but we're not
interpreting it.  Maybe for "catch ...", we shouldn't report a stop in
the "spurious" case?

>  
> -  if (shlib_event)

The shlib_event local should be removed then.

> -    {
> -      if (debug_infrun)
> -	fprintf_unfiltered (gdb_stdlog, "bpstat_what: bp_shlib_event\n");
> -
> -      /* Check for any newly added shared libraries if we're supposed
> -	 to be adding them automatically.  */
> -
> -      /* Switch terminal for any messages produced by
> -	 breakpoint_re_set.  */
> -      target_terminal_ours_for_output ();
> -
> -#ifdef SOLIB_ADD
> -      SOLIB_ADD (NULL, 0, &current_target, auto_solib_add);
> -#else
> -      solib_add (NULL, 0, &current_target, auto_solib_add);
> -#endif
> -
> -      target_terminal_inferior ();
> -    }
> -



> +struct solib_catchpoint

Missing introduction comment.

> +{
> +  /* The base class.  */
> +  struct breakpoint base;
> +
> +  /* True for "catch load".  */
> +  unsigned char is_load;

It helps the reader to mention catch unload explicitly:

  /* True for "catch load", false for "catch unload"  */

> +
> +  /* Regular expression to match, if any.  */
> +  char *regex;
> +  regex_t compiled;
> +};
> +

> +static int
> +breakpoint_hit_catch_solib (const struct bp_location *bl,
> +			    struct address_space *aspace,
> +			    CORE_ADDR bp_addr,
> +			    const struct target_waitstatus *ws)
> +{
> +  struct solib_catchpoint *self = (struct solib_catchpoint *) bl->owner;
> +  struct breakpoint *other;
> +
> +  if (ws->kind == TARGET_WAITKIND_LOADED)
> +    return 1;
> +
> +  ALL_BREAKPOINTS (other)
> +  {
> +    struct bp_location *other_bl;
> +
> +    if (other == bl->owner)
> +      continue;
> +
> +    if (other->type != bp_shlib_event)
> +      continue;
> +
> +    if (self->base.pspace != NULL && other->pspace != self->base.pspace)
> +      continue;

So a consequence of this is that "catch load" is only active for the
inferior was current when the catchpoint was created, right?  Was that the
intention?  If we already had itsets, we could make it trigger on all
inferiors by default, and then use itsets to filter.

> +
> +    for (other_bl = other->loc; other_bl != NULL; other_bl = other_bl->next)
> +      {
> +	if (other->ops->breakpoint_hit (other_bl, aspace, bp_addr, ws))
> +	  return 1;
> +      }
> +  }
> +
> +  return 0;
> +}
> +


> +/* A helper function that does all the work for "catch load" and
> +   "catch unload".  */
> +
> +static void
> +catch_load_or_unload (char *arg, int from_tty, int is_load,
> +		      struct cmd_list_element *command)
> +{
> +  struct solib_catchpoint *c;
> +  struct gdbarch *gdbarch = get_current_arch ();
> +  int tempflag;
> +  regex_t compiled;
> +  struct cleanup *cleanup;
> +
> +  tempflag = get_cmd_context (command) == CATCH_TEMPORARY;
> +
> +  if (!arg)
> +    arg = "";
> +  arg = skip_spaces (arg);
> +
> +  c = XCNEW (struct solib_catchpoint);
> +  cleanup = make_cleanup (xfree, c);
> +
> +  if (*arg != '\0')
> +    {
> +      int errcode;
> +
> +      errcode = regcomp (&c->compiled, arg, REG_NOSUB);
> +      if (errcode != 0)
> +	{
> +	  char *err = get_regcomp_error (errcode, &c->compiled);
> +
> +	  make_cleanup (xfree, err);
> +	  error (_("Invalid regexp (%s): %s"), err, arg);
> +	}
> +      c->regex = xstrdup (arg);
> +    }
> +
> +  c->is_load = is_load;
> +  init_catchpoint (&c->base, gdbarch, tempflag, NULL,
> +		   &catch_solib_breakpoint_ops);
> +
> +  install_breakpoint (0, &c->base, 1);
> +  discard_cleanups (cleanup);

It's more correct to discard the cleanups before installing the breakpoint.
The first thing install_breakpoint does it put the breakpoint in the
breakpoint chain.  Once that is done, we shouldn't delete it with the
cleanup anymore.



> +
> +# Run one set of tests.
> +# SCENARIO is the name of the test scenario, it is just used in test
> +# names.
> +# KIND is is passed to the "catch" command.

Double is.

> +# MATCH is a boolean saying whether we expect the catchpoint to be hit.
> +proc one_catch_load_test {scenario kind match sostop} {
> +    global verbose testfile testfile2 binfile2_dlopen
> +    global pf_prefix srcfile
> +    global decimal gdb_prompt
> +
> +    set saved_prefix $pf_prefix
> +    append pf_prefix "${scenario}:"
> +
> +    clean_restart $testfile
> +    gdb_load_shlibs $binfile2_dlopen
> +
> +    if {![runto_main]} {
> +	fail "can't run to main"
> +	set pf_prefix $saved_prefix
> +	return
> +    }
> +
> +    gdb_breakpoint [gdb_get_line_number "final breakpoint here"]
> +    gdb_test_no_output "set var libname = \"$binfile2_dlopen\""
> +    gdb_test_no_output "set stop-on-solib-events $sostop"
> +    gdb_test "catch $kind" "Catchpoint $decimal \\(.*\\)"
> +
> +    send_gdb "continue\n"
> +    gdb_expect {
> +	-re "Catchpoint $decimal\r\n.*loaded .*/$testfile2.*\r\n.*$gdb_prompt $" {

gdb_test_multiple?


Otherwise looks good to me.  Thanks for doing this.

-- 
Pedro Alves