From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <gdb-patches-return-155405-listarch-gdb-patches=sources.redhat.com@sourceware.org>
Received: (qmail 112609 invoked by alias); 18 Apr 2019 08:02:56 -0000
Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm
Precedence: bulk
List-Id: <gdb-patches.sourceware.org>
List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org>
List-Archive: <http://sourceware.org/ml/gdb-patches/>
List-Post: <mailto:gdb-patches@sourceware.org>
List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs>
Sender: gdb-patches-owner@sourceware.org
Received: (qmail 112588 invoked by uid 89); 18 Apr 2019 08:02:56 -0000
Authentication-Results: sourceware.org; auth=none
X-Spam-SWARE-Status: No, score=-13.0 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=HTo:U*palves
X-HELO: mx1.suse.de
Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 18 Apr 2019 08:02:55 +0000
Received: from relay2.suse.de (unknown [195.135.220.254])	by mx1.suse.de (Postfix) with ESMTP id EF58FB014;	Thu, 18 Apr 2019 08:02:52 +0000 (UTC)
Subject: Re: [PATCH] Handle vfork in thread with follow-fork-mode child
To: Pedro Alves <palves@redhat.com>, gdb-patches@sourceware.org
References: <20190416150652.GA4805@delia> <d5e1c85d-a880-1b6a-7e7c-19c739c111bb@redhat.com>
From: Tom de Vries <tdevries@suse.de>
Message-ID: <d7be9ac9-bf48-145f-3e35-11faaf3da270@suse.de>
Date: Thu, 18 Apr 2019 08:02:00 -0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <d5e1c85d-a880-1b6a-7e7c-19c739c111bb@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
X-IsSubscribed: yes
X-SW-Source: 2019-04/txt/msg00310.txt.bz2

On 17-04-19 19:45, Pedro Alves wrote:
> On 4/16/19 4:06 PM, Tom de Vries wrote:
>> Hi,
> 
> Hi!
> 
> Comments below.  As I was reviewing this, I kept experimenting,
> so I ended up addressing my own comments myself.  See updated patch
> at the bottom.
> 

Hi Pedro,

thanks for the comments and updated patch.

>> @@ -982,7 +983,17 @@ handle_vfork_child_exec_or_exit (int exec)
>>  		}
>>  	    }
>>  
>> -	  target_detach (inf->vfork_parent, 0);
>> +	  /* Now that the vfork child has terminated, make sure during detach
> 
> This path is also reached if the vfork child execs, so the reference to
> "terminated" above would better be exec.  But also, the other paths in
> the function already clear vfork_parent/vfork_child, so I think it's better
> to refactor things a bit so that all paths share the code.
> 

> +      inf->vfork_parent = NULL;

I was checking this and found dereferences of inf->vfork_parent after it
was set to NULL here:
...
      else if (exec)
        {
          ...
          resume_parent = inf->vfork_parent->pid;
...
and here:
...
      else
        {
          ...
          clone_program_space (pspace, inf->vfork_parent->pspace);
...

To confirm, I did another testrun with aborts at the start of the
blocks, and I found no regressions. So, either this is dead code, or we
need test-cases that trigger these paths.

Thanks,
- Tom