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

Message ID 20190705160055.13355-2-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves July 5, 2019, 4 p.m. UTC
  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  <palves@redhat.com>

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

gdb/testsuite/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* 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
  

Comments

Tom Tromey July 9, 2019, 2:09 p.m. UTC | #1
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> 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
  
Pedro Alves July 9, 2019, 7:01 p.m. UTC | #2
On 7/9/19 3:09 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> 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
  

Patch

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
+    }
+}