[PATCH 0/2] Fix "info break" crash + make "catch catch" show as catchpoint

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Fix "info break" crash + make "catch catch" show as catchpoint

Pedro Alves-7
This series fixes the issues I first discovered at:

  https://sourceware.org/ml/gdb-patches/2019-07/msg00078.html

The first patch fixes the crash.  It turns out that the "-qualified" /
wildmatching stuff was not really to blame.

The second patch changes the way "info breakpoints" shows these C++
exception catchpoints.  Currently we show them as breakpoints, after
the patch, we'll show them as catchpoints.

Pedro Alves (2):
  Fix "info break" + "catch catch" + -static-{libstdc++,libgcc}
  "catch catch/throw/rethrow", breakpoint -> catchpoint

 gdb/break-catch-throw.c                            |  25 +++--
 gdb/breakpoint.c                                   | 116 ++++++++++-----------
 gdb/breakpoint.h                                   |   7 +-
 gdb/python/py-breakpoint.c                         |   8 +-
 gdb/testsuite/gdb.cp/except-multi-location-lib.cc  |  25 +++++
 gdb/testsuite/gdb.cp/except-multi-location-main.cc |  36 +++++++
 gdb/testsuite/gdb.cp/except-multi-location.exp     |  87 ++++++++++++++++
 gdb/testsuite/gdb.cp/exception.exp                 |  18 +---
 gdb/testsuite/gdb.python/py-breakpoint.exp         |   5 +-
 9 files changed, 230 insertions(+), 97 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location-lib.cc
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location-main.cc
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location.exp

--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Fix "info break" + "catch catch" + -static-{libstdc++,libgcc}

Pedro Alves-7
If you debug current GDB, set a "catch catch/throw/rethrow"
catchpoint, and then do "info breakpoints", the top GDB hits an
internal error:

 (top-gdb) catch catch
 Catchpoint 1 (catch)
 (top-gdb) info breakpoints
 Num     Type           Disp Enb Address            What
 1       breakpoint     keep y   src/gdb/breakpoint.c:6040: 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)

The assertion in question is asserting that a breakpoint with a
print_one method only has one location, and it fails because this
catchpoint ends up with two locations.

Internally, "catch catch" sets a breakpoint at __cxa_begin_catch.  If
we do that manually, we see the locations:

  (top-gdb) b -qualified __cxa_begin_catch
  Breakpoint 2 at 0xb122b0 (2 locations)
  (top-gdb) info breakpoints
  Num     Type           Disp Enb Address            What
  2       breakpoint     keep y   <MULTIPLE>
  2.1                         y   0x0000000000b122b0 <__cxa_begin_catch>
  2.2                         y   0x00007ffff2f4ddb0 in __cxxabiv1::__cxa_begin_catch(void*) at ../../../../libstdc++-v3/libsupc++/eh_catch.cc:41

Note that I had used -qualified.  It seems strange that we get a
location for a namespaced symbol, but that happens because the minimal
symbol for that address is indeed called __cxa_begin_catch.

The real issue is that gdb is linked with
-static-libgcc/-static-libstdc++.  And then, it _also_ ends up with
shared libstc++ loaded:

  (top-gdb) info sharedlibrary stdc++
  From                To                  Syms Read   Shared Object Library
  0x00007ffff2f4b380  0x00007ffff2ffc018  Yes         /lib64/libstdc++.so.6

Location 2.2 is set within libstdc++.so.6's range:

  (top-gdb) p 0x00007ffff2f4b380 <= 0x00007ffff2f4ddb0 && 0x00007ffff2f4ddb0 < 0x00007ffff2ffc018
  $1 = true

So due to -static-lib*, we end up with _two_ copies of the
__cxa_begin_catch code:

  (top-gdb) disassemble 0x0000000000b122b0
  Dump of assembler code for function __cxa_begin_catch:
     0x0000000000b122b0 <+0>:     push   %rbx
     0x0000000000b122b1 <+1>:     mov    %rdi,%rbx
     0x0000000000b122b4 <+4>:     callq  0xb11a80 <__cxa_get_globals>
     0x0000000000b122b9 <+9>:     movabs $0xb8b1aabcbcd4d500,%rdx
  ...

  (top-gdb) disassemble 0x00007ffff2f4ddb0
  Dump of assembler code for function __cxxabiv1::__cxa_begin_catch(void*):
     0x00007ffff2f4ddb0 <+0>:     push   %rbx
     0x00007ffff2f4ddb1 <+1>:     mov    %rdi,%rbx
     0x00007ffff2f4ddb4 <+4>:     callq  0x7ffff2f4a090 <__cxa_get_globals@plt>
     0x00007ffff2f4ddb9 <+9>:     movabs $0xb8b1aabcbcd4d500,%rdx
  ...

I think we end up with libstdc++.so.6 loaded because
libsource-highlight.so depends on it.

Irrespective of whether it's a good idea to use
-static-libgcc/-static-libstdc++, GDB should not crash.  Since there
are two copies of the code, it seems right to have more than one
location.  So the fix is just to remove the assertion.

A testcase is included, which mimics the scenerio described above,
with binary linked with -static-lib{stdc++,gcc} and a shared library
that is linked normally, along with other combinations for good
measure.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <[hidden email]>

        * breakpoint.c (print_one_breakpoint_location): Remove
        single-location assert.

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <[hidden email]>

        * gdb.cp/except-multi-location-lib.cc: New.
        * gdb.cp/except-multi-location-main.cc: New.
        * gdb.cp/except-multi-location.exp: New.
