Re: [patch] Fix a crash when displaying variables from shared library.

[Paul Pluzhnikov <ppluzhnikov@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

On Tue, Feb 10, 2009 at 5:43 PM, Tom Tromey <tromey@redhat.com> wrote:

> But, it seems to me that notifying the observer here is the right
> thing to do -- otherwise this observer is unreliable.

Vladimir beat me to it.

> This patch is ok.

It turns out that clearing all displays on solib unload was too
aggressive, as can be seen here:

/* --- noshlib.c --- */
int a_global;
int foo()
{
    static int a_static;
    int a_local;
    return a_global + a_local + a_static;
}

int main()
{
    return foo();
}
/* --- cut --- */

Before the patch:

gdb64-cvs -nx -ex 'set confirm off' ./noshlib
GNU gdb (GDB) 6.8.50.20090218-cvs
...
(gdb) b main
Breakpoint 1 at 0x40032a: file noshlib.c, line 11.
(gdb) b foo
Breakpoint 2 at 0x400310: file noshlib.c, line 6.
(gdb) r

Breakpoint 1, main () at noshlib.c:11
11          return foo();
(gdb) c

Breakpoint 2, foo () at noshlib.c:6
6           return a_global + a_local + a_static;
(gdb) display a_global
(gdb) display a_local
(gdb) display a_static
(gdb) r

Breakpoint 1, main () at noshlib.c:11
11          return foo();
3: a_static = 0
1: a_global = 0
(gdb) c

Breakpoint 2, foo () at noshlib.c:6
6           return a_global + a_local + a_static;
3: a_static = 0
2: a_local = 0
1: a_global = 0
(gdb) q

After the patch:

(gdb) b main
Breakpoint 1 at 0x40032a: file noshlib.c, line 11.
(gdb) b foo
Breakpoint 2 at 0x400310: file noshlib.c, line 6.
(gdb) r

Breakpoint 1, main () at noshlib.c:11
11          return foo();
(gdb) c

Breakpoint 2, foo () at noshlib.c:6
6           return a_global + a_local + a_static;
(gdb) display a_global
(gdb) display a_local
(gdb) display a_static
(gdb) r

Breakpoint 1, main () at noshlib.c:11
11          return foo();
warning: Unable to display "a_static": No symbol "a_static" in current context.
warning: Unable to display "a_local": No symbol "a_local" in current context.
1: a_global = 0
(gdb) c

Breakpoint 2, foo () at noshlib.c:6
6           return a_global + a_local + a_static;
1: a_global = 0


I've updated the patch to only clear displays which actually are
about to become dangling, and extended the test to check that displays
that refer to the main executable are not affected.

Thanks,
-- 
Paul Pluzhnikov

[-- Attachment #2: gdb-display-crash-20090218.txt --]
[-- Type: text/plain, Size: 9929 bytes --]

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.142
diff -u -p -u -r1.142 printcmd.c
--- printcmd.c	5 Feb 2009 00:13:43 -0000	1.142
+++ printcmd.c	18 Feb 2009 22:55:18 -0000
@@ -43,6 +43,9 @@
 #include "disasm.h"
 #include "dfp.h"
 #include "valprint.h"
+#include "exceptions.h"
+#include "observer.h"
+#include "solist.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1395,7 +1398,7 @@ display_command (char *exp, int from_tty
 	  fmt.count = 0;
 	}
 
-      innermost_block = 0;
+      innermost_block = NULL;
       expr = parse_expression (exp);
 
       new = (struct display *) xmalloc (sizeof (struct display));
@@ -1519,6 +1522,25 @@ do_one_display (struct display *d)
   if (d->enabled_p == 0)
     return;
 
+  if (d->exp == NULL)
+    {
+      volatile struct gdb_exception ex;
+      TRY_CATCH (ex, RETURN_MASK_ALL)
+	{
+	  innermost_block = NULL;
+	  d->exp = parse_expression (d->exp_string);
+	  d->block = innermost_block;
+	}
+      if (ex.reason < 0)
+	{
+	  /* Can't re-parse the expression. Disable this display item. */
+	  d->enabled_p = 0;
+	  warning (_("Unable to display \"%s\": %s"),
+		   d->exp_string, ex.message);
+	  return;
+	}
+    }
+
   if (d->block)
     within_current_scope = contained_in (get_selected_block (0), d->block);
   else
@@ -1731,6 +1753,70 @@ disable_display_command (char *args, int
 	  p++;
       }
 }
