From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id E171F3858C53 for ; Tue, 7 Nov 2023 15:26:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org E171F3858C53 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org E171F3858C53 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699370784; cv=none; b=Pe3VyE5m/Z45gZRbRnqChuq9XcnERlktNNepmYm0IXtJ4n+1lyH1r/ggCWTNNTg/A2VxIWp41Ph4i7nfy5FmD+G0C56qv57KwdVhEuXc8iOihD1etIlWQGY4nW8TN/W7dHtAKWemyTfexFiI1i2672NBCoNWjcaWkoRsw2Ekp2Q= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699370784; c=relaxed/simple; bh=K92SkA8ZP8yPbaQoUG0uyjWPprTKA9gDLvjn9Otb2yc=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Go4wdryxlPZVExeFMT77y3FTwyqDWV06c9e/+EdTT9ozBhWf+oltnoyYCIB6oHUHfJ9DzWKUDI7b4kMURW/XO5hMGuilUf0K64TAmitJ2Jcn+7hOr6kb1oHioZk7FjWBUaqe5zuTPjcB0n6+eZIDu4iiUp8zH46v5YLT5PCY/sg= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1699370782; bh=K92SkA8ZP8yPbaQoUG0uyjWPprTKA9gDLvjn9Otb2yc=; h=Date:Subject:To:References:From:In-Reply-To:From; b=hbkPRw3ZLvUpYBsLhCttZ+j9mVmvnDjrp5jSfkxyZEZv4kPXtHpupukmVmjW/ABX9 WSDTFj6ohd3fLS9qGrPErJ2g+0SEsIbS/VEeMp5pYFt+BJ9z2rPFtN0L9lFo+Vjdja OXQho1XbVAjxHMifUdPbjW+ENWMITzX/P2LOnWg0= Received: from [172.16.0.192] (192-222-143-198.qc.cable.ebox.net [192.222.143.198]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id 577C21E00F; Tue, 7 Nov 2023 10:26:22 -0500 (EST) Message-ID: Date: Tue, 7 Nov 2023 10:26:21 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/2] [gdb] Fix segfault in for_each_block, part 2 Content-Language: fr To: Tom de Vries , gdb-patches@sourceware.org References: <20231107131950.3083-1-tdevries@suse.de> <20231107131950.3083-3-tdevries@suse.de> From: Simon Marchi In-Reply-To: <20231107131950.3083-3-tdevries@suse.de> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_NUMSUBJECT,SPF_HELO_PASS,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/7/23 08:19, Tom de Vries wrote: > The previous commit describes PR gdb/30547, a segfault when running test-case > gdb.base/vfork-follow-parent.exp on powerpc64 (likewise on s390x). > > The root cause for the segmentation fault is that linux_is_uclinux gives an > incorrect result: it returns true instead of false. > > So, why does linux_is_uclinux: > ... > int > linux_is_uclinux (void) > { > CORE_ADDR dummy; > > return (target_auxv_search (AT_NULL, &dummy) > 0 > && target_auxv_search (AT_PAGESZ, &dummy) == 0); > ... > return true? > > This is because ppc_linux_target_wordsize returns 4 instead of 8, causing > ppc_linux_nat_target::auxv_parse to misinterpret the auxv vector. > > So, why does ppc_linux_target_wordsize: > ... > int > ppc_linux_target_wordsize (int tid) > { > int wordsize = 4; > > /* Check for 64-bit inferior process. This is the case when the host is > 64-bit, and in addition the top bit of the MSR register is set. */ > long msr; > > errno = 0; > msr = (long) ptrace (PTRACE_PEEKUSER, tid, PT_MSR * 8, 0); > if (errno == 0 && ppc64_64bit_inferior_p (msr)) > wordsize = 8; > > return wordsize; > } > ... > return 4? > > Specifically, we get this result because because tid == 0, so we get > errno == ESRCH. I would suggest adding some assertion: - assert that inferior_ptid != null_ptid in ppc_linux_nat_target::auxv_parse and s390_linux_nat_target::auxv_parse - assert that tid != 0 in ppc_linux_target_wordsize (and in s390_target_wordsize), though it could use a bit of refactoring to take tid as a parameter like ppc_linux_target_wordsize) > The tid == 0 is caused by the switch_to_no_thread in > handle_vfork_child_exec_or_exit: > ... > /* Switch to no-thread while running clone_program_space, so > that clone_program_space doesn't want to read the > selected frame of a dead process. */ > scoped_restore_current_thread restore_thread; > switch_to_no_thread (); > > inf->pspace = new program_space (maybe_new_address_space ()); > ... > but moving the maybe_new_address_space call to before that gives us the > same result. The tid is no longer 0, but we still get ESRCH because the > thread has exited. > > Fix this in handle_vfork_child_exec_or_exit by doing the > maybe_new_address_space call in the context of the vfork parent. > > Tested on top of trunk on x86_64-linux and ppc64le-linux. > Tested on top of gdb-14-branch on ppc64-linux. > > PR gdb/30547 > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30547 > --- > gdb/infrun.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/gdb/infrun.c b/gdb/infrun.c > index f0f35c23c9c..94a0924653f 100644 > --- a/gdb/infrun.c > +++ b/gdb/infrun.c > @@ -1105,13 +1105,23 @@ handle_vfork_child_exec_or_exit (int exec) > go ahead and create a new one for this exiting > inferior. */ > > + std::shared_ptr aspace; > + { > + /* Temporarily switch to the vfork parent, to facilitate ptrace > + calls done during maybe_new_address_space. */ > + scoped_restore save_inferior_ptid > + = make_scoped_restore (&inferior_ptid); > + inferior_ptid = ptid_t (vfork_parent->pid); > + aspace = maybe_new_address_space (); > + } I guess that's fine as a minimal patch, but it scares me when we just set part of the global state and inferior_ptid / current_inferior / current_program_space don't match. maybe_new_address_space calls gdbarch_has_shared_address_space, which calls linux_is_uclinux, which reads auxv. auxv can be read in different ways. It's hard to get convinced that only setting inferior_ptid is enough. Since we need a scoped_restore_current_thread in the current scope, can you move it a bit higher? Something like (untested): scoped_restore_current_thread restore_thread; /* Temporarily switch to the vfork parent, to facilitate ptrace calls done during maybe_new_address_space. */ switch_to_thread (any_live_thread_of_inferior (vfork_parent)); std::shared_ptr aspace = maybe_new_address_space (); /* Switch back to the vfork child inferior. Switch to no-thread while running clone_program_space, so that clone_program_space doesn't want to read the selected frame of a dead process. */ switch_to_inferior_no_thread (inf); Below, you can std::move the aspace when creating the new program_space: inf->pspace = new program_space (std::move (aspace)); Even better, I think, would be to pass the vfork parent inferior as a new `existing_inferior` parameter to maybe_new_address_space, and pass it all the way through linux_is_uclinux (and do some inferior switching there in order to do the target calls). The existing_inferior parameter would be an inferior that maybe_new_address_space can inspect to determine if a new address space is needed, and from which it can get an existing address space, if it determines that the address space should be shared between the existing and the new inferior. But I think it's something that can be done later, switching to the parent here is fine to keep the patch small and easily backportable. Simon