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

[Paul Pluzhnikov <ppluzhnikov at google dot com>]

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: gdb-display-crash-20090206.txt
Description: Text document