---
 gdb/breakpoint.c                                   |  8 +-
 gdb/testsuite/gdb.cp/except-multi-location-lib.cc  | 25 +++++++
 gdb/testsuite/gdb.cp/except-multi-location-main.cc | 36 +++++++++
 gdb/testsuite/gdb.cp/except-multi-location.exp     | 87 ++++++++++++++++++++++
 4 files changed, 149 insertions(+), 7 deletions(-)
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location-lib.cc
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location-main.cc
 create mode 100644 gdb/testsuite/gdb.cp/except-multi-location.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index b49be762a2f..d508921fdc5 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6033,13 +6033,7 @@ print_one_breakpoint_location (struct breakpoint *b,
 
   /* 5 and 6 */
   if (b->ops != NULL && b->ops->print_one != NULL)
-    {
-      /* Although the print_one can possibly print all locations,
- calling it here is not likely to get any nice result.  So,
- make sure there's just one location.  */
-      gdb_assert (b->loc == NULL || b->loc->next == NULL);
-      b->ops->print_one (b, last_loc);
-    }
+    b->ops->print_one (b, last_loc);
   else
     switch (b->type)
       {
diff --git a/gdb/testsuite/gdb.cp/except-multi-location-lib.cc b/gdb/testsuite/gdb.cp/except-multi-location-lib.cc
new file mode 100644
index 00000000000..8767d756b33
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/except-multi-location-lib.cc
@@ -0,0 +1,25 @@
+/* Copyright 2019 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+void
+trycatch_lib ()
+{
+  try
+    {
+      throw 1;
+    }
+  catch (...)
+    {
+    }
+}
diff --git a/gdb/testsuite/gdb.cp/except-multi-location-main.cc b/gdb/testsuite/gdb.cp/except-multi-location-main.cc
new file mode 100644
index 00000000000..161897a22ee
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/except-multi-location-main.cc
@@ -0,0 +1,36 @@
+/* Copyright 2019 Free Software Foundation, Inc.
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+extern void trycatch_lib ();
+
+void
+trycatch_main ()
+{
+  try
+    {
+      throw 1;
+    }
+  catch (...)
+    {
+    }
+}
+
+int
+main ()
+{
+  trycatch_lib ();
+  trycatch_main ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.cp/except-multi-location.exp b/gdb/testsuite/gdb.cp/except-multi-location.exp
new file mode 100644
index 00000000000..60d6d0b9351
--- /dev/null
+++ b/gdb/testsuite/gdb.cp/except-multi-location.exp
@@ -0,0 +1,87 @@
+# Copyright 2019 Free Software Foundation, Inc.
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Regression test for a GDB internal error that would trigger if a
+# "catch catch" catchpoint ended up with multiple locations.  This
+# testcase exercises that scenario by building the binary with
+# -static-libgcc/-static-libstdc++ and a shared library that depends
+# on the libstc++.so DSO (which is how GDB was built and revealed the
+# bug), and vice versa.
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+# STATIC_BIN indicates whether to build the main binary with
+# -static-libgcc/-static-libstdc++.  STATIC_LIB is the same, but for
+# the shared library.
+proc test_multi_libstdcpp {static_bin static_lib} {
+    global srcdir subdir
+    global decimal hex
+
+    # Library file.
+    set libname "except-multi-location-lib"
+    set srcfile_lib ${srcdir}/${subdir}/${libname}.cc
+    set binfile_lib [standard_output_file ${libname}-$static_bin-$static_lib.so]
+    set lib_flags {debug c++}
+    if {$static_lib} {
+ lappend lib_flags additional_flags=-static-libgcc additional_flags=-static-libstdc++
+    }
+
+    # Binary file.
+    set testfile "except-multi-location-main"
+    set srcfile ${srcdir}/${subdir}/${testfile}.cc
+    set binfile [standard_output_file ${testfile}-$static_bin-$static_lib]
+    set bin_flags [list debug c++ shlib=${binfile_lib}]
+    if {$static_bin} {
+ lappend bin_flags additional_flags=-static-libgcc additional_flags=-static-libstdc++
+    }
+
+    if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
+ || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
+ untested "failed to compile"
+ return -1
+    }
+
+    clean_restart
+
+    gdb_load ${binfile}
+    gdb_load_shlib $binfile_lib
+
+    if ![runto_main] {
+ fail "can't run to main"
+ return 0
+    }
+
+    gdb_test "catch catch" "Catchpoint \[0-9\]+ \\(catch\\)" \
+ "catch catch"
+    gdb_test "catch throw" "Catchpoint \[0-9\]+ \\(throw\\)" \
+ "catch throw"
+    gdb_test "catch rethrow" "Catchpoint \[0-9\]+ \\(rethrow\\)" \
+ "catch rethrow"
+
+    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"]
+}
+
+# Try different static/not-static combinations.
+foreach_with_prefix static_lib {"0" "1"} {
+    foreach_with_prefix static_bin {"0" "1"} {
+ test_multi_libstdcpp $static_lib $static_bin
+    }
+}
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Pedro Alves-7
In reply to this post by Pedro Alves-7
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

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 <[hidden email]>
  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  <[hidden email]>

        * 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  <[hidden email]>

        * 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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Andrew Burgess
* Pedro Alves <[hidden email]> [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 <[hidden email]>
>   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  <[hidden email]>
>
> * 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  <[hidden email]>
>
> * 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
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Fix "info break" + "catch catch" + -static-{libstdc++,libgcc}

Tom Tromey-2
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:

Pedro> Irrespective of whether it's a good idea to use
Pedro> -static-libgcc/-static-libstdc++, GDB should not crash.  Since there
Pedro> are two copies of the code, it seems right to have more than one
Pedro> location.  So the fix is just to remove the assertion.

Thanks for doing this.

It turns out that this was reported multiple times in bugzilla.  I dup'd
them down to just PR c++/15468.

The patch looks good to me.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

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

This makes sense to me as well.

I read the patch and it looked fine to me.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Pedro Alves-7
In reply to this post by Andrew Burgess
On 7/9/19 10:55 AM, Andrew Burgess wrote:

>> 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.

Indeed, that crossed my mind too, though at first I thought it would
be easier to do with my "always show locations" change.

So, the additional patch below gets us this:

 (gdb) info breakpoints  
 Num     Type           Disp Enb Address            What
 2       catchpoint     keep y                      exception catch

With multiple locations:

 (gdb) maint info breakpoints  
 Num     Type           Disp Enb Address            What
 2       catchpoint     keep y                      exception catch
 2.1                         y   0x000000000040545f <__cxa_begin_catch+95> inf 1
 2.2                         y   0x00007ffff71dbe0f <__cxxabiv1::__cxa_begin_catch(void*)+95> inf 1
 (gdb)

With a single location:

 (gdb) maint info breakpoints 2
 Num     Type           Disp Enb Address            What
 2       catchpoint     keep y                      exception catch inf 1
 2.1                         y   0x00007ffff7bc0b7f <__cxa_begin_catch+95> inf 1

With no locations:

 (gdb) maint info breakpoints 2
 Num     Type           Disp Enb Address            What
 2       catchpoint     keep y                      exception catch inf 1


Other catchpoints still show the same way, here a catch signal:

 (gdb) info breakpoints  
 Num     Type           Disp Enb Address            What
 3       catchpoint     keep y                      signal "<standard signals>"

 (gdb) maint info breakpoints  
 Num     Type           Disp Enb Address            What
 3       catchpoint     keep y                      signal "<standard signals>"  inf 1
 (gdb)

WDYT?

Note: I considered making the locations be printed from within
breakpoint_ops::print_one(), but gave up given the handling
for the broken MI v2 output:

 /* The mi2 broken format: the main breakpoint tuple ends here, the locations
     are outside.  */
  if (!use_fixed_output)
    bkpt_tuple_emitter.reset ();

in print_one_breakpoint.

From 3f315479a9b802d8da92116d49637f73dbb1b483 Mon Sep 17 00:00:00 2001
From: Pedro Alves <[hidden email]>
Date: Tue, 9 Jul 2019 12:40:25 +0100
Subject: [PATCH] Make "maint info breakpoints" show "catch
 catch/throw/rethrow" locations

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <[hidden email]>

        * break-catch-throw.c (is_exception_catchpoint): New.
        * breakpoint.c (print_one_breakpoint_location): New parameter
        'raw_loc'.  Handle it.  Use
        is_watchpoint/is_catchpoint/is_exception_catchpoint instead of
        looking at the breakpoint's type.
        (print_one_breakpoint): If handling "maint info breakpoints", also
        print locations of exception catchpoints.
        * breakpoint.h (is_exception_catchpoint): Declare.
---
 gdb/break-catch-throw.c |   8 ++++
 gdb/breakpoint.c        | 115 +++++++++++++++++++-----------------------------
 gdb/breakpoint.h        |   5 +++
 3 files changed, 58 insertions(+), 70 deletions(-)

diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
index c0b3eec63d1..17e3d26d376 100644
--- a/gdb/break-catch-throw.c
+++ b/gdb/break-catch-throw.c
@@ -83,6 +83,14 @@ struct exception_catchpoint : public breakpoint
   std::unique_ptr<compiled_regex> pattern;
 };
 
+/* See breakpoint.h.  */
+
+bool
+is_exception_catchpoint (breakpoint *bp)
+{
+  return bp->ops == &gnu_v3_exception_catchpoint_ops;
+}
+
 
 
 /* A helper function that fetches exception probe arguments.  This
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 36500758257..25b3dc17476 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -5974,14 +5974,18 @@ output_thread_groups (struct ui_out *uiout,
     }
 }
 
-/* Print B to gdb_stdout.  */
+/* Print B to gdb_stdout.  If RAW_LOC, print raw breakpoint locations
+   instead of going via breakpoint_ops::print_one.  This makes "maint
+   info breakpoints" show the software breakpoint locations of
+   catchpoints, which are considered internal implementation
+   detail.  */
 
 static void
 print_one_breakpoint_location (struct breakpoint *b,
        struct bp_location *loc,
        int loc_number,
        struct bp_location **last_loc,
-       int allflag)
+       int allflag, bool raw_loc)
 {
   struct command_line *l;
   static char bpenables[] = "nynny";
@@ -6034,20 +6038,11 @@ print_one_breakpoint_location (struct breakpoint *b,
     uiout->field_fmt ("enabled", "%c", bpenables[(int) b->enable_state]);
 
   /* 5 and 6 */
-  if (b->ops != NULL && b->ops->print_one != NULL)
+  if (!raw_loc && b->ops != NULL && b->ops->print_one != NULL)
     b->ops->print_one (b, last_loc);
   else
-    switch (b->type)
-      {
-      case bp_none:
- internal_error (__FILE__, __LINE__,
- _("print_one_breakpoint: bp_none encountered\n"));
- break;
-
-      case bp_watchpoint:
-      case bp_hardware_watchpoint:
-      case bp_read_watchpoint:
-      case bp_access_watchpoint:
+    {
+      if (is_watchpoint (b))
  {
   struct watchpoint *w = (struct watchpoint *) b;
 
@@ -6059,55 +6054,26 @@ print_one_breakpoint_location (struct breakpoint *b,
   annotate_field (5);
   uiout->field_string ("what", w->exp_string);
  }
- break;
-
-      case bp_breakpoint:
-      case bp_hardware_breakpoint:
-      case bp_single_step:
-      case bp_until:
-      case bp_finish:
-      case bp_longjmp:
-      case bp_longjmp_resume:
-      case bp_longjmp_call_dummy:
-      case bp_exception:
-      case bp_exception_resume:
-      case bp_step_resume:
-      case bp_hp_step_resume:
-      case bp_watchpoint_scope:
-      case bp_call_dummy:
-      case bp_std_terminate:
-      case bp_shlib_event:
-      case bp_thread_event:
-      case bp_overlay_event:
-      case bp_longjmp_master:
-      case bp_std_terminate_master:
-      case bp_exception_master:
-      case bp_tracepoint:
-      case bp_fast_tracepoint:
-      case bp_static_tracepoint:
-      case bp_dprintf:
-      case bp_jit_event:
-      case bp_gnu_ifunc_resolver:
-      case bp_gnu_ifunc_resolver_return:
- if (opts.addressprint)
-  {
-    annotate_field (4);
-    if (header_of_multiple)
-      uiout->field_string ("addr", "<MULTIPLE>");
-    else if (b->loc == NULL || loc->shlib_disabled)
-      uiout->field_string ("addr", "<PENDING>");
-    else
-      uiout->field_core_addr ("addr",
-      loc->gdbarch, loc->address);
-  }
- annotate_field (5);
- if (!header_of_multiple)
-  print_breakpoint_location (b, loc);
- if (b->loc)
-  *last_loc = b->loc;
- break;
-      }
-
+      else if (!is_catchpoint (b) || is_exception_catchpoint (b))
+ {
+  if (opts.addressprint)
+    {
+      annotate_field (4);
+      if (header_of_multiple)
+ uiout->field_string ("addr", "<MULTIPLE>");
+      else if (b->loc == NULL || loc->shlib_disabled)
+ uiout->field_string ("addr", "<PENDING>");
+      else
+ uiout->field_core_addr ("addr",
+ loc->gdbarch, loc->address);
+    }
+  annotate_field (5);
+  if (!header_of_multiple)
+    print_breakpoint_location (b, loc);
+  if (b->loc)
+    *last_loc = b->loc;
+ }
+    }
 
   if (loc != NULL && !header_of_multiple)
     {
@@ -6336,7 +6302,7 @@ print_one_breakpoint (struct breakpoint *b,
        || fix_multi_location_breakpoint_output_globally);
 
   gdb::optional<ui_out_emit_tuple> bkpt_tuple_emitter (gdb::in_place, uiout, "bkpt");
-  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
+  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag, false);
 
   /* The mi2 broken format: the main breakpoint tuple ends here, the locations
      are outside.  */
@@ -6346,7 +6312,9 @@ print_one_breakpoint (struct breakpoint *b,
   /* 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 (b->ops == NULL
+      || b->ops->print_one == NULL
+      || allflag)
     {
       /* If breakpoint has a single location that is disabled, we
  print it as if it had several locations, since otherwise it's
@@ -6354,10 +6322,16 @@ print_one_breakpoint (struct breakpoint *b,
  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))
+ internally, that's not a property exposed to user.
+
+ Likewise, while catchpoints may be implemented with
+ breakpoints (e.g., catch throw), that's not a property
+ exposed to user.  We do however display the internal
+ breakpoint locations with "maint info breakpoints".  */
+      if (!is_hardware_watchpoint (b)
+  && (!is_catchpoint (b) || is_exception_catchpoint (b))
+  && (allflag
+      || (b->loc && (b->loc->next || !b->loc->enabled))))
  {
   gdb::optional<ui_out_emit_list> locations_list;
 
@@ -6371,7 +6345,8 @@ print_one_breakpoint (struct breakpoint *b,
   for (bp_location *loc = b->loc; loc != NULL; loc = loc->next, ++n)
     {
       ui_out_emit_tuple loc_tuple_emitter (uiout, NULL);
-      print_one_breakpoint_location (b, loc, n, last_loc, allflag);
+      print_one_breakpoint_location (b, loc, n, last_loc,
+     allflag, allflag);
     }
  }
     }
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 6e9a35e82de..43dfa05bbb8 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -872,6 +872,11 @@ extern int is_breakpoint (const struct breakpoint *bpt);
 
 extern int is_watchpoint (const struct breakpoint *bpt);
 
+/* Return true if BPT is a C++ exception catchpoint (catch
+   catch/throw/rethrow).  */
+
+extern bool is_exception_catchpoint (breakpoint *bp);
+
 /* An instance of this type is used to represent all kinds of
    tracepoints.  */
 
--
2.14.5

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Tom Tromey-2
>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:

Pedro>  (gdb) maint info breakpoints  
Pedro>  Num     Type           Disp Enb Address            What
Pedro>  2       catchpoint     keep y                      exception catch
Pedro>  2.1                         y   0x000000000040545f <__cxa_begin_catch+95> inf 1
Pedro>  2.2                         y   0x00007ffff71dbe0f <__cxxabiv1::__cxa_begin_catch(void*)+95> inf 1
Pedro>  (gdb)

Pedro> WDYT?

This looks great to me.

Pedro> Note: I considered making the locations be printed from within
Pedro> breakpoint_ops::print_one(), but gave up given the handling
Pedro> for the broken MI v2 output:

Pedro>  /* The mi2 broken format: the main breakpoint tuple ends here, the locations
Pedro>      are outside.  */
Pedro>   if (!use_fixed_output)
Pedro>     bkpt_tuple_emitter.reset ();

Pedro> in print_one_breakpoint.

Ouch.  It would be nice if we could find a way to make the backward
compatibility hacks not impact the main code.  Like, hacks in
mi/something.c somewhere instead would be preferable.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Andrew Burgess
In reply to this post by Pedro Alves-7
* Pedro Alves <[hidden email]> [2019-07-09 16:47:27 +0100]:

> On 7/9/19 10:55 AM, Andrew Burgess wrote:
>
> >> 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.
>
> Indeed, that crossed my mind too, though at first I thought it would
> be easier to do with my "always show locations" change.
>
> So, the additional patch below gets us this:
>
>  (gdb) info breakpoints  
>  Num     Type           Disp Enb Address            What
>  2       catchpoint     keep y                      exception catch
>
> With multiple locations:
>
>  (gdb) maint info breakpoints  
>  Num     Type           Disp Enb Address            What
>  2       catchpoint     keep y                      exception catch
>  2.1                         y   0x000000000040545f <__cxa_begin_catch+95> inf 1
>  2.2                         y   0x00007ffff71dbe0f <__cxxabiv1::__cxa_begin_catch(void*)+95> inf 1
>  (gdb)
>
> With a single location:
>
>  (gdb) maint info breakpoints 2
>  Num     Type           Disp Enb Address            What
>  2       catchpoint     keep y                      exception catch inf 1
>  2.1                         y   0x00007ffff7bc0b7f <__cxa_begin_catch+95> inf 1
>
> With no locations:
>
>  (gdb) maint info breakpoints 2
>  Num     Type           Disp Enb Address            What
>  2       catchpoint     keep y                      exception catch inf 1
>
>
> Other catchpoints still show the same way, here a catch signal:
>
>  (gdb) info breakpoints  
>  Num     Type           Disp Enb Address            What
>  3       catchpoint     keep y                      signal "<standard signals>"
>
>  (gdb) maint info breakpoints  
>  Num     Type           Disp Enb Address            What
>  3       catchpoint     keep y                      signal "<standard signals>"  inf 1
>  (gdb)
>
> WDYT?

This looks good to me.

Thanks,
Andrew


>
> Note: I considered making the locations be printed from within
> breakpoint_ops::print_one(), but gave up given the handling
> for the broken MI v2 output:
>
>  /* The mi2 broken format: the main breakpoint tuple ends here, the locations
>      are outside.  */
>   if (!use_fixed_output)
>     bkpt_tuple_emitter.reset ();
>
> in print_one_breakpoint.
>
> From 3f315479a9b802d8da92116d49637f73dbb1b483 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <[hidden email]>
> Date: Tue, 9 Jul 2019 12:40:25 +0100
> Subject: [PATCH] Make "maint info breakpoints" show "catch
>  catch/throw/rethrow" locations
>
> gdb/ChangeLog:
> yyyy-mm-dd  Pedro Alves  <[hidden email]>
>
> * break-catch-throw.c (is_exception_catchpoint): New.
> * breakpoint.c (print_one_breakpoint_location): New parameter
> 'raw_loc'.  Handle it.  Use
> is_watchpoint/is_catchpoint/is_exception_catchpoint instead of
> looking at the breakpoint's type.
> (print_one_breakpoint): If handling "maint info breakpoints", also
> print locations of exception catchpoints.
> * breakpoint.h (is_exception_catchpoint): Declare.
> ---
>  gdb/break-catch-throw.c |   8 ++++
>  gdb/breakpoint.c        | 115 +++++++++++++++++++-----------------------------
>  gdb/breakpoint.h        |   5 +++
>  3 files changed, 58 insertions(+), 70 deletions(-)
>
> diff --git a/gdb/break-catch-throw.c b/gdb/break-catch-throw.c
> index c0b3eec63d1..17e3d26d376 100644
> --- a/gdb/break-catch-throw.c
> +++ b/gdb/break-catch-throw.c
> @@ -83,6 +83,14 @@ struct exception_catchpoint : public breakpoint
>    std::unique_ptr<compiled_regex> pattern;
>  };
>  
> +/* See breakpoint.h.  */
> +
> +bool
> +is_exception_catchpoint (breakpoint *bp)
> +{
> +  return bp->ops == &gnu_v3_exception_catchpoint_ops;
> +}
> +
>  
>  
>  /* A helper function that fetches exception probe arguments.  This
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 36500758257..25b3dc17476 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -5974,14 +5974,18 @@ output_thread_groups (struct ui_out *uiout,
>      }
>  }
>  
> -/* Print B to gdb_stdout.  */
> +/* Print B to gdb_stdout.  If RAW_LOC, print raw breakpoint locations
> +   instead of going via breakpoint_ops::print_one.  This makes "maint
> +   info breakpoints" show the software breakpoint locations of
> +   catchpoints, which are considered internal implementation
> +   detail.  */
>  
>  static void
>  print_one_breakpoint_location (struct breakpoint *b,
>         struct bp_location *loc,
>         int loc_number,
>         struct bp_location **last_loc,
> -       int allflag)
> +       int allflag, bool raw_loc)
>  {
>    struct command_line *l;
>    static char bpenables[] = "nynny";
> @@ -6034,20 +6038,11 @@ print_one_breakpoint_location (struct breakpoint *b,
>      uiout->field_fmt ("enabled", "%c", bpenables[(int) b->enable_state]);
>  
>    /* 5 and 6 */
> -  if (b->ops != NULL && b->ops->print_one != NULL)
> +  if (!raw_loc && b->ops != NULL && b->ops->print_one != NULL)
>      b->ops->print_one (b, last_loc);
>    else
> -    switch (b->type)
> -      {
> -      case bp_none:
> - internal_error (__FILE__, __LINE__,
> - _("print_one_breakpoint: bp_none encountered\n"));
> - break;
> -
> -      case bp_watchpoint:
> -      case bp_hardware_watchpoint:
> -      case bp_read_watchpoint:
> -      case bp_access_watchpoint:
> +    {
> +      if (is_watchpoint (b))
>   {
>    struct watchpoint *w = (struct watchpoint *) b;
>  
> @@ -6059,55 +6054,26 @@ print_one_breakpoint_location (struct breakpoint *b,
>    annotate_field (5);
>    uiout->field_string ("what", w->exp_string);
>   }
> - break;
> -
> -      case bp_breakpoint:
> -      case bp_hardware_breakpoint:
> -      case bp_single_step:
> -      case bp_until:
> -      case bp_finish:
> -      case bp_longjmp:
> -      case bp_longjmp_resume:
> -      case bp_longjmp_call_dummy:
> -      case bp_exception:
> -      case bp_exception_resume:
> -      case bp_step_resume:
> -      case bp_hp_step_resume:
> -      case bp_watchpoint_scope:
> -      case bp_call_dummy:
> -      case bp_std_terminate:
> -      case bp_shlib_event:
> -      case bp_thread_event:
> -      case bp_overlay_event:
> -      case bp_longjmp_master:
> -      case bp_std_terminate_master:
> -      case bp_exception_master:
> -      case bp_tracepoint:
> -      case bp_fast_tracepoint:
> -      case bp_static_tracepoint:
> -      case bp_dprintf:
> -      case bp_jit_event:
> -      case bp_gnu_ifunc_resolver:
> -      case bp_gnu_ifunc_resolver_return:
> - if (opts.addressprint)
> -  {
> -    annotate_field (4);
> -    if (header_of_multiple)
> -      uiout->field_string ("addr", "<MULTIPLE>");
> -    else if (b->loc == NULL || loc->shlib_disabled)
> -      uiout->field_string ("addr", "<PENDING>");
> -    else
> -      uiout->field_core_addr ("addr",
> -      loc->gdbarch, loc->address);
> -  }
> - annotate_field (5);
> - if (!header_of_multiple)
> -  print_breakpoint_location (b, loc);
> - if (b->loc)
> -  *last_loc = b->loc;
> - break;
> -      }
> -
> +      else if (!is_catchpoint (b) || is_exception_catchpoint (b))
> + {
> +  if (opts.addressprint)
> +    {
> +      annotate_field (4);
> +      if (header_of_multiple)
> + uiout->field_string ("addr", "<MULTIPLE>");
> +      else if (b->loc == NULL || loc->shlib_disabled)
> + uiout->field_string ("addr", "<PENDING>");
> +      else
> + uiout->field_core_addr ("addr",
> + loc->gdbarch, loc->address);
> +    }
> +  annotate_field (5);
> +  if (!header_of_multiple)
> +    print_breakpoint_location (b, loc);
> +  if (b->loc)
> +    *last_loc = b->loc;
> + }
> +    }
>  
>    if (loc != NULL && !header_of_multiple)
>      {
> @@ -6336,7 +6302,7 @@ print_one_breakpoint (struct breakpoint *b,
>         || fix_multi_location_breakpoint_output_globally);
>  
>    gdb::optional<ui_out_emit_tuple> bkpt_tuple_emitter (gdb::in_place, uiout, "bkpt");
> -  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag);
> +  print_one_breakpoint_location (b, NULL, 0, last_loc, allflag, false);
>  
>    /* The mi2 broken format: the main breakpoint tuple ends here, the locations
>       are outside.  */
> @@ -6346,7 +6312,9 @@ print_one_breakpoint (struct breakpoint *b,
>    /* 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 (b->ops == NULL
> +      || b->ops->print_one == NULL
> +      || allflag)
>      {
>        /* If breakpoint has a single location that is disabled, we
>   print it as if it had several locations, since otherwise it's
> @@ -6354,10 +6322,16 @@ print_one_breakpoint (struct breakpoint *b,
>   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))
> + internally, that's not a property exposed to user.
> +
> + Likewise, while catchpoints may be implemented with
> + breakpoints (e.g., catch throw), that's not a property
> + exposed to user.  We do however display the internal
> + breakpoint locations with "maint info breakpoints".  */
> +      if (!is_hardware_watchpoint (b)
> +  && (!is_catchpoint (b) || is_exception_catchpoint (b))
> +  && (allflag
> +      || (b->loc && (b->loc->next || !b->loc->enabled))))
>   {
>    gdb::optional<ui_out_emit_list> locations_list;
>  
> @@ -6371,7 +6345,8 @@ print_one_breakpoint (struct breakpoint *b,
>    for (bp_location *loc = b->loc; loc != NULL; loc = loc->next, ++n)
>      {
>        ui_out_emit_tuple loc_tuple_emitter (uiout, NULL);
> -      print_one_breakpoint_location (b, loc, n, last_loc, allflag);
> +      print_one_breakpoint_location (b, loc, n, last_loc,
> +     allflag, allflag);
>      }
>   }
>      }
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 6e9a35e82de..43dfa05bbb8 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -872,6 +872,11 @@ extern int is_breakpoint (const struct breakpoint *bpt);
>  
>  extern int is_watchpoint (const struct breakpoint *bpt);
>  
> +/* Return true if BPT is a C++ exception catchpoint (catch
> +   catch/throw/rethrow).  */
> +
> +extern bool is_exception_catchpoint (breakpoint *bp);
> +
>  /* An instance of this type is used to represent all kinds of
>     tracepoints.  */
>  
> --
> 2.14.5
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Fix "info break" + "catch catch" + -static-{libstdc++,libgcc}

Pedro Alves-7
In reply to this post by Tom Tromey-2
On 7/9/19 3:09 PM, Tom Tromey wrote:

>>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:
>
> Pedro> Irrespective of whether it's a good idea to use
> Pedro> -static-libgcc/-static-libstdc++, GDB should not crash.  Since there
> Pedro> are two copies of the code, it seems right to have more than one
> Pedro> location.  So the fix is just to remove the assertion.
>
> Thanks for doing this.
>
> It turns out that this was reported multiple times in bugzilla.  I dup'd
> them down to just PR c++/15468.
>
> The patch looks good to me.

Thanks for doing that.  I added the PR number to the ChangeLogs and
pushed it in.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Pedro Alves-7
In reply to this post by Andrew Burgess
On 7/9/19 5:22 PM, Andrew Burgess wrote:
> This looks good to me.

On 7/9/19 5:07 PM, Tom Tromey wrote:
> This looks great to me.

Thanks.  I pushed it in with a couple additional & obvious tweaks:

- A tweak to gdb.mi/mi-catch-cpp-exceptions.exp, to explicitly
  check for type="catchpoint".

- A tweak to the manual to adjust the MI -catch-* examples.

From cb1e4e32c2d9588e1d77ea49bafb5dcdc1cff764 Mon Sep 17 00:00:00 2001
From: Pedro Alves <[hidden email]>
Date: Tue, 9 Jul 2019 19:26:16 +0100
Subject: [PATCH] "catch catch/throw/rethrow", breakpoint -> catchpoint

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

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 <[hidden email]>
  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:
2019-07-09  Pedro Alves  <[hidden email]>

        * 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/doc/ChangeLog:
2019-07-09  Pedro Alves  <[hidden email]>

        * gdb.texinfo (C++ Exception GDB/MI Catchpoint Commands): Adjust
        examples to show type=catchpoint instead of type=breakpoint and an
        address.

gdb/testsuite/ChangeLog:
2019-07-09  Pedro Alves  <[hidden email]>

        * 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.mi/mi-catch-cpp-exceptions.exp (setup_catchpoint): Expect
        'type="catchpoint"'.
---
 gdb/ChangeLog                                    |  31 +++++++
 gdb/doc/ChangeLog                                |   6 ++
 gdb/testsuite/ChangeLog                          |  10 +++
 gdb/doc/gdb.texinfo                              |  18 ++--
 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.mi/mi-catch-cpp-exceptions.exp |   2 +-
 gdb/testsuite/gdb.python/py-breakpoint.exp       |   5 +-
 12 files changed, 141 insertions(+), 103 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index de2b02ca42b..9ceb425a6bf 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,34 @@
+2019-07-09  Pedro Alves  <[hidden email]>
+
+ * 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.
+
 2019-07-09  Pedro Alves  <[hidden email]>
 
  PR c++/15468
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 10259160038..fe3df3d4d19 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,9 @@
+2019-07-09  Pedro Alves  <[hidden email]>
+
+ * gdb.texinfo (C++ Exception GDB/MI Catchpoint Commands): Adjust
+ examples to show type=catchpoint instead of type=breakpoint and an
+ address.
+
 2019-07-03  Pedro Alves  <[hidden email]>
     Philippe Waroquiers  <[hidden email]>
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 46a16470446..fcf86bd2c01 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,13 @@
+2019-07-09  Pedro Alves  <[hidden email]>
+
+ * 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.mi/mi-catch-cpp-exceptions.exp (setup_catchpoint): Expect
+ 'type="catchpoint"'.
+
 2019-07-09  Pedro Alves  <[hidden email]>
 
  PR c++/15468
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 299c4a12a15..eddd939869a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -30120,9 +30120,9 @@ and @samp{tcatch throw} (@pxref{Set Catchpoints}).
 
 @smallexample
 -catch-throw -r exception_type
-^done,bkpt=@{number="1",type="breakpoint",disp="keep",enabled="y",
-  addr="0x00000000004006c0",what="exception throw",
-  catch-type="throw",thread-groups=["i1"],
+^done,bkpt=@{number="1",type="catchpoint",disp="keep",enabled="y",
+  what="exception throw",catch-type="throw",
+  thread-groups=["i1"],
   regexp="exception_type",times="0"@}
 (gdb)
 -exec-run
@@ -30164,9 +30164,9 @@ and @samp{tcatch rethrow} (@pxref{Set Catchpoints}).
 
 @smallexample
 -catch-rethrow -r exception_type
-^done,bkpt=@{number="1",type="breakpoint",disp="keep",enabled="y",
-  addr="0x00000000004006c0",what="exception rethrow",
-  catch-type="rethrow",thread-groups=["i1"],
+^done,bkpt=@{number="1",type="catchpoint",disp="keep",enabled="y",
+  what="exception rethrow",catch-type="rethrow",
+  thread-groups=["i1"],
   regexp="exception_type",times="0"@}
 (gdb)
 -exec-run
@@ -30208,9 +30208,9 @@ and @samp{tcatch catch} (@pxref{Set Catchpoints}).
 
 @smallexample
 -catch-catch -r exception_type
-^done,bkpt=@{number="1",type="breakpoint",disp="keep",enabled="y",
-  addr="0x00000000004006c0",what="exception catch",
-  catch-type="catch",thread-groups=["i1"],
+^done,bkpt=@{number="1",type="catchpoint",disp="keep",enabled="y",
+  what="exception catch",catch-type="catch",
+  thread-groups=["i1"],
   regexp="exception_type",times="0"@}
 (gdb)
 -exec-run
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 db46741fd1d..1cdeade3af7 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.  */
@@ -8639,11 +8643,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;
@@ -11451,10 +11456,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)
@@ -11794,7 +11798,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
@@ -11934,7 +11938,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 3c9d5069b76..bcfa45b56e6 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.mi/mi-catch-cpp-exceptions.exp b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
index fa5b11e3e58..80ecabbe555 100644
--- a/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
+++ b/gdb/testsuite/gdb.mi/mi-catch-cpp-exceptions.exp
@@ -85,7 +85,7 @@ proc continue_to_breakpoint_in_main {} {
 proc setup_catchpoint {type {extra ""}} {
     global decimal
     mi_gdb_test "-catch-${type} ${extra}" \
- "\\^done,bkpt=\{number=\"$decimal\".*what=\"exception ${type}\",catch-type=\"${type}\".*\}" \
+ "\\^done,bkpt=\{number=\"$decimal\",type=\"catchpoint\".*what=\"exception ${type}\",catch-type=\"${type}\".*\}" \
  "Setup -catch-${type}"
 }
 
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

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Pedro Alves-7
On 7/9/19 8:06 PM, Pedro Alves wrote:
> On 7/9/19 5:22 PM, Andrew Burgess wrote:
>> This looks good to me.
>
> On 7/9/19 5:07 PM, Tom Tromey wrote:
>> This looks great to me.

And I've pushed the "maint info breakpoints" change as well.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint

Tom Tromey-4
In reply to this post by Pedro Alves-7
>>>>> "Pedro" == Pedro Alves <[hidden email]> writes:

Pedro> (Note: might make sense to do the same to Ada exception catchpoints.)

I'll send a patch shortly that does this.

Tom
Reply | Threaded
Open this post in threaded view
|

Regression on gdb.trace/{actions,change-loc,pending}.exp on native-{,extended-}gdbserver (was: Re: [PATCH 2/2] "catch catch/throw/rethrow", breakpoint -> catchpoint)

Sergio Durigan Junior
In reply to this post by Pedro Alves-7
On Tuesday, July 09 2019, Pedro Alves wrote:

> On 7/9/19 5:22 PM, Andrew Burgess wrote:
>> This looks good to me.
>
> On 7/9/19 5:07 PM, Tom Tromey wrote:
>> This looks great to me.
>
> Thanks.  I pushed it in with a couple additional & obvious tweaks:
>
> - A tweak to gdb.mi/mi-catch-cpp-exceptions.exp, to explicitly
>   check for type="catchpoint".
>
> - A tweak to the manual to adjust the MI -catch-* examples.

Hi Pedro,

BuildBot has caught these regressions when testing this patch on
native-{,extended-}gdbserver:

  https://sourceware.org/ml/gdb-testers/2019-q3/msg00438.html

  PASS -> FAIL: gdb.trace/actions.exp: ctf: tracepoint on gdb_asm_test
  PASS -> FAIL: gdb.trace/actions.exp: ctf: tracepoint on gdb_c_test
  new FAIL: gdb.trace/actions.exp: ctf: tracepoint on gdb_recursion_test 0
  PASS -> FAIL: gdb.trace/actions.exp: tfile: tracepoint on gdb_asm_test
  PASS -> FAIL: gdb.trace/actions.exp: tfile: tracepoint on gdb_c_test
  new FAIL: gdb.trace/actions.exp: tfile: tracepoint on gdb_recursion_test 0
  PASS -> FAIL: gdb.trace/change-loc.exp: 1 trace: continue to marker 2
  PASS -> FAIL: gdb.trace/change-loc.exp: 1 trace: continue to marker 3
  PASS -> FAIL: gdb.trace/change-loc.exp: 1 trace: tfind frame 0
  PASS -> FAIL: gdb.trace/change-loc.exp: 1 trace: tracepoint with two locations - installed
  PASS -> FAIL: gdb.trace/change-loc.exp: 1 trace: tracepoint with two locations - pending
  PASS -> FAIL: gdb.trace/change-loc.exp: 2 trace: continue to marker 2
  PASS -> FAIL: gdb.trace/change-loc.exp: 2 trace: continue to marker 3
  PASS -> FAIL: gdb.trace/change-loc.exp: 2 trace: tfind frame 2
  PASS -> FAIL: gdb.trace/change-loc.exp: 2 trace: tracepoint with two locations - installed
  PASS -> FAIL: gdb.trace/change-loc.exp: 2 trace: tracepoint with two locations - pending
  PASS -> FAIL: gdb.trace/pending.exp: ftrace installed_in_trace: continue to marker 2
  new FAIL: gdb.trace/pending.exp: ftrace installed_in_trace: tfind test frame 0
  PASS -> FAIL: gdb.trace/pending.exp: trace installed_in_trace: continue to marker 2
  PASS -> FAIL: gdb.trace/pending.exp: trace installed_in_trace: tfind test frame 0

I've also found them while preparing a new Fedora GDB release.  I can
provide the log files and help with investigating if needed.

Thanks,

--
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/