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 9A93A3858D37 for ; Thu, 9 Nov 2023 16:26:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 9A93A3858D37 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 9A93A3858D37 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=1699547168; cv=none; b=ul3+DhxNTeHQdiq3/uGOzVpG+UjkqiAe3rRGtTvKa0pswtOCuzg21a0nkdEgeiuu47Mdy2R4GJRHy334byWtqtTqt2SCMVX4fopx7KPMCQSrGyxkxIiQZGbHavW6PW82yT4jiDkH+LDNhIKfsbIPftf+PamJnzVWLPZdnO/Xpa0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699547168; c=relaxed/simple; bh=63Iz4JSkQ5rxz3EoNSpsYf/TTY0sPQUNJU4/76v7QoQ=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Xb1IoyVFhUsQZTVtbKN1TTZjaPDV1t4DI39ElY0pBeUR2zbKTOG9NyOQKUoLuJH3xDEb15aTqe4J7C8Th5gHMajqfdgBwXO+wQpGAlo9pcj1lR33t+GTLuA/J0Z2APuJye9aDSxWv0G+v2F1ZMJ8+rTyz+giZdqozd+z2y1kWIs= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1699547166; bh=63Iz4JSkQ5rxz3EoNSpsYf/TTY0sPQUNJU4/76v7QoQ=; h=Date:Subject:To:References:From:In-Reply-To:From; b=s10HIlqZR1fNz2bjt/QGpYcqjTS0yBvmZpDqo0WMcX1WMmJ2w1QTB4SRK6yd8RkP7 lr4k0pMwAbowVFp5xiXvt90oSVYpSraOoCIjs1HAahHrM7y8Qy6iEUi8Q4tyuKp2iO WltV8yZF4JLEyPWn9q4BRyndrRvSiCJTzsxBkRD8= 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 4A6541E098; Thu, 9 Nov 2023 11:26:06 -0500 (EST) Message-ID: Date: Thu, 9 Nov 2023 11:26:05 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 To: Tom de Vries , gdb-patches@sourceware.org References: <20231107131950.3083-1-tdevries@suse.de> <20231107131950.3083-2-tdevries@suse.de> Content-Language: fr From: Simon Marchi In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.6 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/9/23 10:07, Tom de Vries wrote: > On 11/7/23 16:00, Simon Marchi wrote: >> Thanks for the update. I noted a few comments on the use of shared_ptr. >> >>> diff --git a/gdb/inferior.c b/gdb/inferior.c >>> index 1778723863e..37b4fad6e44 100644 >>> --- a/gdb/inferior.c >>> +++ b/gdb/inferior.c >>> @@ -843,15 +843,13 @@ remove_inferior_command (const char *args, int from_tty) >>> struct inferior * >>> add_inferior_with_spaces (void) >>> { >>> - struct address_space *aspace; >>> struct program_space *pspace; >>> struct inferior *inf; >>> /* If all inferiors share an address space on this system, this >>> doesn't really return a new address space; otherwise, it >>> really does. */ >>> - aspace = maybe_new_address_space (); >>> - pspace = new program_space (aspace); >>> + pspace = new program_space (maybe_new_address_space ()); >>> inf = add_inferior (0); >>> inf->pspace = pspace; >> >> Does it work if you remove the pspace variable too and just do: >> >> inf->pspace = new program_space (maybe_new_address_space ()); >> >>> @@ -1014,15 +1012,13 @@ clone_inferior_command (const char *args, int from_tty) >>> for (i = 0; i < copies; ++i) >>> { >>> - struct address_space *aspace; >>> struct program_space *pspace; >>> struct inferior *inf; >>> /* If all inferiors share an address space on this system, this >>> doesn't really return a new address space; otherwise, it >>> really does. */ >>> - aspace = maybe_new_address_space (); >>> - pspace = new program_space (aspace); >>> + pspace = new program_space (maybe_new_address_space ()); >>> inf = add_inferior (0); >>> inf->pspace = pspace; >> >> Same here, if it works. >> > > It does work, but the change is user visible, so I've factored that out into a separate patch in v3 ( https://sourceware.org/pipermail/gdb-patches/2023-November/203931.html ). Oh, well if it changes the behavior of GDB, maybe that's not a good idea then. I suggested that just because it would make the code cleaner, but I don't think that's a good reason to justify changing the behavior. >>> @@ -413,16 +422,12 @@ update_address_spaces (void) >>> { >>> struct address_space *aspace = new address_space (); >>> - delete current_program_space->aspace; >>> for (struct program_space *pspace : program_spaces) >>> - pspace->aspace = aspace; >>> + pspace->aspace.reset (aspace); >> >> I think this is a wrong use of shared_ptr, you tell separate >> shared_ptrs to start managing this address_space object. They'll all >> have a refcount of 1, and will all try to delete the address space >> later. >> > > Ouch. I wondered why this didn't surface in my testing, but I guess that's because I only tested native, and this is only exercised with remote. I don't think any regular testing exercises this, you'd need to test against an uclinux target (an MMU-less Linux) to reach that code. Simon