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 B3DE63858D1E for ; Thu, 9 Nov 2023 18:19:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org B3DE63858D1E 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 B3DE63858D1E 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=1699553981; cv=none; b=RT+DQBm+K273rM5zWubrTNk7m+gJRa20XOECz+tALP9Atey+5XlJbqe/juQirzZF+7620ohRmbHu5090eLPO95GDBVD0KzV0yilBpy6pEPhw3+/bWL3W0tgAUu7PvHBSEYd0BlshepQgVC00iIwgjKP78uqBc2DO/5pDnmnLLlE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699553981; c=relaxed/simple; bh=S+x1vUL04TTu9gdFZ32D2LHLSIx64M6TwYXfSx5YJ/0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=RWU61Xe5v2kOWwwROZW82u8dyNiD/eWGcbCJRlKEBswobjdbrrMAsivLP13V/j2wLbi6SQgiCZG2jQ0HyEkMRcaEPjVGsqZgjoGVMjTk/kGhbOllKylQPiaAZ7WY1jCNhbllHurhKkQj4P+NfgileeQB38pSycDsf3UWABcizOI= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1699553979; bh=S+x1vUL04TTu9gdFZ32D2LHLSIx64M6TwYXfSx5YJ/0=; h=Date:Subject:To:References:From:In-Reply-To:From; b=dAcm1mdJSsTtAVBOkur1P1R9DhZmRySHxxcwm0XulKZ0meJwqL8JXA1oyoz+QJiTa bsdMSkmOoQ6yDBYv376MfZxOaxeOxEKvqe6jBzkvuLSAo7iZLo7VgWl/Pe963GaR0h BSkkfBgxqI61Tot34hL6nSlsCnf8YS9QdaRIpGKY= 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 3D9811E00F; Thu, 9 Nov 2023 13:19:39 -0500 (EST) Message-ID: <7a4ddb90-b040-427e-8b15-dead9e4d6a81@simark.ca> Date: Thu, 9 Nov 2023 13:19:38 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/3] [gdb] Fix segfault in for_each_block, part 1 Content-Language: fr To: Tom de Vries , gdb-patches@sourceware.org References: <20231109150021.5504-1-tdevries@suse.de> <20231109150021.5504-2-tdevries@suse.de> From: Simon Marchi In-Reply-To: <20231109150021.5504-2-tdevries@suse.de> 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:00, Tom de Vries wrote: > @@ -95,9 +95,9 @@ remove_program_space (program_space *pspace) > > /* See progspace.h. */ > > -program_space::program_space (address_space *aspace_) > +program_space::program_space (std::shared_ptr aspace_) > : num (++last_program_space_num), > - aspace (aspace_) > + aspace (std::move (aspace_)) The alignment here is not right, `aspace` should be aligned with `num`. I asked Pedro if he had any thoughts about this patch, and he suggested not to use shared_ptr (it's not ideal for various performance reasons, including its use of atomic erations), but to use refcounted_object (like you initially had) plus gdb::ref_ptr with an appropriate policy that makes it behave like shared_ptr (deletes the object when the refcount reaches 0). I was not really familiar with that, but I gave it a try to avoid making you re-implement this again. Here's a patch (only built-tested) that applies on top of your patch 1/3 that switches to ref_ptr. Sorry for leading you to the wrong direction again. Simon >From 164ffe71449a53579f58be026f4619d8b73e41ff Mon Sep 17 00:00:00 2001 From: Simon Marchi Date: Thu, 9 Nov 2023 16:59:46 +0000 Subject: [PATCH] fixup for patch 1, use gdb::ref_ptr Change-Id: Iad4c992db97664b1ecb97ad60cc36eda6dfb5659 --- gdb/inferior.h | 2 +- gdb/infrun.c | 11 +++--- gdb/progspace.c | 15 ++++---- gdb/progspace.h | 64 +++++++++++++++++++++------------- gdb/scoped-mock-context.h | 2 +- gdbsupport/refcounted-object.h | 17 +++++++++ 6 files changed, 69 insertions(+), 42 deletions(-) diff --git a/gdb/inferior.h b/gdb/inferior.h index db75d6b5ecb..3f74cb5d8d4 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -584,7 +584,7 @@ class inferior : public refcounted_object, bool removable = false; /* The address space bound to this inferior. */ - std::shared_ptr aspace; + address_space_ref_ptr aspace; /* The program space bound to this inferior. */ struct program_space *pspace = NULL; diff --git a/gdb/infrun.c b/gdb/infrun.c index 34c495097e5..95620d479d8 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -529,8 +529,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ } else { - child_inf->pspace - = new program_space (std::make_shared ()); + child_inf->pspace = new program_space (new_address_space ()); child_inf->aspace = child_inf->pspace->aspace; child_inf->removable = true; clone_program_space (child_inf->pspace, parent_inf->pspace); @@ -609,8 +608,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ child_inf->aspace = parent_inf->aspace; child_inf->pspace = parent_inf->pspace; - parent_inf->pspace - = new program_space (std::make_shared ()); + parent_inf->pspace = new program_space (new_address_space ()); parent_inf->aspace = parent_inf->pspace->aspace; clone_program_space (parent_inf->pspace, child_inf->pspace); @@ -620,8 +618,7 @@ holding the child stopped. Try \"set detach-on-fork\" or \ } else { - child_inf->pspace - = new program_space (std::make_shared ()); + child_inf->pspace = new program_space (new_address_space ()); child_inf->aspace = child_inf->pspace->aspace; child_inf->removable = true; child_inf->symfile_flags = SYMFILE_NO_READ; @@ -1057,7 +1054,7 @@ handle_vfork_child_exec_or_exit (int exec) pspace = inf->pspace; inf->pspace = nullptr; - std::shared_ptr aspace = std::move (inf->aspace); + address_space_ref_ptr aspace = std::move (inf->aspace); if (print_inferior_events) { diff --git a/gdb/progspace.c b/gdb/progspace.c index 2d769134ac6..c0f4b725516 100644 --- a/gdb/progspace.c +++ b/gdb/progspace.c @@ -55,8 +55,8 @@ address_space::address_space () return a pointer to an existing address space, in case inferiors share an address space on this target system. */ -std::shared_ptr -maybe_new_address_space (void) +address_space_ref_ptr +maybe_new_address_space () { int shared_aspace = gdbarch_has_shared_address_space (current_inferior ()->arch ()); @@ -67,7 +67,7 @@ maybe_new_address_space (void) return program_spaces[0]->aspace; } - return std::make_shared (); + return new_address_space (); } /* Start counting over from scratch. */ @@ -95,7 +95,7 @@ remove_program_space (program_space *pspace) /* See progspace.h. */ -program_space::program_space (std::shared_ptr aspace_) +program_space::program_space (address_space_ref_ptr aspace_) : num (++last_program_space_num), aspace (std::move (aspace_)) { @@ -409,14 +409,14 @@ update_address_spaces (void) if (shared_aspace) { - auto aspace = std::make_shared (); + address_space_ref_ptr aspace = new_address_space (); for (struct program_space *pspace : program_spaces) pspace->aspace = aspace; } else for (struct program_space *pspace : program_spaces) - pspace->aspace = std::make_shared (); + pspace->aspace = new_address_space (); for (inferior *inf : all_inferiors ()) if (gdbarch_has_global_solist (current_inferior ()->arch ())) @@ -453,6 +453,5 @@ initialize_progspace (void) modules have done that. Do this before initialize_current_architecture, because that accesses the ebfd of current_program_space. */ - current_program_space - = new program_space (std::make_shared ()); + current_program_space = new program_space (new_address_space ()); } diff --git a/gdb/progspace.h b/gdb/progspace.h index c169690681b..163cbf6a0f8 100644 --- a/gdb/progspace.h +++ b/gdb/progspace.h @@ -29,6 +29,8 @@ #include "gdbsupport/next-iterator.h" #include "gdbsupport/safe-iterator.h" #include "gdbsupport/intrusive_list.h" +#include "gdbsupport/refcounted-object.h" +#include "gdbsupport/gdb_ref_ptr.h" #include #include @@ -43,6 +45,40 @@ struct shobj; typedef std::list> objfile_list; +/* An address space. It is used for comparing if + pspaces/inferior/threads see the same address space and for + associating caches to each address space. */ +struct address_space : public refcounted_object +{ + /* Create a new address space object, and add it to the list. */ + address_space (); + DISABLE_COPY_AND_ASSIGN (address_space); + + /* Returns the integer address space id of this address space. */ + int num () const + { + return m_num; + } + + /* Per aspace data-pointers required by other GDB modules. */ + registry registry_fields; + +private: + int m_num; +}; + +using address_space_ref_ptr + = gdb::ref_ptr>; + +/* Create a new address space. */ + +static inline address_space_ref_ptr +new_address_space () +{ + return address_space_ref_ptr::new_reference (new address_space); +} + /* An iterator that wraps an iterator over std::unique_ptr, and dereferences the returned object. This is useful for iterating over a list of shared pointers and returning raw pointers -- which @@ -192,7 +228,7 @@ struct program_space { /* Constructs a new empty program space, binds it to ASPACE, and adds it to the program space list. */ - explicit program_space (std::shared_ptr aspace); + explicit program_space (address_space_ref_ptr aspace); /* Releases a program space, and all its contents (shared libraries, objfiles, and any other references to the program space in other @@ -334,7 +370,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). */ - std::shared_ptr aspace; + address_space_ref_ptr aspace; /* True if this program space's section offsets don't yet represent the final offsets of the "live" address space (that is, the @@ -381,28 +417,6 @@ struct program_space std::vector m_target_sections; }; -/* An address space. It is used for comparing if - pspaces/inferior/threads see the same address space and for - associating caches to each address space. */ -struct address_space -{ - /* Create a new address space object, and add it to the list. */ - address_space (); - DISABLE_COPY_AND_ASSIGN (address_space); - - /* Returns the integer address space id of this address space. */ - int num () const - { - return m_num; - } - - /* Per aspace data-pointers required by other GDB modules. */ - registry registry_fields; - -private: - int m_num; -}; - /* The list of all program spaces. There's always at least one. */ extern std::vectorprogram_spaces; @@ -445,7 +459,7 @@ class scoped_restore_current_program_space /* Maybe create a new address space object, and add it to the list, or return a pointer to an existing address space, in case inferiors share an address space. */ -extern std::shared_ptr maybe_new_address_space (void); +extern address_space_ref_ptr maybe_new_address_space (); /* Update all program spaces matching to address spaces. The user may have created several program spaces, and loaded executables into diff --git a/gdb/scoped-mock-context.h b/gdb/scoped-mock-context.h index f6bd63a5ae7..70b913b949b 100644 --- a/gdb/scoped-mock-context.h +++ b/gdb/scoped-mock-context.h @@ -38,7 +38,7 @@ struct scoped_mock_context Target mock_target; ptid_t mock_ptid {1, 1}; - program_space mock_pspace {std::make_shared ()}; + program_space mock_pspace {new_address_space ()}; inferior mock_inferior {mock_ptid.pid ()}; thread_info mock_thread {&mock_inferior, mock_ptid}; diff --git a/gdbsupport/refcounted-object.h b/gdbsupport/refcounted-object.h index d8fdb950043..294fd873df1 100644 --- a/gdbsupport/refcounted-object.h +++ b/gdbsupport/refcounted-object.h @@ -67,4 +67,21 @@ struct refcounted_object_ref_policy } }; +/* A policy class to interface gdb::ref_ptr with a refcounted_object, that + deletes the object once the refcount reaches 0.. */ + +template +struct refcounted_object_delete_ref_policy +{ + static void incref (T *obj) + { obj->incref (); } + + static void decref (T *obj) + { + obj->decref (); + if (obj->refcount () == 0) + delete obj; + } +}; + #endif /* COMMON_REFCOUNTED_OBJECT_H */ -- 2.39.2