From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 030BF3858D33 for ; Mon, 6 Nov 2023 15:24:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 030BF3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 030BF3858D33 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699284253; cv=none; b=bNOPBd+dif5cy9B4IS2n3IEMJcGDlbBnpopLJhEskteRduO3Wi41BSLTEY+/EfLwQns89/uHczAapY2fXT1UG414KwoM65O6lMk2DSZSQ2XSIDtGByAODB4ld31lIU1IqDolSMCLfpWWte0laF4WWEvECIVqvMjLDn/Hg001IXg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699284253; c=relaxed/simple; bh=2bnOGeyhK/P8jQ7F/mgnktBnFuYpIpA4eQj6iXZcjq8=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=B74x50Tq1rcFsOJ9+QCx/qwbAoQ7fEFlp9out90OLerg4fbxZf9CTMN3O1v5wELw2hxg4TQ8vYbnVTOmN6RQFnOHzNknSZFltU02iTmrMClpLUGasoxSYpNvs1vXo6QF++LNP9eflb1zHWKnHhFwiqfZpWLLz1mizrITLNHNY0M= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699284251; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=MYgolc34bSwSCZkWjP6y4TRlOtuU3fpE9vm4XQvrods=; b=MHsODlWA17FPNZvdtCIm/xeHbYBr0qhrhuRIEauVd37JAgRBYejQPCBZQn7Z5qi2b5gcIv bKzCdmo47w9UnHnfv4Mjn/e/hY0Pi+QIf6PohuCCHrF3DJNBaecw39m56FnPxwKemzaIBQ Z6YN7Mv2CGGeLi8km10vzG+nJnSB14M= Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-588-g1OYp7weNhmFeqc3vSySVg-1; Mon, 06 Nov 2023 10:24:05 -0500 X-MC-Unique: g1OYp7weNhmFeqc3vSySVg-1 Received: by mail-qk1-f198.google.com with SMTP id af79cd13be357-778964b7c8bso505055785a.1 for ; Mon, 06 Nov 2023 07:24:05 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699284244; x=1699889044; h=mime-version:message-id:date:references:in-reply-to:subject:to:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=t8rdYUR2Svby0K2ZIg+L8XTT9jcmcqgTiYLDdrGWQq4=; b=PVCbAdzQx6aOdX5YI3VMnDV3L13kXVjUYLRKdboYQoOvUEnSRDLZvsZlG7M4Kgb1S6 bLGyI65nDzbtHEjbtrPSamkl0JwxitkFHiM8dEBfzBIZzbY+dkoM9mTxo3kYEyySxFhj KqCip+oSWzN2HSnr5qNU0ZRRLnGQkjDh0DRgBGUtNDyAG8Tkch7QO9+hP5usW7fZliLn bFIV4ai2JZkAtC2VgldH8ZyINey6pySiofGskN+yvgFogPiJXGr8CEcv9Ozx2uH5VZYY ohOf28IQjFgPLaWcP4o8y4dp2Jl+PX8Es0iMr7MXG0evE50Uj/MPVjjUP0atXEeyR7U5 ccdQ== X-Gm-Message-State: AOJu0YyyprkjiABGzwoZj45R2PkxlDllfKFPT46/96V45UY2Jlzj7k8a gPb8y2jFq23HMm8aQPFKHa1PHv5sx3OC65yldMI7CvO7nRTwy/2EEYR1R04lZZsr0xry/HiOl+/ ugYxIwX2hs7opfkO818WnIB7c54g84Q== X-Received: by 2002:a05:620a:6649:b0:773:c19f:9b9 with SMTP id qg9-20020a05620a664900b00773c19f09b9mr23982205qkn.46.1699284244607; Mon, 06 Nov 2023 07:24:04 -0800 (PST) X-Google-Smtp-Source: AGHT+IFCDL5YNoLIyeuRdKgfbla8gMNzAP0zvQvQwUxoSVNeLbm8vcFM9HXjbvZGwmKL+xsBYSao2g== X-Received: by 2002:a05:620a:6649:b0:773:c19f:9b9 with SMTP id qg9-20020a05620a664900b00773c19f09b9mr23982187qkn.46.1699284244258; Mon, 06 Nov 2023 07:24:04 -0800 (PST) Received: from localhost (105.226.159.143.dyn.plus.net. [143.159.226.105]) by smtp.gmail.com with ESMTPSA id a12-20020a05620a124c00b0076db1caab16sm3382903qkl.22.2023.11.06.07.24.03 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Nov 2023 07:24:04 -0800 (PST) From: Andrew Burgess To: Tom de Vries , gdb-patches@sourceware.org Subject: Re: [PATCH 1/2] [gdb] Fix segfault in for_each_block, part 1 In-Reply-To: <20231104155757.16649-2-tdevries@suse.de> References: <20231104155757.16649-1-tdevries@suse.de> <20231104155757.16649-2-tdevries@suse.de> Date: Mon, 06 Nov 2023 15:24:02 +0000 Message-ID: <87v8aeucq5.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_INVALID,DKIM_SIGNED,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: Tom de Vries writes: > When running test-case gdb.base/vfork-follow-parent.exp on powerpc64 (likewise > on s390x), I run into: > ... > (gdb) PASS: gdb.base/vfork-follow-parent.exp: \ > exec_file=vfork-follow-parent-exit: target-non-stop=on: non-stop=off: \ > resolution_method=schedule-multiple: print unblock_parent = 1 > continue^M > Continuing.^M > Reading symbols from vfork-follow-parent-exit...^M > ^M > ^M > Fatal signal: Segmentation fault^M > ----- Backtrace -----^M > 0x1027d3e7 gdb_internal_backtrace_1^M > src/gdb/bt-utils.c:122^M > 0x1027d54f _Z22gdb_internal_backtracev^M > src/gdb/bt-utils.c:168^M > 0x1057643f handle_fatal_signal^M > src/gdb/event-top.c:889^M > 0x10576677 handle_sigsegv^M > src/gdb/event-top.c:962^M > 0x3fffa7610477 ???^M > 0x103f2144 for_each_block^M > src/gdb/dcache.c:199^M > 0x103f235b _Z17dcache_invalidateP13dcache_struct^M > src/gdb/dcache.c:251^M > 0x10bde8c7 _Z24target_dcache_invalidatev^M > src/gdb/target-dcache.c:50^M > ... > or similar. > > The root cause for the segmentation fault is that linux_is_uclinux gives an > incorrect result: it should always return false, given that we're running on a > regular linux system, but instead it returns first true, then false. > > In more detail, the segmentation fault happens as follows: > - a program space with an address space is created > - a second program space is about to be created. maybe_new_address_space > is called, and because linux_is_uclinux returns true, maybe_new_address_space > returns false, and no new address space is created > - a second program space with the same address space is created > - a program space is deleted. Because linux_is_uclinux now returns false, > gdbarch_has_shared_address_space (current_inferior ()->arch ()) returns > false, and the address space is deleted > - when gdb uses the address space of the remaining program space, we run into > the segfault, because the address space is deleted. > > Hardcoding linux_is_uclinux to false makes the test-case pass. > > We leave addressing the root cause for the following commit in this series. > > For now, prevent the segmentation fault by making the address space a refcounted > object. > > This was already suggested here [1]: > ... > A better solution might be to have the address spaces be reference counted > ... > > 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 > > [1] https://sourceware.org/pipermail/gdb-patches/2023-October/202928.html > --- > gdb/progspace.c | 37 +++++++++++++++++++++++++++---------- > gdb/progspace.h | 11 ++++++++++- > 2 files changed, 37 insertions(+), 11 deletions(-) > > diff --git a/gdb/progspace.c b/gdb/progspace.c > index 839707e9d71..4fea21f0ca1 100644 > --- a/gdb/progspace.c > +++ b/gdb/progspace.c > @@ -96,9 +96,9 @@ 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) > { > + set_aspace (aspace_); > program_spaces.push_back (this); > gdb::observers::new_program_space.notify (this); > } > @@ -122,8 +122,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; > + reset_aspace (); > } > > /* See progspace.h. */ > @@ -409,20 +408,19 @@ update_address_spaces (void) > > init_address_spaces (); > > + for (struct program_space *pspace : program_spaces) > + pspace->reset_aspace (); > + > if (shared_aspace) > { > struct address_space *aspace = new address_space (); > > - delete current_program_space->aspace; > for (struct program_space *pspace : program_spaces) > - pspace->aspace = aspace; > + pspace->set_aspace (aspace); > } > else > for (struct program_space *pspace : program_spaces) > - { > - delete pspace->aspace; > - pspace->aspace = new address_space (); > - } > + pspace->set_aspace (new address_space ()); > > for (inferior *inf : all_inferiors ()) > if (gdbarch_has_global_solist (current_inferior ()->arch ())) > @@ -433,8 +431,27 @@ update_address_spaces (void) > > > > +void > +program_space::set_aspace (struct address_space *aspace_) > +{ > + aspace = aspace_; > + > + aspace->incref (); > +} > + > /* See progspace.h. */ > > +void > +program_space::reset_aspace () > +{ > + aspace->decref (); > + > + if (aspace->refcount () == 0) > + delete aspace; > + > + aspace = nullptr; > +} I wouldn't have expected the reference counting to be done manually like this. I would have expected either: * Use a std::shared_ptr within program_space, and then either also hold a std::shared_ptr within the inferior too, or potentially loan out raw pointers (to the inferior) using std::shared_ptr::get, or * Create a new type using gdb::ref_ptr ... but thinking about it, given the reference counting policy on this would be pretty vanilla, if you had done this, I think I'd be asking why not just use std::shared_ptr. > + > void > program_space::clear_solib_cache () > { > diff --git a/gdb/progspace.h b/gdb/progspace.h > index a22e427400e..065ca38e255 100644 > --- a/gdb/progspace.h > +++ b/gdb/progspace.h > @@ -336,6 +336,10 @@ struct program_space > make breakpoints global). */ > struct address_space *aspace = NULL; > > + void set_aspace (struct address_space *aspace); > + > + void reset_aspace (); These should have explanatory comments. It feels like at a minimum aspace should be made private, though I really think its type should be changed to something that manages the reference counting for us. Thanks, Andrew > + > /* True if this program space's section offsets don't yet represent > the final offsets of the "live" address space (that is, the > section addresses still require the relocation offsets to be > @@ -384,12 +388,17 @@ struct program_space > /* 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 > +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); > > + ~address_space () > + { > + gdb_assert (refcount () == 0); > + } > + > /* Returns the integer address space id of this address space. */ > int num () const > { > -- > 2.35.3