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

[Tom Tromey <tromey@redhat.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