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

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

[Paul Pluzhnikov]

Greetings,

Consider this source:

/* main.c */
int main() { return foo(); }

/* foo.c; compile as a shared library */
int a_global;
int foo() { return ++a_global; }


If you set "display a_global", and re-run the program, GDB will
crash (gdb-6.7, 6.8, Head).

This is happening because display_chain expressions refer to
symbols, but solib symbols get freed and reloaded across inferior
restarts.

Attached patch adds a test case and fixes the problem.

Tested on x86_64-linux with no regressions.

OK to commit?

--
Paul Pluzhnikov

ChangeLog:

2009-02-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

        * printcmd.c (do_one_display): Reparse exp_string.
	(clear_dangling_display_expressions): New function.
	(_initialize_printcmd): Add observer.
	* solib.c (clear_solib): Add missing notification.

testsuite/ChangeLog:
	
2009-02-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* solib-display.exp: New file.
	* solib-display-main.c: New file.
	* solib-display-lib.c: New file.


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	5 Feb 2009 02:43:31 -0000
@@ -43,6 +43,7 @@
 #include "disasm.h"
 #include "dfp.h"
 #include "valprint.h"
+#include "observer.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1526,6 +1527,9 @@ do_one_display (struct display *d)
   if (!within_current_scope)
     return;
 
+  if (d->exp == NULL)
+    d->exp = parse_expression (d->exp_string);
+
   current_display_number = d->number;
 
   annotate_display_begin ();
@@ -1731,6 +1735,18 @@ disable_display_command (char *args, int
 	  p++;
       }
 }
+
+static void
+clear_dangling_display_expressions (struct so_list *solist)
+{
+  struct display *d;
+
+  for (d = display_chain; d; d = d->next)
+    {
+      xfree (d->exp);
+      d->exp = NULL;
+    }
+}
 \f
 
 /* Print the value in stack frame FRAME of a variable specified by a
@@ -2367,6 +2383,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: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.109
diff -u -p -u -r1.109 solib.c
--- solib.c	15 Jan 2009 16:35:22 -0000	1.109
+++ solib.c	5 Feb 2009 02:43:31 -0000
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
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	5 Feb 2009 02:43:31 -0000
@@ -0,0 +1,19 @@
+/* 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;
+int foo () { return ++a_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	5 Feb 2009 02:43:31 -0000
@@ -0,0 +1,23 @@
+/* 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 ()
+{
+  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	5 Feb 2009 02:43:31 -0000
@@ -0,0 +1,61 @@
+# 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>
+#
+
+if {[skip_shlib_tests]} {
+    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 ldflags=-Wl,-Bsymbolic]
+# 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"
+gdb_test "continue"
+gdb_test "run" "1: a_global = 0"
+
+gdb_exit
+
+return 0
+
+

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

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> @@ -1526,6 +1527,9 @@ do_one_display (struct display *d)
[...]
Paul> +  if (d->exp == NULL)
Paul> +    d->exp = parse_expression (d->exp_string);

Suppose this fails.  I think what would happen is that none of the
following displays would print anything.  However, it seems like what
should happen is that gdb deletes this display and moves on to try the
next.  What do you think?

Also, are blocks valid after an solib is deleted?  (I don't know.)  If
not then that means that the 'block' field must also be invalidated.

Paul> +
Paul> +static void
Paul> +clear_dangling_display_expressions (struct so_list *solist)

I think any new function needs an explanatory comment.

Paul> --- solib.c	15 Jan 2009 16:35:22 -0000	1.109
Paul> +++ solib.c	5 Feb 2009 02:43:31 -0000
Paul> @@ -908,6 +908,7 @@ clear_solib (void)
Paul>      {
Paul>        struct so_list *so = so_list_head;
Paul>        so_list_head = so->next;
Paul> +      observer_notify_solib_unloaded (so);
Paul>        if (so->abfd)

This hunk is already under discussion:

    http://sourceware.org/ml/gdb-patches/2009-01/msg00542.html

In particular, I think Daniel's question should be answered before
this goes in:

    http://sourceware.org/ml/gdb-patches/2009-02/msg00002.html

Paul> Index: testsuite/gdb.base/solib-display.exp
[...]
Paul> +gdb_test "display a_global"
Paul> +gdb_test "continue"
Paul> +gdb_test "run" "1: a_global = 0"

For regression tests, I like to add a comment explaining what is being
tested.  Could you do this?  It doesn't have to be long.  My reason
for doing this is that later on it isn't always obvious what a test is
trying to test.


Otherwise this patch looks good to me.

Tom

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

[Pedro Alves]

On Thursday 05 February 2009 03:02:57, Paul Pluzhnikov wrote:
> +gdb_test "display a_global"
> +gdb_test "continue"
> +gdb_test "run" "1: a_global = 0"
> +

Expecting an inferior exit like this, and using an hardcoded "run"
will make this test fail against (most) remote targets.

(there's a board file in the wiki you could use for easy
gdbserver testing, if you'd like)

Can you adapt the test to not use an hardcoded "run", or perhaps just
skip it against remote targets?

Thanks,
-- 
Pedro Alves

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

[Paul Pluzhnikov]

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

On Fri, Feb 6, 2009 at 1:36 PM, Tom Tromey <tromey@redhat.com> wrote:
>>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:
>
> Paul> @@ -1526,6 +1527,9 @@ do_one_display (struct display *d)
> [...]
> Paul> +  if (d->exp == NULL)
> Paul> +    d->exp = parse_expression (d->exp_string);
>
> Suppose this fails.  I think what would happen is that none of the
> following displays would print anything.  However, it seems like what
> should happen is that gdb deletes this display and moves on to try the
> next.  What do you think?

Fixed: I believe we should disable this display, not delete it
altogether though.

I extended the test case to verify this works.

> Also, are blocks valid after an solib is deleted?  (I don't know.)  If
> not then that means that the 'block' field must also be invalidated.

Fixed: the blocks are also obstack_free'd (AFAICT).

They should be cleared even if they weren't free'd: for all we know
reloaded shlib may have been completely re-factored, and the
expression could have moved to another block.

> Paul> +
> Paul> +static void
> Paul> +clear_dangling_display_expressions (struct so_list *solist)
>
> I think any new function needs an explanatory comment.

Done.

> Paul> --- solib.c       15 Jan 2009 16:35:22 -0000      1.109
> Paul> +++ solib.c       5 Feb 2009 02:43:31 -0000
> Paul> @@ -908,6 +908,7 @@ clear_solib (void)
> Paul>      {
> Paul>        struct so_list *so = so_list_head;
> Paul>        so_list_head = so->next;
> Paul> +      observer_notify_solib_unloaded (so);
> Paul>        if (so->abfd)
>
> This hunk is already under discussion:
>
>    http://sourceware.org/ml/gdb-patches/2009-01/msg00542.html
>
> In particular, I think Daniel's question should be answered before
> this goes in:
>
>    http://sourceware.org/ml/gdb-patches/2009-02/msg00002.html

This turned out to be non-issue: disable_breakpoints_in_shlibs()
is called just a couple of lines above, so by the time we get into
disable_breakpoints_in_unloaded_shlib(), all of them already have
loc->shlib_disable == 1, and it remains silent.

> Paul> Index: testsuite/gdb.base/solib-display.exp
> [...]
> Paul> +gdb_test "display a_global"
> Paul> +gdb_test "continue"
> Paul> +gdb_test "run" "1: a_global = 0"
>
> For regression tests, I like to add a comment explaining what is being
> tested.  Could you do this?  It doesn't have to be long.  My reason
> for doing this is that later on it isn't always obvious what a test is
> trying to test.

Done.

> Otherwise this patch looks good to me.
>
> Tom
>

On Fri, Feb 6, 2009 at 1:53 PM, Pedro Alves <pedro@codesourcery.com> wrote:
> On Thursday 05 February 2009 03:02:57, Paul Pluzhnikov wrote:
>> +gdb_test "display a_global"
>> +gdb_test "continue"
>> +gdb_test "run" "1: a_global = 0"
>> +
>
> Expecting an inferior exit like this, and using an hardcoded "run"
> will make this test fail against (most) remote targets.
>
> (there's a board file in the wiki you could use for easy
> gdbserver testing, if you'd like)
>
> Can you adapt the test to not use an hardcoded "run", or perhaps just
> skip it against remote targets?

Done.

Thanks for the reviews.

I re-tested on x86_64-linux with no regressions.

OK?
-- 
Paul Pluzhnikov

[-- Attachment #2: gdb-display-crash-20090206.txt --]
[-- Type: text/plain, Size: 8382 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	7 Feb 2009 02:31:00 -0000
@@ -43,6 +43,8 @@
 #include "disasm.h"
 #include "dfp.h"
 #include "valprint.h"
+#include "exceptions.h"
+#include "observer.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1395,7 +1397,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 +1521,24 @@ 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 +1751,28 @@ disable_display_command (char *args, int
 	  p++;
       }
 }
+
+/* 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 *solist)
+{
+  struct display *d;
+
+  for (d = display_chain; d; d = d->next)
+    {
+      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 +2409,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: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.109
diff -u -p -u -r1.109 solib.c
--- solib.c	15 Jan 2009 16:35:22 -0000	1.109
+++ solib.c	7 Feb 2009 02:31:00 -0000
@@ -908,6 +908,7 @@ clear_solib (void)
     {
       struct so_list *so = so_list_head;
       so_list_head = so->next;
+      observer_notify_solib_unloaded (so);
       if (so->abfd)
 	remove_target_sections (so->abfd);
       free_so (so);
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	7 Feb 2009 02:31:00 -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	7 Feb 2009 02:31:00 -0000
@@ -0,0 +1,23 @@
+/* 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 ()
+{
+  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	7 Feb 2009 02:31:00 -0000
@@ -0,0 +1,82 @@
+# 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.
+
+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"
+gdb_test "display b_global"
+gdb_test "display c_global"
+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"
+
+
+gdb_exit
+
+return 0
+
+

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

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

>> http://sourceware.org/ml/gdb-patches/2009-02/msg00002.html

Paul> This turned out to be non-issue: disable_breakpoints_in_shlibs()
Paul> is called just a couple of lines above, so by the time we get into
Paul> disable_breakpoints_in_unloaded_shlib(), all of them already have
Paul> loc-> shlib_disable == 1, and it remains silent.

The comment in that function says that this is true for ELF shared
libraries, but not others.

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

This patch is ok.

Tom

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

[Paul Pluzhnikov]

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

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

[Paul Pluzhnikov]

On Wed, Feb 18, 2009 at 2:58 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

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

Forgot to mention: regtested on Linux/x86_64 with no regressions.

Thanks,
-- 
Paul Pluzhnikov

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

[Joel Brobecker]

Hello Paul,

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

Good catch!

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

You forgot to include a ChangeLog...

> +	  /* Can't re-parse the expression. Disable this display item. */

