[review] Fix crash with core + TUI + run

Sergio Durigan Junior (Code Review)
2019-11-08T21:26:20Z

Sergio Durigan Junior has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/483
......................................................................

Patch Set 1:

(3 comments)

On Tuesday, November 05 2019, Pedro Alves wrote:

> --- /dev/null
> +++ /COMMIT_MSG
> @@ -1,0 +1,16 @@
> +Parent: e48f6033 (Move check for strerror_r to common.m4 where it belongs)
> +Author: Sergio Durigan Junior <sergiodj@redhat.com>
> +AuthorDate: 2019-10-30 13:58:29 -0400
> +Commit: Sergio Durigan Junior <sergiodj@redhat.com>
> +CommitDate: 2019-11-01 11:53:06 -0400
> +
> +Make sure we have a valid target on top when pushing a new target

PS1, Line 7:

Thanks for the review. I changed the subject as suggested.

> +
> +Ref.: https://bugzilla.redhat.com/show_bug.cgi?id=1765117
> +
> +A segfault can happen in a specific scenario when using TUI + a
> +corefile, as explained in the bug mentioned above. The problem
> +happens when opening a corefile on GDB:
> +
> + $ gdb ./core program
> +
> --- gdb/target.c
> +++ gdb/target.c
> @@ -569,14 +566,18 @@ target_stack::push (target_ops *t)
> - }
> + prev = m_stack[stratum];
>
> /* Now add the new one. */
> m_stack[stratum] = t;
>
> + /* And close the previous one, if it exists. */
> + if (prev != nullptr)
> + target_close (prev);
> +

PS1, Line 574:

>> /* Finally close the target. Note we do this after unchaining, so
>> any target method calls from within the target_close
>> implementation don't end up in T anymore. */
>> target_close (t);

>> /* Unchain the target. */
>> m_stack[stratum] = NULL;
>>
>> if (m_top == stratum)
>> m_top = t->beneath ()->stratum ();

>>
>> /* Finally close the target. Note we do this after unchaining, so
>> any target method calls from within the target_close
>> implementation don't end up in T anymore. */
>> target_close (t);

Interesting. While hacking, I wasn't sure if I should call 'unpush'
directly (even wrote about that in the commit message), but in the end
decided to just reorder the call to 'target_close'. It's interesting
that I read the comment on 'unpush' regarding calling 'target_close'
after unchaining... Anyway, thanks for pointing this out. I updated
the patch.

> if (m_top < stratum)
> m_top = stratum;
> }
>
> /* See target.h. */
>
> void
> push_target (struct target_ops *t)
> {
> --- /dev/null
> +++ gdb/testsuite/gdb.tui/segfault-corefile-run.exp
> @@ -1,0 +1,10 @@
> +# Copyright 2019 Free Software Foundation, Inc.

PS1, Line 1:

Fair enough; renamed. Thanks.

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

--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I39e2f8b538c580c8ea5bf1d657ee877e47746c8f
Gerrit-Change-Number: 483
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Reviewer: Pedro Alves <palves@redhat.com>
Gerrit-Reviewer: Sergio Durigan Junior <sergiodj@redhat.com>
Gerrit-Comment-Date: Fri, 08 Nov 2019 21:26:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Pedro Alves <palves@redhat.com>