From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id EF1713858C5F for ; Thu, 9 Nov 2023 15:05:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EF1713858C5F Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EF1713858C5F Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=195.135.220.29 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699542361; cv=none; b=pzEKOQE02fdH11mToysnBtD6PsZn4YkiiqhWTJyWfFjsXXrPspd8Tj0Qc54MGAHUU62brZNOeByPLLbKQaB7MM741pBIx1otNq/ECGJxgi9/f1sW3jISCE9fgmsajVA2DP/9sQt5CPoxENKO+AAYVDku7XL89783rw2bKMFhTh0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699542361; c=relaxed/simple; bh=W31oBJSgzTADjz1KCnq/Tod+YyTSxZmJs+OuMA8tCog=; h=DKIM-Signature:DKIM-Signature:Message-ID:Date:MIME-Version: Subject:To:From; b=UqAgYKonf3w0o1TziDW22mO0TVcjRrItLmd0K/+gLtDX/IOJKbwGf1aUD32uxB10jXTinkCSXlZSVyMbtuVc/Tq8MbJLMSVZvTD665b3kQqQju0/Y7gpH/oC6jzel+63rkrZkZ0bwoGQ1DHleRhrtT1Fx2NmaJhgpVnfcsXGHps= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 182571F8B6; Thu, 9 Nov 2023 15:05:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1699542359; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=clWnTS0XLW3AB+rZFjzBY3TEIEizJPVBL7nx0RII8dc=; b=MqhyUa+MFRfTzI7ILiHUeXqDvKYlnf+ABZEux587g5TScek3ta6Q9VQSR2wQO1AeyS8Gq9 uF5DaTcXxOPClxso9RKiKEbLuUuS3ZAWUlHmUXw2/QzcdYmZ8tVR6Dp4XOuRvzztkCKNvQ 1k7qUicqPQ3x8R46lFFt+94hodI6bXY= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1699542359; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=clWnTS0XLW3AB+rZFjzBY3TEIEizJPVBL7nx0RII8dc=; b=ryj53yW5s208nqoBgI+2A0UK1O2VLoRs6o3KECmf+RsghCPpduqtv8829xu2Kyqgmq53S9 s+YuVjvCOTMQ2gDA== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 0420913524; Thu, 9 Nov 2023 15:05:58 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id LsZzO1b1TGVVAQAAMHmgww (envelope-from ); Thu, 09 Nov 2023 15:05:58 +0000 Message-ID: Date: Thu, 9 Nov 2023 16:07:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/2] [gdb] Fix segfault in for_each_block, part 1 Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org References: <20231107131950.3083-1-tdevries@suse.de> <20231107131950.3083-2-tdevries@suse.de> From: Tom de Vries In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_INFOUSMEBIZ,KAM_NUMSUBJECT,SPF_HELO_NONE,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 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 ). >> @@ -584,7 +584,7 @@ class inferior : public refcounted_object, >> bool removable = false; >> >> /* The address space bound to this inferior. */ >> - struct address_space *aspace = NULL; >> + std::shared_ptr aspace = NULL; > > Remove `= NULL`. > Done. >> @@ -529,8 +529,8 @@ holding the child stopped. Try \"set detach-on-fork\" or \ >> } >> else >> { >> - child_inf->aspace = new address_space (); >> - child_inf->pspace = new program_space (child_inf->aspace); >> + child_inf->pspace = new program_space (new address_space ()); >> + child_inf->aspace = child_inf->pspace->aspace; > > I would suggest removing the program_space(address_space *) constructor, > and replacing these calls with: > > new program_space (std::make_shared ()) > Ah, I was wondering about something like that, make sense, thanks. > I think the `address_space *` is a bit dangerous. If you pass an > address_space that is managed by a shared_ptr elsewhere, there will be > two shared_ptr separately managing this address_space, which will result > in a double free. > >> @@ -1029,7 +1029,7 @@ handle_vfork_child_exec_or_exit (int exec) >> if (vfork_parent->pending_detach) >> { >> struct program_space *pspace; >> - struct address_space *aspace; >> + std::shared_ptr aspace; > > Especially for non-trivially constructible objects, declare the variable > where first used, to avoid constructing an empty object and then doing > an assignment. In this case, we can even steal the reference from the > inferior to avoid in incref + decref (not a big deal but heh, why not): > > std::shared_ptr aspace = std::move (inf->aspace); > > After that call, inf->aspace is empty (contains nullptr), at least > according to: > > https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr > Done. >> @@ -67,7 +67,9 @@ maybe_new_address_space (void) >> return program_spaces[0]->aspace; >> } >> >> - return new address_space (); >> + std::shared_ptr res; >> + res.reset (new address_space ()); >> + return std::move (res); > > return std::make_shared (); > Done. >> @@ -96,9 +98,17 @@ remove_program_space (program_space *pspace) >> /* See progspace.h. */ >> >> program_space::program_space (address_space *aspace_) >> - : num (++last_program_space_num), >> - aspace (aspace_) >> + : num (++last_program_space_num) >> { >> + aspace.reset (aspace_); >> + program_spaces.push_back (this); >> + gdb::observers::new_program_space.notify (this); >> +} >> + >> +program_space::program_space (std::shared_ptr aspace_) >> + : num (++last_program_space_num) >> +{ >> + aspace = aspace_; > > Initialize aspace in the initialization list, and std::move it while at > it to avoid an incref/decref: > Ack, done. > program_space::program_space (std::shared_ptr aspace_) > : num (++last_program_space_num), > aspace (std::move (aspace_)) > >> @@ -122,8 +132,7 @@ program_space::~program_space () >> /* Defer breakpoint re-set because we don't want to create new >> locations for this pspace which we're tearing down. */ >> clear_symtab_users (SYMFILE_DEFER_BP_RESET); >> - if (!gdbarch_has_shared_address_space (current_inferior ()->arch ())) >> - delete this->aspace; >> + aspace = nullptr; > > This should not be necessary, it will happen when the aspace shared_ptr > gets destroyed (right after this). > Ack, done. >> @@ -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. > Instead, create one shared_ptr like this: > > auto aspace = std::make_shared (); > > And then assign to each (the loop doesn't need to change in fact): > > for (struct program_space *pspace : program_spaces) > pspace->aspace = aspace; > > >> } >> else >> for (struct program_space *pspace : program_spaces) >> - { >> - delete pspace->aspace; >> - pspace->aspace = new address_space (); >> - } >> + pspace->aspace.reset (new address_space ()); > > This would work, but to be more robust and avoid the pitfalls I > explained elsewhere, I would suggest creating address spaces only > through std::make_shared. So: > > pspace->aspace = std::make_shared (); > > > There is also a slight performance advantage in using make_shared, > explained here: > > https://ddanilov.me/shared-ptr-is-evil/ > > make_shared allocates the shared_ptr's control block (the shared thing > that holds the refcount) and the actual object in the same buffer, > whereas using reset + new allocates two separate buffers. > Done. >> @@ -334,7 +335,7 @@ struct program_space >> are global, then this field is ignored (we don't currently >> support inferiors sharing a program space if the target doesn't >> make breakpoints global). */ >> - struct address_space *aspace = NULL; >> + std::shared_ptr aspace = nullptr; > > No need to initialize with nullptr (just like unique_ptr) (and it gets > initialized in the constructor anyway). Done. V3 submitted here ( https://sourceware.org/pipermail/gdb-patches/2023-November/203934.html ). Thanks, - Tom