Minor style issue: You need to have two spaces after each period
(one in the middle, and one at the end, before the "*/").

> +/* Answer 1 if "d" uses "solib" (and will become dangling when "solib"
> +   is unloaded), otherwise answer 0.  */

If you don't mind, I think using "Return 1" instead of "Answer 1" would
be more consistent with the other descriptions.

Another minor style correction: In GDB, we refer to the function
parameters by using their names in ALL_CAPS, and without the quotes.
So, in your case, you would write:

/* Return 1 if D uses SOLIB (and will become dangling [...] */

> +  if (d->block != NULL
> +      && addr_low <= d->block->startaddr && d->block->endaddr <= addr_high)
> +    return 1;

I suggest you use solib_address instead of doing the check yourself.
As mentioned by Daniel in another thread, shared libraries on SVR4
systems occupy a contiguous address block, but this is not the case
of DLLs where the data and text sections might be separate.
I verified that solib_address should handle the DLL case.

> +  for (i = 0; i < d->exp->nelts; i++)
> +    {
> +      union exp_element *elts = d->exp->elts;
> +      if (elts[i].opcode == OP_VAR_VALUE)

I'm afraid this isn't going to work for more complex structures...
The problem is that you might be reading an undefined field of
union exp_element. Imagine for instance that you have an expression
that looks like this: "foo->bar".

At one point, you'll encounter the following elements:

   [i  ]  ->  STRUCTOP_PTR
   [i+1]  ->  A string
   [i+2]  ->  STRUCTOP_PTR

Iterating over the expression, you'll ignore the element at index i,
and then check the opcode of the element at i+1, which is the wrong
field of the enum to access in this case...

I can't think of a way of doing what you're trying to do off the top
of my head. I'll have to think about it a little more. Perhaps others
will have suggestions... Or perhaps we'll have to attack the problem
with a different angle, I'm not very familiar with how "display"
expressions are handled...

> +gdb_test "run" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rebuild"

Can this be changed to use either one of the runto routines, or maybe
gdb_run_cmd if one of the above doesn't work in this case?

> +gdb_test "run" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"

Same here.

-- 
Joel

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

[Paul Pluzhnikov]

On Sun, Feb 22, 2009 at 5:07 PM, Joel Brobecker <brobecker@adacore.com> wrote:

>> +       /* Can't re-parse the expression. Disable this display item. */
>
> Minor style issue: You need to have two spaces after each period
> (one in the middle, and one at the end, before the "*/").

Done.

>> +/* Answer 1 if "d" uses "solib" (and will become dangling when "solib"
>> +   is unloaded), otherwise answer 0.  */
>
> If you don't mind, I think using "Return 1" instead of "Answer 1" would
> be more consistent with the other descriptions.

Done.

> Another minor style correction: In GDB, we refer to the function
> parameters by using their names in ALL_CAPS, and without the quotes.
> So, in your case, you would write:
>
> /* Return 1 if D uses SOLIB (and will become dangling [...] */

Done.

>> +  if (d->block != NULL
>> +      && addr_low <= d->block->startaddr && d->block->endaddr <= addr_high)
>> +    return 1;
>
> I suggest you use solib_address instead of doing the check yourself.
> As mentioned by Daniel in another thread, shared libraries on SVR4
> systems occupy a contiguous address block, but this is not the case
> of DLLs where the data and text sections might be separate.
> I verified that solib_address should handle the DLL case.

Done.

>> +  for (i = 0; i < d->exp->nelts; i++)
>> +    {
>> +      union exp_element *elts = d->exp->elts;
>> +      if (elts[i].opcode == OP_VAR_VALUE)
>
> I'm afraid this isn't going to work for more complex structures...
> The problem is that you might be reading an undefined field of
> union exp_element. Imagine for instance that you have an expression
> that looks like this: "foo->bar".
>
> At one point, you'll encounter the following elements:
>
>   [i  ]  ->  STRUCTOP_PTR
>   [i+1]  ->  A string
>   [i+2]  ->  STRUCTOP_PTR
>
> Iterating over the expression, you'll ignore the element at index i,
> and then check the opcode of the element at i+1, which is the wrong
> field of the enum to access in this case...

I was afraid of that ...

> I can't think of a way of doing what you're trying to do off the top
> of my head. I'll have to think about it a little more. Perhaps others
> will have suggestions... Or perhaps we'll have to attack the problem
> with a different angle, I'm not very familiar with how "display"
> expressions are handled...

One way I think this could be fixed is to refactor
e.g. dump_subexp_body_standard() to use "visitor pattern", and then
use a different callback for display_uses_solib_p().

Perhaps refactoring should be done first as a separate patch?

A simpler way is to cut/paste/modify dump_subexp_body_standard(),
but there is (IMHO) already too much code duplication between it
and e.g. evaluate_subexp_standard(), so I'd rather not introduce
a 3rd copy.

Another way would be to replace malloc()/realloc()s in
parse_exp_in_context() with calloc()s, so that all "unused" .opcode's
become OP_NULLs. This appears tricky, and there are language hooks
which may need similar fixes.


>> +gdb_test "run" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rebuild"
>
> Can this be changed to use either one of the runto routines, or maybe
> gdb_run_cmd if one of the above doesn't work in this case?

Note that this test is skipped when "is_remote target".
Is there still a need to use runto?

Is it better to use runto and *not* skip the test for remote targets?

>> +gdb_test "run" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
>
> Same here.

Thanks,
-- 
Paul Pluzhnikov

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

[Paul Pluzhnikov]

On Mon, Feb 23, 2009 at 10:12 AM, Paul Pluzhnikov
<ppluzhnikov@google.com> wrote:

> On Sun, Feb 22, 2009 at 5:07 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>>> +  for (i = 0; i < d->exp->nelts; i++)
>>> +    {
>>> +      union exp_element *elts = d->exp->elts;
>>> +      if (elts[i].opcode == OP_VAR_VALUE)
>>
>> I'm afraid this isn't going to work for more complex structures...
>> The problem is that you might be reading an undefined field of
>> union exp_element. Imagine for instance that you have an expression
>> that looks like this: "foo->bar".
>>
>> At one point, you'll encounter the following elements:
>>
>>   [i  ]  ->  STRUCTOP_PTR
>>   [i+1]  ->  A string
>>   [i+2]  ->  STRUCTOP_PTR
>>
>> Iterating over the expression, you'll ignore the element at index i,
>> and then check the opcode of the element at i+1, which is the wrong
>> field of the enum to access in this case...
>
> I was afraid of that ...

So I just discovered operator_length_standard, which lets me rewrite
this loop and avoid this problem:

  for (i = 0; i < d->exp->nelts; )
    {
      union exp_element *elts = d->exp->elts;
      int oplen, args;
      if (elts[i].opcode == OP_VAR_VALUE)
	{
	  struct block *block = elts[i + 1].block;
	  struct symbol *symbol = elts[i + 2].symbol;
	  struct obj_section *section;

	  gdb_assert (elts[i + 3].opcode == OP_VAR_VALUE);

	  if (block)
	    {
	      const char *const solib_name = solib_address(block->startaddr);
	      if (solib_name && strcmp(solib_name, solib->so_name) == 0)
		return 1;
	    }

	  section = SYMBOL_OBJ_SECTION (symbol);
	  if (section && section->objfile == solib->objfile)
	    return 1;
	}
      operator_length_standard (d->exp, i + 1, &oplen, &args);
      i += oplen;
    }

I think this will correctly iterate over all elements of the
expression, will it not?

Unfortunately, this code still crashes, because no_shared_libraries
first calls objfile_purge_solibs (which indirectly does
obstack_free), and only then clear_solib, which notifies me that
the library has already disappeared. When I proceed to use symbol,
I am already using dangling obstack :-(

Is it ok to move observer notification to before objfile_purge_solibs,
or should I add a new notification? Something like:

  @deftypefun void solib_about_to_be_unloaded (struct so_list *@var{solib})
  The shared library specified by @var{solib} is about to be unloaded.
  @end deftypefun

Thanks,
-- 
Paul Pluzhnikov

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

[Tom Tromey]

>>>>> "Paul" == Paul Pluzhnikov <ppluzhnikov@google.com> writes:

Paul> Unfortunately, this code still crashes, because no_shared_libraries
Paul> first calls objfile_purge_solibs (which indirectly does
Paul> obstack_free), and only then clear_solib, which notifies me that
Paul> the library has already disappeared. When I proceed to use symbol,
Paul> I am already using dangling obstack :-(

Paul> Is it ok to move observer notification to before
Paul> objfile_purge_solibs, or should I add a new notification?

IMO it is ok to move this notification if you audit the existing users
to make sure they don't break.

However, it seems to me that you could also do this another way, by
noting at parse time which objfiles are referenced by a given display,
and then arranging to require a re-parse when an objfile is destroyed.
I think the existing objfile_data machinery could be used for this.

Tom

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

[Paul Pluzhnikov]

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

On Tue, Mar 3, 2009 at 4:48 PM, Tom Tromey <tromey@redhat.com> wrote:

> Paul> Is it ok to move observer notification to before
> Paul> objfile_purge_solibs, or should I add a new notification?
>
> IMO it is ok to move this notification if you audit the existing users
> to make sure they don't break.

I ended up just swapping the order of clear_solib and objfile_purge_solibs
in no_shared_libraries. AFAICT, objfile_purge_solibs is "lower level"
and doesn't need anything from the solib list.

Regtested on Linux/x86_64 with no regressions.

> However, it seems to me that you could also do this another way, by
> noting at parse time which objfiles are referenced by a given display,
> and then arranging to require a re-parse when an objfile is destroyed.
> I think the existing objfile_data machinery could be used for this.

Thanks for the pointer.

I tried that, but it ended up being very tangled and inefficient mess,
and I abandoned this approach.

Thanks,
-- 
Paul Pluzhnikov


ChangeLog:

2009-03-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * printcmd.c (do_one_display): Reparse exp_string.
       (display_uses_solib_p): New function.
       (clear_dangling_display_expressions): New function.
       (_initialize_printcmd): Add observer.
       * solib.c (no_shared_libraries): Swap order of calls to
       clear_solib and objfile_purge_solibs.

testsuite/ChangeLog:

2009-02-04  Paul Pluzhnikov  <ppluzhnikov@google.com>

       * solib-display.exp: New file.
       * solib-display-main.c: New file.
       * solib-display-lib.c: New file.

[-- Attachment #2: gdb-display-crash-20090304-1.txt --]
[-- Type: text/plain, Size: 11084 bytes --]

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.144
diff -u -p -u -r1.144 printcmd.c
--- printcmd.c	25 Feb 2009 18:26:53 -0000	1.144
+++ printcmd.c	4 Mar 2009 19:17:56 -0000
@@ -43,6 +43,11 @@
 #include "disasm.h"
 #include "dfp.h"
 #include "valprint.h"
+#include "exceptions.h"
+#include "observer.h"
+#include "solist.h"
+#include "solib.h"
+#include "parser-defs.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1395,7 +1400,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 +1524,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 +1755,74 @@ disable_display_command (char *args, int
 	  p++;
       }
 }
+
+/* Return 1 if D uses SOLIB (and will become dangling when SOLIB
+   is unloaded), otherwise return 0.  */
+
+static int
+display_uses_solib_p (const struct display *d,
+		      const struct so_list *solib)
+{
+  int i;
+  struct expression *const exp = d->exp;
+
+  if (d->block != NULL &&
+      solib_address (d->block->startaddr) == solib->so_name)
+    return 1;
+
+  for (i = 0; i < exp->nelts; )
+    {
+      int args, oplen = 0;
+      const union exp_element *const elts = exp->elts;
+
+      if (elts[i].opcode == OP_VAR_VALUE)
+	{
+	  const struct block *const block = elts[i + 1].block;
+	  const struct symbol *const symbol = elts[i + 2].symbol;
+	  const struct obj_section *const section =
+	    SYMBOL_OBJ_SECTION (symbol);
+
+	  gdb_assert (elts[i + 3].opcode == OP_VAR_VALUE);
+
+	  if (block != NULL
+	      && solib_address (block->startaddr) == solib->so_name)
+	    return 1;
+
+	  if (section && section->objfile == solib->objfile)
+	    return 1;
+	}
+      exp->language_defn->la_exp_desc->operator_length (exp, i + 1,
+							&oplen, &args);
+      gdb_assert (oplen > 0);
+      i += oplen;
+    }
+  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 fields will be restored next time we need to display
+   an item by re-parsing .exp_string field in the new execution context.  */
+
+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 +2459,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: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.111
diff -u -p -u -r1.111 solib.c
--- solib.c	21 Feb 2009 16:14:49 -0000	1.111
+++ solib.c	4 Mar 2009 19:17:56 -0000
@@ -999,8 +999,13 @@ sharedlibrary_command (char *args, int f
 void
 no_shared_libraries (char *ignored, int from_tty)
 {
-  objfile_purge_solibs ();
+  /* The order of the two routines below is important: clear_solib
+     will notify observers, and at least clear_dangling_display_expressions
+     observer needs access to objfiles associated with solibs being
+     cleared.  */
+
   clear_solib ();
+  objfile_purge_solibs ();
 }
 
 static void
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	4 Mar 2009 19:17:56 -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	4 Mar 2009 19:17:56 -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	4 Mar 2009 19:17:56 -0000
@@ -0,0 +1,115 @@
+# 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] then {
+    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"
+
+if ![runto_main] then {
+    fail "Can't run to main (2)"
+    return 0
+}
+
+gdb_test "stepi" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
+
+# 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"
+}
+
+if ![runto_main] then {
+    fail "Can't run to main (3)"
+    return 0
+}
+
+gdb_test "stepi" "3: c_global = 43\\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"
+
+if ![runto_main] then {
+    fail "Can't run to main (4)"
+    return 0
+}
+
+gdb_test "stepi" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
+gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
+	".*Breakpoint.* at .*" 
+gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
+
+gdb_exit
+
+return 0
+
+

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

[Joel Brobecker]

> 2009-03-04  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>        * printcmd.c (do_one_display): Reparse exp_string.
>        (display_uses_solib_p): New function.
>        (clear_dangling_display_expressions): New function.
>        (_initialize_printcmd): Add observer.
>        * solib.c (no_shared_libraries): Swap order of calls to
>        clear_solib and objfile_purge_solibs.
> 
> testsuite/ChangeLog:
> 
> 2009-02-04  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
>        * solib-display.exp: New file.
>        * solib-display-main.c: New file.
>        * solib-display-lib.c: New file.

All pre-approved after taking the minor comments below into account.

> +  if (d->block != NULL &&
> +      solib_address (d->block->startaddr) == solib->so_name)

Minor style issue: The "&&" should be at the start of the next line.

> +	  gdb_assert (elts[i + 3].opcode == OP_VAR_VALUE);

Is this really necessary? I don't believe we have this sort
of assertion anywhere else. 

> +/* display_chain items point to blocks and expressions. Some expressions in

One space is missing after the period (I feel really sorry about
perstering anyone about this, but this is the GCS)

> Index: solib.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/solib.c,v
> retrieving revision 1.111
> diff -u -p -u -r1.111 solib.c
> --- solib.c	21 Feb 2009 16:14:49 -0000	1.111
> +++ solib.c	4 Mar 2009 19:17:56 -0000
> @@ -999,8 +999,13 @@ sharedlibrary_command (char *args, int f
>  void
>  no_shared_libraries (char *ignored, int from_tty)
>  {
> -  objfile_purge_solibs ();
> +  /* The order of the two routines below is important: clear_solib
> +     will notify observers, and at least clear_dangling_display_expressions
> +     observer needs access to objfiles associated with solibs being
> +     cleared.  */

I'm relunctant to have comments mention the name of a routine as
an example. I think we can get away from it by staying general:

   /* The order of the two routines below is important: clear_solib
      notifies the solib_unloaded observers, and some of these observes
      might need access to their associated objfiles.  Therefore,
      we cannot purge the solibs' objfiles before clear_solib has
      been called.  */

> +if ![runto_main] then {
> +    fail "Can't run to main (2)"
> +    return 0
> +}
> +
> +gdb_test "stepi" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"

I am guessing that the "stepi" test is really there because
you couldn't get the runto_main output, and therefore couldn't
verify it. 

I suggest a different approach:

  | # Start the program, we should land in the program main procedure
  | if { [gdb_start_cmd] < 0 } {
  |     fail "Can't run to main"
  |     return -1
  | }
  | 
  | gdb_test "" \
  |          "first \\(\\) at .*first.adb.*" \
  |          "start first"

The second gdb_test should allow you to verify that the debugger
displays your variables correctly.

What do you think?

-- 
Joel

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

[Paul Pluzhnikov]

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

On Thu, Mar 5, 2009 at 12:04 PM, Joel Brobecker <brobecker@adacore.com> wrote:

>> +  if (d->block != NULL &&
>> +      solib_address (d->block->startaddr) == solib->so_name)
>
> Minor style issue: The "&&" should be at the start of the next line.

Sorry 'bout that.

>
>> +       gdb_assert (elts[i + 3].opcode == OP_VAR_VALUE);
>
> Is this really necessary? I don't believe we have this sort
> of assertion anywhere else.

Ok, removed.

>> +/* display_chain items point to blocks and expressions. Some expressions in
>
> One space is missing after the period (I feel really sorry about
> perstering anyone about this, but this is the GCS)

Sorry about missing this one ...

>> +  /* The order of the two routines below is important: clear_solib
>> +     will notify observers, and at least clear_dangling_display_expressions
>> +     observer needs access to objfiles associated with solibs being
>> +     cleared.  */
>
> I'm relunctant to have comments mention the name of a routine as
> an example. I think we can get away from it by staying general:
>
>   /* The order of the two routines below is important: clear_solib
>      notifies the solib_unloaded observers, and some of these observes
>      might need access to their associated objfiles.  Therefore,
>      we cannot purge the solibs' objfiles before clear_solib has
>      been called.  */

Sounds good, comment updated.

>> +if ![runto_main] then {
>> +    fail "Can't run to main (2)"
>> +    return 0
>> +}
>> +
>> +gdb_test "stepi" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
>
> I am guessing that the "stepi" test is really there because
> you couldn't get the runto_main output, and therefore couldn't
> verify it.

Correct.

> I suggest a different approach:
>
>  | # Start the program, we should land in the program main procedure
>  | if { [gdb_start_cmd] < 0 } {
>  |     fail "Can't run to main"
>  |     return -1
>  | }
>  |
>  | gdb_test "" \
>  |          "first \\(\\) at .*first.adb.*" \
>  |          "start first"
>
> The second gdb_test should allow you to verify that the debugger
> displays your variables correctly.

Looks good.

Attached is the patch I just committed.

Thanks,
-- 
Paul Pluzhnikov

ChangeLog:
2009-03-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* printcmd.c (do_one_display): Reparse exp_string.
	(display_uses_solib_p): New function.
	(clear_dangling_display_expressions): New function.
	(_initialize_printcmd): Add observer.
	* solib.c (no_shared_libraries): Swap order of calls to
	clear_solib and objfile_purge_solibs.
	
testsuite/ChangeLog:
2009-03-05  Paul Pluzhnikov  <ppluzhnikov@google.com>

	* solib-display.exp: New file.
	* solib-display-main.c: New file.
	* solib-display-lib.c: New file.

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

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.144
diff -u -p -u -r1.144 printcmd.c
--- printcmd.c	25 Feb 2009 18:26:53 -0000	1.144
+++ printcmd.c	5 Mar 2009 23:29:35 -0000
@@ -43,6 +43,11 @@
 #include "disasm.h"
 #include "dfp.h"
 #include "valprint.h"
+#include "exceptions.h"
+#include "observer.h"
+#include "solist.h"
+#include "solib.h"
+#include "parser-defs.h"
 
 #ifdef TUI
 #include "tui/tui.h"		/* For tui_active et.al.   */
@@ -1395,7 +1400,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 +1524,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 +1755,72 @@ disable_display_command (char *args, int
 	  p++;
       }
 }
+
+/* Return 1 if D uses SOLIB (and will become dangling when SOLIB
+   is unloaded), otherwise return 0.  */
+
+static int
+display_uses_solib_p (const struct display *d,
+		      const struct so_list *solib)
+{
+  int i;
+  struct expression *const exp = d->exp;
+
+  if (d->block != NULL
+      && solib_address (d->block->startaddr) == solib->so_name)
+    return 1;
+
+  for (i = 0; i < exp->nelts; )
+    {
+      int args, oplen = 0;
+      const union exp_element *const elts = exp->elts;
+
+      if (elts[i].opcode == OP_VAR_VALUE)
+	{
+	  const struct block *const block = elts[i + 1].block;
+	  const struct symbol *const symbol = elts[i + 2].symbol;
+	  const struct obj_section *const section =
+	    SYMBOL_OBJ_SECTION (symbol);
+
+	  if (block != NULL
+	      && solib_address (block->startaddr) == solib->so_name)
+	    return 1;
+
+	  if (section && section->objfile == solib->objfile)
+	    return 1;
+	}
+      exp->language_defn->la_exp_desc->operator_length (exp, i + 1,
+							&oplen, &args);
+      gdb_assert (oplen > 0);
+      i += oplen;
+    }
+  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 fields will be restored next time we need to display
+   an item by re-parsing .exp_string field in the new execution context.  */
+
+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 +2457,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: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.111
diff -u -p -u -r1.111 solib.c
--- solib.c	21 Feb 2009 16:14:49 -0000	1.111
+++ solib.c	5 Mar 2009 23:29:35 -0000
@@ -999,8 +999,13 @@ sharedlibrary_command (char *args, int f
 void
 no_shared_libraries (char *ignored, int from_tty)
 {
-  objfile_purge_solibs ();
+  /* The order of the two routines below is important: clear_solib notifies
+     the solib_unloaded observers, and some of these observers might need
+     access to their associated objfiles.  Therefore, we can not purge the
+     solibs' objfiles before clear_solib has been called.  */
+
   clear_solib ();
+  objfile_purge_solibs ();
 }
 
 static void
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	5 Mar 2009 23:29:36 -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	5 Mar 2009 23:29:36 -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	5 Mar 2009 23:29:36 -0000
@@ -0,0 +1,115 @@
+# 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] then {
+    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"
+
+if { [gdb_start_cmd] < 0 } {
+    fail "Can't run to main (2)"
+    return 0
+}
+
+gdb_test "" "3: c_global = 43\\r\\n2: b_global = 42\\r\\n1: a_global = 41" "after rerun"
+
+# 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"
+}
+
+if { [gdb_start_cmd] < 0 } {
+    fail "Can't run to main (3)"
+    return 0
+}
+
+gdb_test "" "3: c_global = 43\\r\\nwarning: .*b_global.*\\r\\n1: a_global = 41" "after rerun"
+
+# 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"
+
+if { [gdb_start_cmd] < 0 } {
+    fail "Can't run to main (4)"
+    return 0
+}
+
+gdb_test "" "6: a_static = 46\\r\\n4: main_global = 44\\r\\n.*"
+gdb_test "break [gdb_get_line_number "break here" ${testfile}.c]" \
+	".*Breakpoint.* at .*" 
+gdb_test "continue" "6: a_static = 46\\r\\n5: a_local = 45\\r\\n4: main_global = 44\\r\\n.*"
+
+gdb_exit
+
+return 0
+
+

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

[Paul Pluzhnikov]

On Thu, Mar 5, 2009 at 3:46 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> Attached is the patch I just committed.

While back-porting this fix into our GDB-6.8, I discovered that this part:

+	  if (block != NULL
+	      && solib_address (block->startaddr) == solib->so_name)

will never work, because in clear_solib:

      so_list_head = so->next;
      observer_notify_solib_unloaded (so);

and solib_address uses so_list_head to search for matching address.
[Aren't ordering issues fun :-[

So I reversed the order:

      observer_notify_solib_unloaded (so);
      so_list_head = so->next;

only to discover that this causes

  FAIL: gdb.base/attach.exp: attach1 detach

because:

  warning: Temporarily disabling breakpoints for unloaded shared
library "/lib64/ld-linux-x86-64.so.2"

Why wasn't this breakpoint shlib_disabled in disable_breakpoints_in_shlibs()
just a couple of lines above?

Because it's of the wrong type (bp_shlib_event), and
disable_breakpoints_in_shlibs() only disables bp_breakpoint and
bp_hardware_breakpoint.

I am not sure whether:
A) bp_shlib_event type breakpoints should also be shlib_disable'd, or
B) disable_breakpoints_in_unloaded_shlib() should only warn about
   bp_breakpoint and bp_hardware_breakpoint type, or
C) the call to "solib_address (block->startaddr)" should be replaced with
   something like "solib_contains_p (solib, block->startaddr)" (which would
   then work independently of the so_list_head).

Advice?

(C) is the least disruptive; (A) appears to be logical (but I don't know
whether it will break anything); (B) looks like a hack to me.

Thanks,
-- 
Paul Pluzhnikov

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

[Paul Pluzhnikov]

On Thu, Mar 5, 2009 at 7:06 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:

> I am not sure whether:
> A) bp_shlib_event type breakpoints should also be shlib_disable'd, or
> B) disable_breakpoints_in_unloaded_shlib() should only warn about
>   bp_breakpoint and bp_hardware_breakpoint type, or
> C) the call to "solib_address (block->startaddr)" should be replaced with
>   something like "solib_contains_p (solib, block->startaddr)" (which would
>   then work independently of the so_list_head).

