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: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint


* Pedro Alves <palves@redhat.com> [2019-07-05 17:00:55 +0100]:

> Currently, with:
> 
>  (gdb) catch catch
>  Catchpoint 1 (catch)
>  (gdb) catch throw
>  Catchpoint 2 (throw)
>  (gdb) catch rethrow
>  Catchpoint 3 (rethrow)
> 
> You get:
> 
> (gdb) info breakpoints
>  Num     Type           Disp Enb Address            What
>  1       breakpoint     keep y   0x0000000000b122af exception catch
>  2       breakpoint     keep y   0x0000000000b1288d exception throw
>  3       breakpoint     keep y   0x0000000000b12931 exception rethrow
> 
> I think it doesn't make much sense usability-wise, to show a
> catchpoint as a breakpoint.  The fact that GDB sets a breakpoint at
> some magic address in the C++ run time is an implementation detail,
> IMO.  And as seen in the previous patch, such a catchpoint can end up
> with more than one location/address even, so showing a single address
> isn't entirely accurate.
> 
> This commit hides the addresses from view, and makes GDB show
> "catchpoint" for type as well:
> 
>   (gdb) info breakpoints
>   Num     Type           Disp Enb Address            What
>   1       catchpoint     keep y                      exception catch
>   2       catchpoint     keep y                      exception throw
>   3       catchpoint     keep y                      exception rethrow
>

I agree for users knowing the address is probably not helpful, but we
also have 'maintenance info breakpoints'.  I think it might (one day)
be helpful if we _did_ print the address in the maintenance version of
the command.

thanks,
Andrew



