[review] Fix crash with core + TUI + run

["Sergio Durigan Junior (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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>
Gerrit-MessageType: comment