Or (D) remove (currently useless) block->startaddr checks, and rely solely on
symbol->section->objfile == solib->objfile check.

Thanks,
-- 
Paul Pluzhnikov

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

[Joel Brobecker]

> A) bp_shlib_event type breakpoints should also be shlib_disable'd, or

For this option, I'm not sure off the top of my head. I think it would
require careful analysis. If we were going this route, I would
definitely not approve the change just on my own (I'd ask for confirmation
from my fellow maintainers).

Let put this aside for now...

> C) the call to "solib_address (block->startaddr)" should be replaced with
>    something like "solib_contains_p (solib, block->startaddr)" (which would
>    then work independently of the so_list_head).

Duh! Yes - this sounds like a pretty simple way to do this. We know
which solib we're trying to match our expression against, why are we
iterating over all SOs again? I like your suggestion.

The body of solib_contains_p (can we rename it to "solib_contains_address"
or "solib_has_address") can be extracted from solib_address.

(On a side note - I think that "solib_address" is a bad name.
Independently of this change, we should think about changing it
to "solib_name_from_address" one of these days).

-- 
Joel

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

[Paul Pluzhnikov]

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

On Fri, Mar 6, 2009 at 9:48 AM, Joel Brobecker <brobecker@adacore.com> wrote:

> (On a side note - I think that "solib_address" is a bad name.
> Independently of this change, we should think about changing it
> to "solib_name_from_address" one of these days).