+
+/* Answer 1 if "d" uses "solib" (and will become dangling when "solib"
+   is unloaded), otherwise answer 0.  */
+
+static int
+display_uses_solib_p (const struct display *d,
+		      const struct so_list *solib)
+{
+  int i;
+  const CORE_ADDR addr_low = solib->addr_low;
+  const CORE_ADDR addr_high = solib->addr_high;
+
+  if (d->block != NULL
+      && addr_low <= d->block->startaddr && d->block->endaddr <= addr_high)
+    return 1;
+
+  for (i = 0; i < d->exp->nelts; i++)
+    {
+      union exp_element *elts = d->exp->elts;
+      if (elts[i].opcode == OP_VAR_VALUE)
+	{
+	  struct block *block = elts[++i].block;
+	  struct symbol *symbol = elts[++i].symbol;
+	  struct obj_section *section;
+
+	  gdb_assert (elts[++i].opcode == OP_VAR_VALUE);
+
+	  if (block
+	      && addr_low <= block->startaddr && block->endaddr <= addr_high)
+	    return 1;
+
+	  section = SYMBOL_OBJ_SECTION (symbol);
+	  if (section && section->objfile == solib->objfile)
+	    return 1;
+	}
+    }
+  return 0;
+}
+
+/* display_chain items point to blocks and expressions. Some
+   expressions in turn may point to symbols.
+   Both symbols and blocks are obstack_alloc'd on objfile_stack, and are
+   obstack_free'd when a shared library is unloaded.
+   Clear pointers that are about to become dangling.
+   Both .exp and .block will be restored next time we need to display an item
+   from the newly-loaded shared library.
+*/
+
+static void
+clear_dangling_display_expressions (struct so_list *solib)
+{
+  struct display *d;
+  struct objfile *objfile = NULL;
+
+  for (d = display_chain; d; d = d->next)
+    {
+      if (d->exp && display_uses_solib_p (d, solib))
+	{
+	  xfree (d->exp);
+	  d->exp = NULL;
+	  d->block = NULL;
+	}
+    }
+}
 \f
 
 /* Print the value in stack frame FRAME of a variable specified by a
@@ -2367,6 +2453,8 @@ _initialize_printcmd (void)
 
   current_display_number = -1;
 
+  observer_attach_solib_unloaded (clear_dangling_display_expressions);
+
   add_info ("address", address_info,
 	    _("Describe where symbol SYM is stored."));
 
Index: testsuite/gdb.base/solib-display-lib.c
===================================================================
RCS file: testsuite/gdb.base/solib-display-lib.c
diff -N testsuite/gdb.base/solib-display-lib.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/solib-display-lib.c	18 Feb 2009 22:55:19 -0000
@@ -0,0 +1,30 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009 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/>.  */
+
+int a_global = 41;
+#ifndef NO_B_GLOBAL
+int b_global = 42;
+#endif
+int c_global = 43;
+
+int foo () {
+  return a_global +
+#ifndef NO_B_GLOBAL
+    b_global +
+#endif
+    c_global;
+}
Index: testsuite/gdb.base/solib-display-main.c
===================================================================
RCS file: testsuite/gdb.base/solib-display-main.c
diff -N testsuite/gdb.base/solib-display-main.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/solib-display-main.c	18 Feb 2009 22:55:19 -0000
@@ -0,0 +1,32 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2009 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 int foo ();
+
+int main_global = 44;
+int bar ()
+{
+  int a_local = 45;
+  static int a_static = 46;
+  return main_global + a_local + a_static; /* break here */
+}
+
+int main ()
+{
+  bar ();
+  return foo ();
+}
Index: testsuite/gdb.base/solib-display.exp
===================================================================
RCS file: testsuite/gdb.base/solib-display.exp
diff -N testsuite/gdb.base/solib-display.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ testsuite/gdb.base/solib-display.exp	18 Feb 2009 22:55:19 -0000
@@ -0,0 +1,99 @@
+# Copyright 2009 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/>.
+#
+# Contributed by Paul Pluzhnikov <ppluzhnikov@google.com>
+#
+
+# This test case verifies that if a display is active on a variable
+# which belongs in a shared library, and that shared library is
+# reloaded (e.g. due to re-execution of the program), GDB will continue
+# to display it (gdb-6.8 crashed under this scenario).
+
+# Also test that a display of variable which is currently present in
+# a shared library, but disappears before re-run, doesn't cause GDB
+# difficulties, and that it continues to display other variables.
+
+# Finally, test that displays which refer to main executable
+# (and thus aren't affected by shared library unloading) are not
+# disabled prematurely.
+
+if {[skip_shlib_tests] || [is_remote target]} {
+    return 0
+}
+
+# Library file.
+set libname "solib-display-lib"
+set srcfile_lib ${srcdir}/${subdir}/${libname}.c
+set binfile_lib ${objdir}/${subdir}/${libname}.so
+set lib_flags [list debug]
+# Binary file.
+set testfile "solib-display-main"
+set srcfile ${srcdir}/${subdir}/${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+set bin_flags [list debug shlib=${binfile_lib}]
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} $lib_flags] != ""
+     || [gdb_compile ${srcfile} ${binfile} executable $bin_flags] != "" } {
+  untested "Could not compile $binfile_lib or $binfile."
+  return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+  fail "Can't run to main"
+  return 0
+}
+
+gdb_test "display a_global" "1: a_global = 41"
+gdb_test "display b_global" "2: b_global = 42"
+gdb_test "display c_global" "3: c_global = 43"
+gdb_test "continue"
+gdb_test "run" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
+gdb_test "continue"
+
+# Now rebuild the library without b_global
+if { [gdb_compile_shlib ${srcfile_lib} ${binfile_lib} \
+	  "$lib_flags additional_flags=-DNO_B_GLOBAL"] != ""} {
+    fail "Can't rebuild $binfile_lib"
+}
+
+gdb_test "run" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rebuild"
+
+# Now verify that displays which are not in the shared library
+# are not cleared permaturely.
+
+gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
+	".*Breakpoint.* at .*" 
+
+gdb_test "continue"
+gdb_test "display main_global" "4: main_global = 44"
+gdb_test "display a_local" "5: a_local = 45"
+gdb_test "display a_static" "6: a_static = 46"
+gdb_test "continue"
+gdb_test "run" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
+gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
+
+gdb_exit
+
+return 0
+
+