> This comment in the code seems telling:
> 
>   /* We need to reset 'type' in order for code in breakpoint.c to do
>      the right thing.  */
>   cp->type = bp_breakpoint;
> 
> It kind of suggests that the reason catchpoints end up shown as
> breakpoints was that it was easier to implement them that way, rather
> than a desired property.
> 
> This commit fixes things up to make it possible to have bp_catch
> breakpoints have software/hardware breakpoint locations, thus
> eliminating the need for that hack:
> 
>  - redo breakpoint_address_is_meaningful in terms of the location's
>    type rather than breakpoint type.
>  - teach bpstat_what about stepping over the catchpoint locations.
>  - install a allocate_location method for "catch catch/throw/rethrow",
>    one that forces the location type.
> 
> Note that this also reverts the gdb hunk from:
> 
>   commit 2a8be20359dba9cc684fd3ffa222d985399f3b18
>   Commit:     Tom Tromey <tom@tromey.com>
>   CommitDate: Sat Oct 6 22:17:45 2018 -0600
> 
>       Fix Python gdb.Breakpoint.location crash
> 
> because now "catch throw" catchpoints hit the
> 
>    if (obj->bp->type != bp_breakpoint)
>      Py_RETURN_NONE;
> 
> check above, and, adjusts the testcase to no longer expect to see the
> catchpoint in the gdb.breakpoints() list.
> 
> (Note: might make sense to do the same to Ada exception catchpoints.)
> 
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* break-catch-throw.c (print_one_exception_catchpoint): Skip the
> 	"addr" field.
> 	(allocate_location_exception_catchpoint): New.
> 	(handle_gnu_v3_exceptions): Don't reset 'type' to bp_breakpoint.
> 	(initialize_throw_catchpoint_ops): Install
> 	allocate_location_exception_catchpoint as allocate_location
> 	method.
> 	* breakpoint.c (bpstat_what) <bp_catch>: Set action to
> 	BPSTAT_WHAT_SINGLE if not stopping and the location's type is not
> 	bp_loc_other.
> 	(breakpoint_address_is_meaningful): Delete.
> 	(bl_address_is_meaningful): New.
> 	(breakpoint_locations_match): Adjust comment.
> 	(bp_location_from_bp_type): New, factored out of...
> 	(bp_location::bp_location(breakpoint *)): ... this.
> 	(bp_location::bp_location(breakpoint *, bp_loc_type)): New,
> 	factored out of...
> 	(bp_location::bp_location(breakpoint *)): ... this.  Reimplement.
> 	(bp_loc_is_permanent): Use bl_address_is_meaningful instead of
> 	breakpoint_address_is_meaningful.
> 	(bp_locations_compare): Adjust comment.
> 	(update_global_location_list): Use bl_address_is_meaningful
> 	instead of breakpoint_address_is_meaningful.
> 	* breakpoint.h (bp_location::bp_location(breakpoint *)): New
> 	explicit.
> 	(bp_location::bp_location(breakpoint *, bp_loc_type)): Declare.
> 	* python/py-breakpoint.c (bppy_get_location): No longer check
> 	whether location is null.
> 
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <palves@redhat.com>
> 
> 	* gdb.cp/catch-multi-stdlib.exp: Adjust expected "info
> 	breakpoints" output.
> 	* gdb.cp/exception.exp: Adjust expected "info breakpoints" output.
> 	* gdb.python/py-breakpoint.exp: No longer expect that "catch
> 	throw" creates breakpoint.
> ---
>  gdb/break-catch-throw.c                        |  25 +++---
>  gdb/breakpoint.c                               | 108 +++++++++++++------------
>  gdb/breakpoint.h                               |   7 +-
>  gdb/python/py-breakpoint.c                     |   8 +-
>  gdb/testsuite/gdb.cp/except-multi-location.exp |   6 +-
>  gdb/testsuite/gdb.cp/exception.exp             |  18 +----
>  gdb/testsuite/gdb.python/py-breakpoint.exp     |   5 +-
>  7 files changed, 84 insertions(+), 93 deletions(-)
> 
> diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
> index 107ac74f5cc..c0b3eec63d1 100644
> --- a/gdb/break-catch-throw.c
> +++ b/gdb/break-catch-throw.c
> @@ -249,18 +249,10 @@ print_one_exception_catchpoint (struct breakpoint *b,
>    enum exception_event_kind kind = classify_exception_breakpoint (b);
>  
>    get_user_print_options (&opts);
> +
>    if (opts.addressprint)
> -    {
> -      annotate_field (4);
> -      if (b->loc == NULL || b->loc->shlib_disabled)
> -	uiout->field_string ("addr", "<PENDING>");
> -      else
> -	uiout->field_core_addr ("addr",
> -				b->loc->gdbarch, b->loc->address);
> -    }
> +    uiout->field_skip ("addr");
>    annotate_field (5);
> -  if (b->loc)
> -    *last_loc = b->loc;
>  
>    switch (kind)
>      {
> @@ -344,6 +336,15 @@ print_recreate_exception_catchpoint (struct breakpoint *b,
>    print_recreate_thread (b, fp);
>  }
>  
> +/* Implement the "allocate_location" breakpoint_ops method for throw
> +   and catch catchpoints.  */
> +
> +static bp_location *
> +allocate_location_exception_catchpoint (breakpoint *self)
> +{
> +  return new bp_location (self, bp_loc_software_breakpoint);
> +}
> +
>  static void
>  handle_gnu_v3_exceptions (int tempflag, std::string &&except_rx,
>  			  const char *cond_string,
> @@ -361,9 +362,6 @@ handle_gnu_v3_exceptions (int tempflag, std::string &&except_rx,
>  
>    init_catchpoint (cp.get (), get_current_arch (), tempflag, cond_string,
>  		   &gnu_v3_exception_catchpoint_ops);
> -  /* We need to reset 'type' in order for code in breakpoint.c to do
> -     the right thing.  */
> -  cp->type = bp_breakpoint;
>    cp->kind = ex_event;
>    cp->exception_rx = std::move (except_rx);
>    cp->pattern = std::move (pattern);
> @@ -521,6 +519,7 @@ initialize_throw_catchpoint_ops (void)
>    ops->print_recreate = print_recreate_exception_catchpoint;
>    ops->print_one_detail = print_one_detail_exception_catchpoint;
>    ops->check_status = check_status_exception_catchpoint;
> +  ops->allocate_location = allocate_location_exception_catchpoint;
>  }
>  
>  void
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index d508921fdc5..36500758257 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5603,8 +5603,10 @@ bpstat_what (bpstat bs_head)
>  	    }
>  	  else
>  	    {
> -	      /* There was a catchpoint, but we're not stopping.
> -		 This requires no further action.  */
> +	      /* Some catchpoints are implemented with breakpoints.
> +		 For those, we need to step over the breakpoint.  */
> +	      if (bs->bp_location_at->loc_type != bp_loc_other)
> +		this_action = BPSTAT_WHAT_SINGLE;
>  	    }
>  	  break;
>  	case bp_jit_event:
> @@ -6686,27 +6688,21 @@ describe_other_breakpoints (struct gdbarch *gdbarch,
>  }
>  
>  
> -/* Return true iff it is meaningful to use the address member of
> -   BPT locations.  For some breakpoint types, the locations' address members
> -   are irrelevant and it makes no sense to attempt to compare them to other
> -   addresses (or use them for any other purpose either).
> -
> -   More specifically, each of the following breakpoint types will
> -   always have a zero valued location address and we don't want to mark
> -   breakpoints of any of these types to be a duplicate of an actual
> -   breakpoint location at address zero:
> -
> -      bp_watchpoint
> -      bp_catchpoint
> +/* Return true iff it is meaningful to use the address member of LOC.
> +   For some breakpoint types, the locations' address members are
> +   irrelevant and it makes no sense to attempt to compare them to
> +   other addresses (or use them for any other purpose either).
>  
> -*/
> +   More specifically, software watchpoints and catchpoints that are
> +   not backed by breakpoints always have a zero valued location
> +   address and we don't want to mark breakpoints of any of these types
> +   to be a duplicate of an actual breakpoint location at address
> +   zero.  */
>  
> -static int
> -breakpoint_address_is_meaningful (struct breakpoint *bpt)
> +static bool
> +bl_address_is_meaningful (bp_location *loc)
>  {
> -  enum bptype type = bpt->type;
> -
> -  return (type != bp_watchpoint && type != bp_catchpoint);
> +  return loc->loc_type != bp_loc_other;
>  }
>  
>  /* Assuming LOC1 and LOC2's owners are hardware watchpoints, returns
> @@ -6838,8 +6834,8 @@ tracepoint_locations_match (struct bp_location *loc1,
>  }
>  
>  /* Assuming LOC1 and LOC2's types' have meaningful target addresses
> -   (breakpoint_address_is_meaningful), returns true if LOC1 and LOC2
> -   represent the same location.  */
> +   (bl_address_is_meaningful), returns true if LOC1 and LOC2 represent
> +   the same location.  */
>  
>  static int
>  breakpoint_locations_match (struct bp_location *loc1, 
> @@ -6937,16 +6933,10 @@ adjust_breakpoint_address (struct gdbarch *gdbarch,
>      }
>  }
>  
> -bp_location::bp_location (breakpoint *owner)
> +static bp_loc_type
> +bp_location_from_bp_type (bptype type)
>  {
> -  bp_location *loc = this;
> -
> -  loc->owner = owner;
> -  loc->cond_bytecode = NULL;
> -  loc->shlib_disabled = 0;
> -  loc->enabled = 1;
> -
> -  switch (owner->type)
> +  switch (type)
>      {
>      case bp_breakpoint:
>      case bp_single_step:
> @@ -6972,30 +6962,44 @@ bp_location::bp_location (breakpoint *owner)
>      case bp_gnu_ifunc_resolver:
>      case bp_gnu_ifunc_resolver_return:
>      case bp_dprintf:
> -      loc->loc_type = bp_loc_software_breakpoint;
> -      mark_breakpoint_location_modified (loc);
> -      break;
> +      return bp_loc_software_breakpoint;
>      case bp_hardware_breakpoint:
> -      loc->loc_type = bp_loc_hardware_breakpoint;
> -      mark_breakpoint_location_modified (loc);
> -      break;
> +      return bp_loc_hardware_breakpoint;
>      case bp_hardware_watchpoint:
>      case bp_read_watchpoint:
>      case bp_access_watchpoint:
> -      loc->loc_type = bp_loc_hardware_watchpoint;
> -      break;
> +      return bp_loc_hardware_watchpoint;
>      case bp_watchpoint:
>      case bp_catchpoint:
>      case bp_tracepoint:
>      case bp_fast_tracepoint:
>      case bp_static_tracepoint:
> -      loc->loc_type = bp_loc_other;
> -      break;
> +      return bp_loc_other;
>      default:
>        internal_error (__FILE__, __LINE__, _("unknown breakpoint type"));
>      }
> +}
> +
> +bp_location::bp_location (breakpoint *owner, bp_loc_type type)
> +{
> +  this->owner = owner;
> +  this->cond_bytecode = NULL;
> +  this->shlib_disabled = 0;
> +  this->enabled = 1;
> +
> +  this->loc_type = type;
>  
> -  loc->refc = 1;
> +  if (this->loc_type == bp_loc_software_breakpoint
> +      || this->loc_type == bp_loc_hardware_breakpoint)
> +    mark_breakpoint_location_modified (this);
> +
> +  this->refc = 1;
> +}
> +
> +bp_location::bp_location (breakpoint *owner)
> +  : bp_location::bp_location (owner,
> +			      bp_location_from_bp_type (owner->type))
> +{
>  }
>  
>  /* Allocate a struct bp_location.  */
> @@ -8643,11 +8647,12 @@ bp_loc_is_permanent (struct bp_location *loc)
>  {
>    gdb_assert (loc != NULL);
>  
> -  /* If we have a catchpoint or a watchpoint, just return 0.  We should not
> -     attempt to read from the addresses the locations of these breakpoint types
> -     point to.  program_breakpoint_here_p, below, will attempt to read
> +  /* If we have a non-breakpoint-backed catchpoint or a software
> +     watchpoint, just return 0.  We should not attempt to read from
> +     the addresses the locations of these breakpoint types point to.
> +     program_breakpoint_here_p, below, will attempt to read
>       memory.  */
> -  if (!breakpoint_address_is_meaningful (loc->owner))
> +  if (!bl_address_is_meaningful (loc))
>      return 0;
>  
>    scoped_restore_current_pspace_and_thread restore_pspace_thread;
> @@ -11455,10 +11460,9 @@ breakpoint_auto_delete (bpstat bs)
>  
>  /* A comparison function for bp_location AP and BP being interfaced to
>     qsort.  Sort elements primarily by their ADDRESS (no matter what
> -   does breakpoint_address_is_meaningful say for its OWNER),
> -   secondarily by ordering first permanent elements and
> -   terciarily just ensuring the array is sorted stable way despite
> -   qsort being an unstable algorithm.  */
> +   bl_address_is_meaningful says), secondarily by ordering first
> +   permanent elements and terciarily just ensuring the array is sorted
> +   stable way despite qsort being an unstable algorithm.  */
>  
>  static int
>  bp_locations_compare (const void *ap, const void *bp)
> @@ -11798,7 +11802,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  		 this one from the target.  */
>  
>  	      /* OLD_LOC comes from existing struct breakpoint.  */
> -	      if (breakpoint_address_is_meaningful (old_loc->owner))
> +	      if (bl_address_is_meaningful (old_loc))
>  		{
>  		  for (loc2p = locp;
>  		       (loc2p < bp_locations + bp_locations_count
> @@ -11938,7 +11942,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>        b = loc->owner;
>  
>        if (!unduplicated_should_be_inserted (loc)
> -	  || !breakpoint_address_is_meaningful (b)
> +	  || !bl_address_is_meaningful (loc)
>  	  /* Don't detect duplicate for tracepoint locations because they are
>  	   never duplicated.  See the comments in field `duplicate' of
>  	   `struct bp_location'.  */
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 75f19785cb1..6e9a35e82de 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -316,7 +316,12 @@ class bp_location
>  public:
>    bp_location () = default;
>  
> -  bp_location (breakpoint *owner);
> +  /* Construct a bp_location with the type inferred from OWNER's
> +     type.  */
> +  explicit bp_location (breakpoint *owner);
> +
> +  /* Construct a bp_location with type TYPE.  */
> +  bp_location (breakpoint *owner, bp_loc_type type);
>  
>    virtual ~bp_location ();
>  
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 88cd7de3343..698d91e9d32 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -379,7 +379,6 @@ bppy_set_hit_count (PyObject *self, PyObject *newvalue, void *closure)
>  static PyObject *
>  bppy_get_location (PyObject *self, void *closure)
>  {
> -  const char *str;
>    gdbpy_breakpoint_object *obj = (gdbpy_breakpoint_object *) self;
>  
>    BPPY_REQUIRE_VALID (obj);
> @@ -387,12 +386,7 @@ bppy_get_location (PyObject *self, void *closure)
>    if (obj->bp->type != bp_breakpoint)
>      Py_RETURN_NONE;
>  
> -  struct event_location *location = obj->bp->location.get ();
> -  /* "catch throw" makes a breakpoint of type bp_breakpoint that does
> -     not have a location.  */
> -  if (location == nullptr)
> -    Py_RETURN_NONE;
> -  str = event_location_to_string (location);
> +  const char *str = event_location_to_string (obj->bp->location.get ());
>    if (! str)
>      str = "";
>    return host_string_to_python_string (str).release ();
> diff --git a/gdb/testsuite/gdb.cp/except-multi-location.exp b/gdb/testsuite/gdb.cp/except-multi-location.exp
> index 60d6d0b9351..1258d893afb 100644
> --- a/gdb/testsuite/gdb.cp/except-multi-location.exp
> +++ b/gdb/testsuite/gdb.cp/except-multi-location.exp
> @@ -74,9 +74,9 @@ proc test_multi_libstdcpp {static_bin static_lib} {
>      set ws "\[ \t\]*"
>      gdb_test "info breakpoints" \
>  	[multi_line \
> -	     "${decimal}${ws}breakpoint${ws}keep${ws}y${ws}${hex}${ws}exception catch" \
> -	     "${decimal}${ws}breakpoint${ws}keep${ws}y${ws}${hex}${ws}exception throw" \
> -	     "${decimal}${ws}breakpoint${ws}keep${ws}y${ws}${hex}${ws}exception rethrow"]
> +	     "${decimal}${ws}catchpoint${ws}keep${ws}y${ws}exception catch" \
> +	     "${decimal}${ws}catchpoint${ws}keep${ws}y${ws}exception throw" \
> +	     "${decimal}${ws}catchpoint${ws}keep${ws}y${ws}exception rethrow"]
>  }
>  
>  # Try different static/not-static combinations.
> diff --git a/gdb/testsuite/gdb.cp/exception.exp b/gdb/testsuite/gdb.cp/exception.exp
> index 4761de20e19..0be382ba98c 100644
> --- a/gdb/testsuite/gdb.cp/exception.exp
> +++ b/gdb/testsuite/gdb.cp/exception.exp
> @@ -62,16 +62,10 @@ gdb_test "catch rethrow" "Catchpoint \[0-9\]+ \\(rethrow\\)" \
>      "catch rethrow (before inferior run)"
>  
>  
> -# The catchpoints should be listed in the list of breakpoints.
> -# In case of a statically linked test, we won't have a pending breakpoint.
> -# Hence we allow for both an address or "<PENDING>". If we ever become able
> -# to tell whether the target is linked statically or not, we can be more
> -# precise and require exact output.
> -set addr "\(<PENDING>|$hex\)"
>  set re_head	"Num${ws}Type${ws}Disp${ws}Enb${ws}Address${ws}What"
> -set re_2_bp	"1${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception catch"
> -set re_3_bp	"2${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception throw"
> -set re_4_bp   "3${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception rethrow"
> +set re_2_bp	"1${ws}catchpoint${ws}keep${ws}y${ws}exception catch"
> +set re_3_bp	"2${ws}catchpoint${ws}keep${ws}y${ws}exception throw"
> +set re_4_bp	"3${ws}catchpoint${ws}keep${ws}y${ws}exception rethrow"
>  
>  set name "info breakpoints (before inferior run)"
>  gdb_test_multiple "info breakpoints" $name {
> @@ -100,12 +94,6 @@ if { !$ok } {
>      continue
>  }
>  
> -set addr "$hex"
> -set re_head	"Num${ws}Type${ws}Disp${ws}Enb${ws}Address${ws}What"
> -set re_2_bp	"1${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception catch"
> -set re_3_bp	"2${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception throw"
> -set re_4_bp   "3${ws}breakpoint${ws}keep${ws}y${ws}$addr${ws}exception rethrow"
> -
>  set name "info breakpoints (after inferior run)"
>  gdb_test_multiple "info breakpoints" $name {
>      -re "$re_head${ws}$re_2_bp${ws}$re_3_bp${ws}$re_4_bp\r\n$gdb_prompt $" {
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index 241e7e6839e..625977c0ad4 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -619,8 +619,9 @@ proc_with_prefix test_bkpt_explicit_loc {} {
>  
>      delete_breakpoints
>      gdb_test "catch throw" "Catchpoint .* \\(throw\\)"
> -    gdb_test "python print (gdb.breakpoints()\[0\].location)" None \
> -	"Examine location of catchpoint"
> +    gdb_test "python print (gdb.breakpoints())" \
> +	"\(\)" \
> +	"catch throw is not a breakpoint"
>  }
>  
>  proc_with_prefix test_bkpt_qualified {} {
> -- 
> 2.14.5
> 


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