Attached patch does that.

I didn't touch similarly mis-named xcoff_solib_address, because I can't
see any use of that symbol. Perhaps it should be deleted?

If it's a debug-only helper, I can rename it and add a comment to that
effect.

Thanks,
-- 
Paul Pluzhnikov

2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

	    Rename solib_address to solib_name_from_address.
	    * breakpoint.c (insert_bp_location, disable_breakpoints_in_shlibs)
	    (disable_breakpoints_in_unloaded_shlib): Update.
	    * printcmd.c (display_uses_solib_p): Likewise.
	    * stack.c (print_frame): Likewise.
	    * solib.c: Rename.
	    * solib.h: Rename.

[-- Attachment #2: gdb-rename-solib-20090306.txt --]
[-- Type: text/plain, Size: 4358 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.381
diff -u -p -u -r1.381 breakpoint.c
--- breakpoint.c	5 Mar 2009 22:37:10 -0000	1.381
+++ breakpoint.c	6 Mar 2009 18:23:46 -0000
@@ -1160,7 +1160,7 @@ Note: automatically using hardware break
       if (val)
 	{
 	  /* Can't set the breakpoint.  */
-	  if (solib_address (bpt->address))
+	  if (solib_name_from_address (bpt->address))
 	    {
 	      /* See also: disable_breakpoints_in_shlibs. */
 	      val = 0;
@@ -1629,7 +1629,7 @@ remove_breakpoint (struct bp_location *b
       /* In some cases, we might not be able to remove a breakpoint
 	 in a shared library that has already been removed, but we
 	 have not yet processed the shlib unload event.  */
-      if (val && solib_address (b->address))
+      if (val && solib_name_from_address (b->address))
 	val = 0;
 
       if (val)
@@ -4439,7 +4439,7 @@ disable_breakpoints_in_shlibs (void)
 #ifdef PC_SOLIB
 	&& PC_SOLIB (loc->address)
 #else
-	&& solib_address (loc->address)
+	&& solib_name_from_address (loc->address)
 #endif
 	)
       {
@@ -4475,7 +4475,7 @@ disable_breakpoints_in_unloaded_shlib (s
 #ifdef PC_SOLIB
 	char *so_name = PC_SOLIB (loc->address);
 #else
-	char *so_name = solib_address (loc->address);
+	char *so_name = solib_name_from_address (loc->address);
 #endif
 	if (so_name && !strcmp (so_name, solib->so_name))
           {
Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.145
diff -u -p -u -r1.145 printcmd.c
--- printcmd.c	5 Mar 2009 23:45:14 -0000	1.145
+++ printcmd.c	6 Mar 2009 18:23:46 -0000
@@ -1767,7 +1767,7 @@ display_uses_solib_p (const struct displ
   struct expression *const exp = d->exp;
 
   if (d->block != NULL
-      && solib_address (d->block->startaddr) == solib->so_name)
+      && solib_name_from_address (d->block->startaddr) == solib->so_name)
     return 1;
 
   for (i = 0; i < exp->nelts; )
@@ -1783,7 +1783,7 @@ display_uses_solib_p (const struct displ
 	    SYMBOL_OBJ_SECTION (symbol);
 
 	  if (block != NULL
-	      && solib_address (block->startaddr) == solib->so_name)
+	      && solib_name_from_address (block->startaddr) == solib->so_name)
 	    return 1;
 
 	  if (section && section->objfile == solib->objfile)
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.112
diff -u -p -u -r1.112 solib.c
--- solib.c	5 Mar 2009 23:45:14 -0000	1.112
+++ solib.c	6 Mar 2009 18:23:46 -0000
@@ -838,11 +838,12 @@ info_sharedlibrary_command (char *ignore
 
    GLOBAL FUNCTION
 
-   solib_address -- check to see if an address is in a shared lib
+   solib_name_from_address -- if an address is in a shared lib, return
+   its name.
 
    SYNOPSIS
 
-   char * solib_address (CORE_ADDR address)
+   char * solib_name_from_address (CORE_ADDR address)
 
    DESCRIPTION
 
@@ -856,7 +857,7 @@ info_sharedlibrary_command (char *ignore
  */
 
 char *
-solib_address (CORE_ADDR address)
+solib_name_from_address (CORE_ADDR address)
 {
   struct so_list *so = 0;	/* link map state variable */
 
Index: solib.h
===================================================================
RCS file: /cvs/src/src/gdb/solib.h,v
retrieving revision 1.21
diff -u -p -u -r1.21 solib.h
--- solib.h	3 Jan 2009 05:57:53 -0000	1.21
+++ solib.h	6 Mar 2009 18:23:46 -0000
@@ -45,7 +45,7 @@ extern void solib_create_inferior_hook (
 
 /* If ADDR lies in a shared library, return its name.  */
 
-extern char *solib_address (CORE_ADDR);
+extern char *solib_name_from_address (CORE_ADDR);
 
 /* Return 1 if PC lies in the dynamic symbol resolution code of the
    run time loader.  */
Index: stack.c
===================================================================
RCS file: /cvs/src/src/gdb/stack.c,v
retrieving revision 1.185
diff -u -p -u -r1.185 stack.c
--- stack.c	11 Feb 2009 16:07:28 -0000	1.185
+++ stack.c	6 Mar 2009 18:23:46 -0000
@@ -730,7 +730,7 @@ print_frame (struct frame_info *frame, i
 #ifdef PC_SOLIB
       char *lib = PC_SOLIB (get_frame_pc (frame));
 #else
-      char *lib = solib_address (get_frame_pc (frame));
+      char *lib = solib_name_from_address (get_frame_pc (frame));
 #endif
       if (lib)
 	{

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

[Joel Brobecker]

> Attached patch does that.

Thanks!

> I didn't touch similarly mis-named xcoff_solib_address, because I can't
> see any use of that symbol. Perhaps it should be deleted?

This function is actually used as a #define in config/rs6000/nm-rs6000.h:

    #define     PC_SOLIB(PC)    xcoff_solib_address(PC)

I'll change its name when I have a moment. But perhaps the whole
file could be modernized too. So let's leave it aside for now.

> 2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	    Rename solib_address to solib_name_from_address.
> 	    * breakpoint.c (insert_bp_location, disable_breakpoints_in_shlibs)
> 	    (disable_breakpoints_in_unloaded_shlib): Update.
> 	    * printcmd.c (display_uses_solib_p): Likewise.
> 	    * stack.c (print_frame): Likewise.
> 	    * solib.c: Rename.
> 	    * solib.h: Rename.

Looks great. Please apply.

And thank you for taking care of this.

-- 
Joel

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

[Paul Pluzhnikov]

On Fri, Mar 6, 2009 at 10:46 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>> I didn't touch similarly mis-named xcoff_solib_address, because I can't
>> see any use of that symbol. Perhaps it should be deleted?
>
> This function is actually used as a #define in config/rs6000/nm-rs6000.h:
>
>    #define     PC_SOLIB(PC)    xcoff_solib_address(PC)

Ah, I should have used 'grep -r' ...

> Looks great. Please apply.

Committed.

Thanks,
-- 
Paul Pluzhnikov

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

[Paul Pluzhnikov]

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

On Fri, Mar 6, 2009 at 9:48 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>> C) the call to "solib_address (block->startaddr)" should be replaced with
>>    something like "solib_contains_p (solib, block->startaddr)" (which would
>>    then work independently of the so_list_head).
>
> Duh! Yes - this sounds like a pretty simple way to do this. We know
> which solib we're trying to match our expression against, why are we
> iterating over all SOs again? I like your suggestion.
>
> The body of solib_contains_p (can we rename it to "solib_contains_address"
> or "solib_has_address") can be extracted from solib_address.

Attached patch implements this; regtested on Linux/x86_64 with
no regressions.

Thanks,
-- 
Paul Pluzhnikov

2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>

	    * solib.c (solib_contains_address_p): New function.
	    (solib_name_from_address): Use it.
	    * printcmd.c (display_uses_solib_p): Use it.
	    * solib.h (solib_contains_address_p): Declare it.

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

Index: printcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/printcmd.c,v
retrieving revision 1.146
diff -u -p -u -r1.146 printcmd.c
--- printcmd.c	6 Mar 2009 18:51:05 -0000	1.146
+++ printcmd.c	6 Mar 2009 20:32:59 -0000
@@ -1767,7 +1767,7 @@ display_uses_solib_p (const struct displ
   struct expression *const exp = d->exp;
 
   if (d->block != NULL
-      && solib_name_from_address (d->block->startaddr) == solib->so_name)
+      && solib_contains_address_p (solib, d->block->startaddr))
     return 1;
 
   for (i = 0; i < exp->nelts; )
@@ -1783,7 +1783,7 @@ display_uses_solib_p (const struct displ
 	    SYMBOL_OBJ_SECTION (symbol);
 
 	  if (block != NULL
-	      && solib_name_from_address (block->startaddr) == solib->so_name)
+	      && solib_contains_address_p (solib, block->startaddr))
 	    return 1;
 
 	  if (section && section->objfile == solib->objfile)
Index: solib.c
===================================================================
RCS file: /cvs/src/src/gdb/solib.c,v
retrieving revision 1.113
diff -u -p -u -r1.113 solib.c
--- solib.c	6 Mar 2009 18:51:05 -0000	1.113
+++ solib.c	6 Mar 2009 20:32:59 -0000
@@ -834,6 +834,21 @@ info_sharedlibrary_command (char *ignore
     }
 }
 
+/* Return 1 if ADDRESS lies within SOLIB.  */
+
+int
+solib_contains_address_p (const struct so_list *const solib,
+			  CORE_ADDR address)
+{
+  struct section_table *p;
+
+  for (p = solib->sections; p < solib->sections_end; p++)
+    if (p->addr <= address && address < p->endaddr)
+      return 1;
+
+  return 0;
+}
+
 /*
 
    GLOBAL FUNCTION
@@ -862,15 +877,8 @@ solib_name_from_address (CORE_ADDR addre
   struct so_list *so = 0;	/* link map state variable */
 
   for (so = so_list_head; so; so = so->next)
-    {
-      struct section_table *p;
-
-      for (p = so->sections; p < so->sections_end; p++)
-	{
-	  if (p->addr <= address && address < p->endaddr)
-	    return (so->so_name);
-	}
-    }
+    if (solib_contains_address_p (so, address))
+      return (so->so_name);
 
   return (0);
 }
Index: solib.h
===================================================================
RCS file: /cvs/src/src/gdb/solib.h,v
retrieving revision 1.22
diff -u -p -u -r1.22 solib.h
--- solib.h	6 Mar 2009 18:51:05 -0000	1.22
+++ solib.h	6 Mar 2009 20:32:59 -0000
@@ -47,6 +47,10 @@ extern void solib_create_inferior_hook (
 
 extern char *solib_name_from_address (CORE_ADDR);
 
+/* Return 1 if ADDR lies within SOLIB.  */
+
+extern int solib_contains_address_p (const struct so_list *, CORE_ADDR);
+
 /* Return 1 if PC lies in the dynamic symbol resolution code of the
    run time loader.  */
 

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

[Joel Brobecker]

> 2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	    * solib.c (solib_contains_address_p): New function.
> 	    (solib_name_from_address): Use it.
> 	    * printcmd.c (display_uses_solib_p): Use it.
> 	    * solib.h (solib_contains_address_p): Declare it.

Looks great to me. Go ahead and check it in.

Thanks,
-- 
Joel

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

[Paul Pluzhnikov]

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

On Mon, Mar 9, 2009 at 11:33 AM, Joel Brobecker <brobecker@adacore.com> wrote:

>> 2009-03-06  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>           * solib.c (solib_contains_address_p): New function.
>
> Looks great to me. Go ahead and check it in.

Thanks, so committed.

And now we come back to the shlib_disable question.

With the change above, I believe it to be quite reasonable to update
breakpoint.c as attached patch does.

But doing that breaks attach.exp again:
  FAIL: gdb.base/attach.exp: attach1 detach
due to:
  warning: Temporarily disabling breakpoints for unloaded shared
library "/lib64/ld-linux-x86-64.so.2"

With current code we don't issue the warning, but only because
ld-linux has already been removed from the so_list_head, and so
solib_name_from_address() returns NULL.

It looks to me that this works purely by accident, and so I ask
again: shouldn't all types of breakpoints, and not just
bp_breakpoint and bp_hardware_breakpoint, be shlib_disabled in
disable_breakpoints_in_shlibs?

I went ahead and did this:

diff -u -p -u -r1.382 breakpoint.c
--- breakpoint.c        6 Mar 2009 18:51:05 -0000       1.382
+++ breakpoint.c        9 Mar 2009 22:55:02 -0000
@@ -4434,8 +4434,7 @@ disable_breakpoints_in_shlibs (void)
        becomes enabled, or the duplicate is removed, gdb will try to insert
        all breakpoints.  If we don't set shlib_disabled here, we'll try
        to insert those breakpoints and fail.  */
-    if (((b->type == bp_breakpoint) || (b->type == bp_hardware_breakpoint))
-       && !loc->shlib_disabled
+    if (!loc->shlib_disabled
 #ifdef PC_SOLIB
        && PC_SOLIB (loc->address)
 #else

And that did not produce any new failures on Linux/x86_64.

It appears to me that this is the logically correct thing to do.

Thanks,
-- 
Paul Pluzhnikov

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

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.382
diff -u -p -u -r1.382 breakpoint.c
--- breakpoint.c	6 Mar 2009 18:51:05 -0000	1.382
+++ breakpoint.c	9 Mar 2009 22:51:45 -0000
@@ -4470,28 +4470,21 @@ disable_breakpoints_in_unloaded_shlib (s
     struct breakpoint *b = loc->owner;
     if ((loc->loc_type == bp_loc_hardware_breakpoint
 	 || loc->loc_type == bp_loc_software_breakpoint)
-	&& !loc->shlib_disabled)
+	&& !loc->shlib_disabled
+	&& solib_contains_address_p (solib, loc->address))
       {
-#ifdef PC_SOLIB
-	char *so_name = PC_SOLIB (loc->address);
-#else
-	char *so_name = solib_name_from_address (loc->address);
-#endif
-	if (so_name && !strcmp (so_name, solib->so_name))
-          {
-	    loc->shlib_disabled = 1;
-	    /* At this point, we cannot rely on remove_breakpoint
-	       succeeding so we must mark the breakpoint as not inserted
-	       to prevent future errors occurring in remove_breakpoints.  */
-	    loc->inserted = 0;
-	    if (!disabled_shlib_breaks)
-	      {
-		target_terminal_ours_for_output ();
-		warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
-			  so_name);
-	      }
-	    disabled_shlib_breaks = 1;
+	loc->shlib_disabled = 1;
+	/* At this point, we cannot rely on remove_breakpoint
+	   succeeding so we must mark the breakpoint as not inserted
+	   to prevent future errors occurring in remove_breakpoints.  */
+	loc->inserted = 0;
+	if (!disabled_shlib_breaks)
+	  {
+	    target_terminal_ours_for_output ();
+	    warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
+		     solib->so_name);
 	  }
+	disabled_shlib_breaks = 1;
       }
   }
 }

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

[Daniel Jacobowitz]

On Mon, Mar 09, 2009 at 04:24:48PM -0700, Paul Pluzhnikov wrote:
> It looks to me that this works purely by accident, and so I ask
> again: shouldn't all types of breakpoints, and not just
> bp_breakpoint and bp_hardware_breakpoint, be shlib_disabled in
> disable_breakpoints_in_shlibs?

Disabling a shlib_event breakpoint is pretty weird - there won't be an
event to restore it, so unless we're restarting the program it won't
be enabled again ever.  If we're disabling a step-return or finish
breakpoint, we'll lose control of the inferior.  And so forth...

> I went ahead and did this:
> 
> diff -u -p -u -r1.382 breakpoint.c
> --- breakpoint.c        6 Mar 2009 18:51:05 -0000       1.382
> +++ breakpoint.c        9 Mar 2009 22:55:02 -0000
> @@ -4434,8 +4434,7 @@ disable_breakpoints_in_shlibs (void)
>         becomes enabled, or the duplicate is removed, gdb will try to insert
>         all breakpoints.  If we don't set shlib_disabled here, we'll try
>         to insert those breakpoints and fail.  */
> -    if (((b->type == bp_breakpoint) || (b->type == bp_hardware_breakpoint))
> -       && !loc->shlib_disabled
> +    if (!loc->shlib_disabled
>  #ifdef PC_SOLIB
>         && PC_SOLIB (loc->address)
>  #else
> 
> And that did not produce any new failures on Linux/x86_64.

It needs at least breakpoint_address_is_meaningful.

-- 
Daniel Jacobowitz
CodeSourcery

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

[Paul Pluzhnikov]

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

On Tue, Mar 10, 2009 at 6:37 AM, Daniel Jacobowitz <drow@false.org> wrote:

> Disabling a shlib_event breakpoint is pretty weird - there won't be an
> event to restore it, so unless we're restarting the program it won't
> be enabled again ever.  If we're disabling a step-return or finish
> breakpoint, we'll lose control of the inferior.  And so forth...

Thanks, I think I understood how this works now.

Attached patch is simply a cleanup (i.e. I dont't really care if
it is applied or not, though I think it is more efficient and
easier to understand the source). It introduces no regressions
of Linux/x86_64.

OK to commit?
-- 
Paul Pluzhnikov


2009-03-11  Paul Pluzhnikov  <ppluzhnikov@google.com>

	    * breakpoint.c (disable_breakpoints_in_shlibs): Use
	    solib_contains_address_p instead of searching.

[-- Attachment #2: gdb-rename-solib-20090309.txt --]
[-- Type: text/plain, Size: 2204 bytes --]

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.383
diff -u -p -u -r1.383 breakpoint.c
--- breakpoint.c	11 Mar 2009 20:26:02 -0000	1.383
+++ breakpoint.c	12 Mar 2009 01:04:34 -0000
@@ -4442,7 +4442,7 @@ disable_breakpoints_in_shlibs (void)
        all breakpoints.  If we don't set shlib_disabled here, we'll try
        to insert those breakpoints and fail.  */
     if (((b->type == bp_breakpoint) || (b->type == bp_hardware_breakpoint))
-	&& !loc->shlib_disabled
+ 	&& !loc->shlib_disabled
 #ifdef PC_SOLIB
 	&& PC_SOLIB (loc->address)
 #else
@@ -4477,28 +4477,22 @@ disable_breakpoints_in_unloaded_shlib (s
     struct breakpoint *b = loc->owner;
     if ((loc->loc_type == bp_loc_hardware_breakpoint
 	 || loc->loc_type == bp_loc_software_breakpoint)
-	&& !loc->shlib_disabled)
+	&& !loc->shlib_disabled
+	&& (b->type == bp_breakpoint || b->type == bp_hardware_breakpoint)
+	&& solib_contains_address_p (solib, loc->address))
       {
-#ifdef PC_SOLIB
-	char *so_name = PC_SOLIB (loc->address);
-#else
-	char *so_name = solib_name_from_address (loc->address);
-#endif
-	if (so_name && !strcmp (so_name, solib->so_name))
-          {
-	    loc->shlib_disabled = 1;
-	    /* At this point, we cannot rely on remove_breakpoint
-	       succeeding so we must mark the breakpoint as not inserted
-	       to prevent future errors occurring in remove_breakpoints.  */
-	    loc->inserted = 0;
-	    if (!disabled_shlib_breaks)
-	      {
-		target_terminal_ours_for_output ();
-		warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
-			  so_name);
-	      }
-	    disabled_shlib_breaks = 1;
+	loc->shlib_disabled = 1;
+	/* At this point, we cannot rely on remove_breakpoint
+	   succeeding so we must mark the breakpoint as not inserted
+	   to prevent future errors occurring in remove_breakpoints.  */
+	loc->inserted = 0;
+	if (!disabled_shlib_breaks)
+	  {
+	    target_terminal_ours_for_output ();
+	    warning (_("Temporarily disabling breakpoints for unloaded shared library \"%s\""),
+		     solib->so_name);
 	  }
+	disabled_shlib_breaks = 1;
       }
   }
 }

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

[Pedro Alves]

Hi guys,

On Thursday 05 March 2009 23:46:32, Paul Pluzhnikov wrote:
> > I suggest a different approach:
> >
> >  | # Start the program, we should land in the program main procedure
> >  | if { [gdb_start_cmd] < 0 } {
> >  |     fail "Can't run to main"
> >  |     return -1
> >  | }
> >  |
> >  | gdb_test "" \
> >  |          "first \\(\\) at .*first.adb.*" \
> >  |          "start first"
> >
> > The second gdb_test should allow you to verify that the debugger
> > displays your variables correctly.
> 
> Looks good.
> 
> Attached is the patch I just committed.

I just noticed that this test is failing against gdbserver:

  FAIL: gdb.base/solib-display.exp: Can't run to main (2)

The problem is that gdb_start_cmd is a nop for remote targets:

proc gdb_start_cmd {args} {
...
    if [target_info exists use_gdb_stub] {
        return -1
    }

What do you think?  Should we skip this test for remote
targets, or perhaps we do things differently here?

-- 
Pedro Alves

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

[Joel Brobecker]

> The problem is that gdb_start_cmd is a nop for remote targets:
[...]
> What do you think?  Should we skip this test for remote
> targets, or perhaps we do things differently here?

If I understand the original issue correctly, we need to restart
the execution of our program in order to demonstrate the issue.
Except with the extended-remote protocol, we can't do that when
using the gdbserver, right? In other words, the "run" command with
target remote doesn't restart the program like in the native case.
So I'd say we kill the test for remote targets.

-- 
Joel

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

[Paul Pluzhnikov]

On Tue, Mar 17, 2009 at 8:15 PM, Joel Brobecker <brobecker@adacore.com> wrote:

> If I understand the original issue correctly, we need to restart
> the execution of our program in order to demonstrate the issue.

Well, not really. You could also
  dlopen foo.so
  break in foo.so
  display something referring to foo.so symbols
  dlclose foo.so
  re-display

But writing a portable test for that is harder.

> Except with the extended-remote protocol, we can't do that when
> using the gdbserver, right? In other words, the "run" command with
> target remote doesn't restart the program like in the native case.
> So I'd say we kill the test for remote targets.

Is it as simple as removing changing fail to untested:

if { [gdb_start_cmd] < 0 } {
-    fail "Can't run to main (2)"
+    untested "Can't run to main (2)"
    return 0
}

Of course we might just as well skip the test, since it's not going
to test much in that case.


-- 
Paul Pluzhnikov

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

[Pedro Alves]

On Wednesday 18 March 2009 03:15:16, Joel Brobecker wrote:
> If I understand the original issue correctly, we need to restart
> the execution of our program in order to demonstrate the issue.
> Except with the extended-remote protocol, we can't do that when
> using the gdbserver, right? In other words, the "run" command with
> target remote doesn't restart the program like in the native case.
> So I'd say we kill the test for remote targets.

Right, target remote doesn't support "run" at all:

  (gdb) run
  The "remote" target does not support "run".  Try "help target" or "continue".

You'd have to disconnect/kill, and then reconnect perhaps.  Maybe
using rerun_to_main.

On Wednesday 18 March 2009 03:24:04, Paul Pluzhnikov wrote:
> On Tue, Mar 17, 2009 at 8:15 PM, Joel Brobecker <brobecker@adacore.com> wrote:
> 
> > If I understand the original issue correctly, we need to restart
> > the execution of our program in order to demonstrate the issue.
> 
> Well, not really. You could also
>   dlopen foo.so
>   break in foo.so
>   display something referring to foo.so symbols
>   dlclose foo.so
>   re-display
> 
> But writing a portable test for that is harder.

Or that.  It may not be so hard for most targets we care
about.  See unload.exp or watchpoint-solib.exp.

> Is it as simple as removing changing fail to untested:
> 
> if { [gdb_start_cmd] < 0 } {
> -    fail "Can't run to main (2)"
> +    untested "Can't run to main (2)"
>     return 0
> }

Or not that.  :-)

> 
> Of course we might just as well skip the test, since it's not going
> to test much in that case.

Or that is fine with me too.

-- 
Pedro Alves

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

[Paul Pluzhnikov]

On Tue, Mar 17, 2009 at 9:12 PM, Pedro Alves <pedro@codesourcery.com> wrote:

>> Of course we might just as well skip the test, since it's not going
>> to test much in that case.
>
> Or that is fine with me too.

I just committed this:

Index: gdb.base/solib-display.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/solib-display.exp,v
retrieving revision 1.1
diff -u -p -u -r1.1 solib-display.exp
--- gdb.base/solib-display.exp  5 Mar 2009 23:45:14 -0000       1.1
+++ gdb.base/solib-display.exp  18 Mar 2009 05:41:35 -0000
@@ -28,7 +28,7 @@
 # (and thus aren't affected by shared library unloading) are not
 # disabled prematurely.

-if [skip_shlib_tests] then {
+if { [skip_shlib_tests] || [is_remote target] } {
     return 0
 }




-- 
Paul Pluzhnikov

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

[Pedro Alves]

On Wednesday 18 March 2009 05:45:38, Paul Pluzhnikov wrote:
> I just committed this:

Thanks!

-- 
Pedro Alves

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

[Joel Brobecker]

> 2009-03-11  Paul Pluzhnikov  <ppluzhnikov@google.com>
> 
> 	    * breakpoint.c (disable_breakpoints_in_shlibs): Use
> 	    solib_contains_address_p instead of searching.

Unfortunately, we can't apply this patch just yet, because of:

> -#ifdef PC_SOLIB
> -	char *so_name = PC_SOLIB (loc->address);
> -#else

There is still one architecture that uses PC_SOLIB (ppc-aix) :-(.
I wonder if I might be able to work on this sometime soon. There
are a whole bunch of macros that we could get rid of on AIX.

Also, I have a couple of questions:

> -	&& !loc->shlib_disabled
> + 	&& !loc->shlib_disabled

I can't figure out what the change was in this case. The lines
look completely identical. I suspect a change in white-spaces,
but I couldn't see any.

> -	&& !loc->shlib_disabled)
> +	&& !loc->shlib_disabled
> +	&& (b->type == bp_breakpoint || b->type == bp_hardware_breakpoint)
> +	&& solib_contains_address_p (solib, loc->address))

I am wondering why you are checking the breakpoint type in addition
to the location type. In particular, I'm trying to figure out whether
it's possible to have a b->type that's not a breakpoint if loc->type
is a breakpoint.  Also, we weren't making that check before, so what
did you see that made it you do it now?

-- 
Joel

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

[Paul Pluzhnikov]

On Fri, Mar 20, 2009 at 12:54 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> 2009-03-11  Paul Pluzhnikov  <ppluzhnikov@google.com>
>>
>>           * breakpoint.c (disable_breakpoints_in_shlibs): Use
>>           solib_contains_address_p instead of searching.
>
> Unfortunately, we can't apply this patch just yet, because of:
>
>> -#ifdef PC_SOLIB
>> -     char *so_name = PC_SOLIB (loc->address);
>> -#else

But I think we can. After the patch, we don't care about so_name, except
to print the warning, and we have it readily available in solib->so_name.

> Also, I have a couple of questions:
>
>> -     && !loc->shlib_disabled
>> +     && !loc->shlib_disabled
>
> I can't figure out what the change was in this case. The lines
> look completely identical. I suspect a change in white-spaces,
> but I couldn't see any.

There is a white-space diff, and a bogus one at that:
"cat -T gdb-rename-solib-20090309.txt" shows:

-^I&& !loc->shlib_disabled
+ ^I&& !loc->shlib_disabled

Sorry about that.

>> -     && !loc->shlib_disabled)
>> +     && !loc->shlib_disabled
>> +     && (b->type == bp_breakpoint || b->type == bp_hardware_breakpoint)
>> +     && solib_contains_address_p (solib, loc->address))
>
> I am wondering why you are checking the breakpoint type in addition
> to the location type.

Because if I don't, then a warning is issued when b->type == bp_shlib_event
and ld-linux-x86-64.so.2 is unloaded.

> In particular, I'm trying to figure out whether
> it's possible to have a b->type that's not a breakpoint if loc->type
> is a breakpoint.

Yes, for b->type == bp_shlib_event.

> Also, we weren't making that check before, so what
> did you see that made it you do it now?

That is exactly what I am trying to say: the current code works (as in,
does not issue a warning) "by accident": we remove ld-linux... from the
so_list_head, *then* run observer notification. Observer searches the list
for "so" containing the bp_shlib_event, and doesn't find it (we just removed
it from there!) and so remains silent. Reverse these lines in clear_solib:

     observer_notify_solib_unloaded (so);
     so_list_head = so->next;

and current code will start issuing the warning as well.

-- 
Paul Pluzhnikov

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

[Joel Brobecker]

> But I think we can. After the patch, we don't care about so_name, except
> to print the warning, and we have it readily available in solib->so_name.

After looking deeper into this, I think you are right. But maybe not
for the reason you thought. As far as I can tell, the current solib
mechanism used on AIX remains painfully disconnected to the standard
solib mechanism, and so I'm guessing that the solib list remains
empty.

The function where you're removing the PC_SOLIB is only called as
an observer of the solib_unloaded event. This event is only triggered
from solib.c in clear_solib and update_solib_list. Either way, if
the solib list remains empty as I think it does, then the observer
is never called on AIX. And so the PC_SOLIB thing can indeed be
removed.

(in fact, I just verified this by debugging GDB)

> > In particular, I'm trying to figure out whether
> > it's possible to have a b->type that's not a breakpoint if loc->type
> > is a breakpoint.
> 
> Yes, for b->type == bp_shlib_event.

Cool. I was certain that you hadn't added this condition just "because" :-).

> it from there!) and so remains silent. Reverse these lines in clear_solib:
> 
>      observer_notify_solib_unloaded (so);
>      so_list_head = so->next;
> 
> and current code will start issuing the warning as well.

Thanks for the explanation.

The patch looks good to me, now. Please go ahead and check it in.
But be careful with the ChangeLog, as the one you sent was incomplete.

-- 
Joel