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

Paul Pluzhnikov
2009-02-07T02:37:20Z

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