This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [patch] Fix a crash when displaying variables from shared library.
- From: Tom Tromey <tromey at redhat dot com>
- To: ppluzhnikov at google dot com (Paul Pluzhnikov)
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 06 Feb 2009 14:36:14 -0700
- Subject: Re: [patch] Fix a crash when displaying variables from shared library.
- References: <20090205030257.8A6073A6B7A@localhost>
- Reply-to: tromey at redhat dot com
>>>>> "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