From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by sourceware.org (Postfix) with ESMTPS id BCB4F3858C5E for ; Wed, 28 Jun 2023 08:39:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BCB4F3858C5E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=intel.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=intel.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1687941589; x=1719477589; h=from:to:subject:date:message-id:references:in-reply-to: mime-version:content-transfer-encoding; bh=QYAI+UpoIT/U9/+t4ESTZ8E+mXO3/Idrc8AqRYa6oeo=; b=eK8AGIrAoHJK/ISf7rkhiIcXSvQHN6kaEf+ehyjI6ZfQBnXaBT3RCgKi eXhjzbkNTJp9CUtLDmUiB/hO4jIGvqF6skSGyYciEiQbeY1G3rVkS4FHb QzsFs3Z1TlAqVCC+PdDcheM82BKz7BiILFQ9g+LyoS/UR9/lBgE0OTK6I om5n+ZhNFhM6gCn6UPfcJ26wozylsCNsPnXsGyfZCp9djOIlu2oc7+n68 ffIBi855KcrJuJK8PNg1T8ZHd5MOgS1v/HQCAgvgyzpZ5XmYvMvaX5PZD evJi7L44kQqTkiVZMSm7vPj/2jTX8FwHzDN+aRCqwJwepLdrIIsbXwm8Q A==; X-IronPort-AV: E=McAfee;i="6600,9927,10754"; a="448173491" X-IronPort-AV: E=Sophos;i="6.01,165,1684825200"; d="scan'208";a="448173491" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jun 2023 01:39:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10754"; a="1047307596" X-IronPort-AV: E=Sophos;i="6.01,165,1684825200"; d="scan'208";a="1047307596" Received: from orsmsx601.amr.corp.intel.com ([10.22.229.14]) by fmsmga005.fm.intel.com with ESMTP; 28 Jun 2023 01:39:44 -0700 Received: from orsmsx612.amr.corp.intel.com (10.22.229.25) by ORSMSX601.amr.corp.intel.com (10.22.229.14) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Wed, 28 Jun 2023 01:39:44 -0700 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX612.amr.corp.intel.com (10.22.229.25) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Wed, 28 Jun 2023 01:39:43 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx610.amr.corp.intel.com (10.22.229.23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27 via Frontend Transport; Wed, 28 Jun 2023 01:39:43 -0700 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (104.47.57.171) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.27; Wed, 28 Jun 2023 01:39:43 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=eXFN67xWUN6Fv0S990niTTiIg/9cMn9jVG9W4GfZ5L7/GnkAuW3erqyOs9uBzPRDu0Ve5MNIcO9zRyxqocvelibKGH21fvirMGVg2Bi44pccwRnESPiklnSug0fL0jr1Z8HJxpJ7FzcSUmdlfbXQdaJafSosYUrYb1ZCGH7vZ149r9fA1IkHlyzcE73IrNRtx074I7hNGkLppBD75rZkFICfFeY3ykhl6s65rzBNaXcivhrtLmBXJowseoS+GlOEFbTf+mLTUF+7mYKSpyVLfytnV4ns3r7LiwedurGK/wizQ35CZNzvr/Nt91teM5jVZE/a/qaFcIOL7UhJxFpiQw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=YLZd/4/b2i7njJf/igTdaOQqtp8eMNat7dJh+iBQbxE=; b=am7stNjqqr3VwhQwXuB3HtuFfVNTj3lT9F1YtZ6V9V+XzggEs1Sv1967kvZu3VrPxZo3L4w4LTLeuTtvQtKSH2+Hsbni0ZwtMAml7VPMxjQCUYR8QU2S2YJCdoGZv89VfEKluqe54FWyHPaDqdgN4HLifI416Ykzie+FuHp2nTGChEzIvvpG3bBheTc6s0/dQIH1i+Qld8jTu3tW4p1KKWxW7fuA7FURLeJBnG52K+pWrjJvO7OZLnH3K3BbWnN3my0hGmEP+o/FvvAVsfPD3zrtP32y1eYD1XDVz3s5iKrwDIEpVbMuefjaE31h/4ysUaSyPOlLZE7I+0KREexQYA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Received: from DM4PR11MB7303.namprd11.prod.outlook.com (2603:10b6:8:108::21) by DS7PR11MB7689.namprd11.prod.outlook.com (2603:10b6:8:e2::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6521.26; Wed, 28 Jun 2023 08:39:41 +0000 Received: from DM4PR11MB7303.namprd11.prod.outlook.com ([fe80::e57e:3584:99ad:d384]) by DM4PR11MB7303.namprd11.prod.outlook.com ([fe80::e57e:3584:99ad:d384%7]) with mapi id 15.20.6521.023; Wed, 28 Jun 2023 08:39:35 +0000 From: "Aktemur, Tankut Baris" To: Andrew Burgess , Pedro Alves , "gdb-patches@sourceware.org" Subject: RE: [PATCH 2/2] gdbserver: track current process as well as current thread Thread-Topic: [PATCH 2/2] gdbserver: track current process as well as current thread Thread-Index: AQHZd33wCsp7lr1F7EiU08OZaT3L+q89H65QgFpVPwCACNKhEA== Date: Wed, 28 Jun 2023 08:39:35 +0000 Message-ID: References: <20220419224739.3029868-1-pedro@palves.net> <20220419224739.3029868-3-pedro@palves.net> <87v8hkawun.fsf@redhat.com> <87o7l7s7y3.fsf@redhat.com> In-Reply-To: <87o7l7s7y3.fsf@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: DM4PR11MB7303:EE_|DS7PR11MB7689:EE_ x-ms-office365-filtering-correlation-id: 47d56a2d-ff3a-4465-3dd8-08db77b333aa x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: k4UzP3ZPdXDjavZwW8vcYgkxMX+bt9VqRyMrEtSP0j/LflvUsJsogKskYbgDssv9b2DjDnAYx0S36ktG2uK16Ao5wSpMPL1lTpOQZVt+RKusVIDHRMhsbIybi+Fk/g6AuWRU/sU6nb+Vu92gmv9ksZV2coKVLlqggKP0lxEOzp8pupYpq0qHwxGikmxWT7pwrEQw4hzBZuxKpjas22+IojP5ZTRkxv4m6snX1IjWMO6k4GujAUOoNySn15q3Qbop8vPbaK4nVliO9kxinssF3YMFCDfxa5tKauZQ+sfRYzfZRFoKhPaordxEe5+2oisb/NsHqCmpZtxBrYH7O/Oy5HFiR7jZWk2xI6G0KkXf7uUnv/8fICcGzGJZO3d5SXDIFss1fYu/Yi39UHneazEFtZGQ7XoXf7e3ImVVBKZtX8jD2EqGy52UFTglivyyGAwSP2zVkHB2+WKSyk/SFxx3Nd6CSlPINyhFboEDbfh9HbP8IK9Gkis2plE6v/8UW3X9mE9kHyzanZfADMWxkMxUfht9ROJkPekNkQLyYwc1negH5PjjQWHZABOMDJlXrh59gowBbxfx92hKTF5hVZj0ZF9d/Fhgo1mU6pfuob933vM= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM4PR11MB7303.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230028)(396003)(39860400002)(136003)(376002)(366004)(346002)(451199021)(84970400001)(52536014)(5660300002)(66946007)(76116006)(33656002)(66556008)(64756008)(66446008)(66476007)(478600001)(316002)(8936002)(8676002)(2906002)(110136005)(55016003)(38070700005)(41300700001)(71200400001)(186003)(7696005)(86362001)(26005)(6506007)(53546011)(9686003)(122000001)(38100700002)(82960400001)(83380400001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Cbq1pKUNPrDFdnspbcPhXr0M8DARzhh9UIytMyyxDiehOf0IaDt3WJqEeurt?= =?us-ascii?Q?6RlvBDdrHrzoUOyQrMbvIf3Q+Rj8gX6AxMeAcnby0Lk8erPEJUmfTrtzhZ7L?= =?us-ascii?Q?AFPPKZq2vZWhMc+dXMa+QRoSYKOoj6sw4zHRnCQ11Up97CRfIuLh8GvKP0Uq?= =?us-ascii?Q?oo4oyyPXsOPkze74DjRiBUCEI6xfcuCjHJT/5mPCfTdyHVqUje7J1oeDj/I1?= =?us-ascii?Q?M6GDjqKxoBaEpQ4yaWBOHkw7R72hmDzJ7lBD57daVTWCmFjqFz3+Dq97Gmur?= =?us-ascii?Q?2OoYWqN3cCXeuf7TzAQx6eqUNFDRZmRFrxSFoLHcoXr8/b1r5QbufyTt8+Ic?= =?us-ascii?Q?GdtqKrhgt7TXHzX/p195Y3d2JsOmaCq3+rqjslzB11/eHbyjBGg589Pi3msd?= =?us-ascii?Q?ZmAiJ8yN77eRNNlBttGXk0Eu4zHWNDiokQ7MRomx1dAlKxdiXPxeZYcAh3FL?= =?us-ascii?Q?J545JOAauQNZWI/aDjYI5M+cefkwWl1oK3q8eFXKzN3BBuDC6ajfKkuMUywi?= =?us-ascii?Q?7KoffuGZGNzEPJw+Dsa7BIEyElCTgl+eS5PbsoRohbWD09f7efYhVVrcjNbh?= =?us-ascii?Q?m0QOPS592YnN+MhbLhH9Ta5zQaPaSqd9LzUPwQlOyAbxMokV7tJ0xTrHwdEr?= =?us-ascii?Q?xpJ7dCOpTOQ866RV9gysr9botozWTOoYJ14e4tDlUgqln8YSjerka5OXyfzI?= =?us-ascii?Q?ZXKxdgeJkeahlM9Umh8CGODxK+2cuU6c9wwdTU2j971QH92gnBBySdMiTF9V?= =?us-ascii?Q?MkaId9lXiKF99EsY4S/I7JbjLTLGnOvn80BgSQp0jJsmxAtxzKWCAIvLb1v/?= =?us-ascii?Q?L1zoAYPysCk8XNj8Zkj1F6lA9zljme9RtQ0+wLBqQ4Y547FQpRm1z6H6TG2w?= =?us-ascii?Q?vrJplIJ2nHWiIXCTU6GdvEiu/JLpLGgynNbf6APjlsSmIcBqGorGFfJvllf3?= =?us-ascii?Q?0MhXrt3/zk2GUt/W80H7CZk4AsJmEs4naqZjIoTExn9VMaftQp2fWiNVFaE0?= =?us-ascii?Q?9dsM8lXfG6Y+cH6y/3KEQtn/gZc3EUfbLY1RArxfJ7gDMkv91ck8J5Lnrubw?= =?us-ascii?Q?m0Uqm67wz1nzOTPKpTJm9PezxQH59fuglR+lsT7fmJEH/2VEk6rXDr+jAPA9?= =?us-ascii?Q?yPY/Tgckw66lc7/YiQ44vNBm/qihrKoAtEuroICRWw36sEFzOxsssqlzlrMt?= =?us-ascii?Q?0o882flgri2HyCetvlu6jRXMlr+uqochjxsLNAiQg18BruF2vdVX195DD4vj?= =?us-ascii?Q?g/gKoVh95tHALJIrcSvEUNnVtw53NNAtBaR1JxDA3TlxGbwKZ5pnrM16p2ZO?= =?us-ascii?Q?RxupQVjW8lmHlie70q9veAOCMck2YDQU6iwX3mwv6eNhLM04ZSfQBaIcs1Lr?= =?us-ascii?Q?fdeGVyD1BaZ3FkE4BrjdDzGD4xFR5DsahAN5CTGFGYR1tBYwyFCf08lqF8aR?= =?us-ascii?Q?cnDe8Lq28jk7gc2a7pDAl0EdwjpNMmW8VxMy4wkrotdW3Pd0QwuvMnOqFWZP?= =?us-ascii?Q?PG2puTqHH+oPWiL2PopzqZn0f2Osf7pKbYrZJ4KmQn22sKz/HSY3clppA0En?= =?us-ascii?Q?6l5b6gkD8ZiwT0Ga/shMA1vkUlWjffXGnMxZ+6V+v2zxfZwhDLto4qxS6ybX?= =?us-ascii?Q?pw=3D=3D?= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB7303.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 47d56a2d-ff3a-4465-3dd8-08db77b333aa X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jun 2023 08:39:35.2437 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Tu2mYaJz9nlkfzCxk+04Kl4VKZepUBG8Qo1fWnNwohj5Ds8aPtXj/ZsNZib6W07ZAblUt+fj5+dy/GI16xTIdYO/1YuAQOoQiK/F+Fu7GJA= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB7689 X-OriginatorOrg: intel.com Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: On Thursday, June 22, 2023 7:50 PM, Andrew Burgess wrote: > "Aktemur, Tankut Baris" writes: > = > > On Tuesday, April 25, 2023 3:57 PM, Andrew Burgess wrote: > >> Pedro Alves writes: > >> > >> > The recent commit 421490af33bf ("gdbserver/linux: Access memory even > >> > if threads are running") caused a regression in > >> > gdb.threads/access-mem-running-thread-exit.exp with gdbserver, which= I > >> > somehow missed. Like so: > >> > > >> > (gdb) print global_var > >> > Cannot access memory at address 0x555555558010 > >> > (gdb) FAIL: gdb.threads/access-mem-running-thread-exit.exp: non-sto= p: access mem > (print > >> global_var after writing, inf=3D2, iter=3D1) > >> > > >> > The problem starts with GDB telling GDBserver to select a thread, via > >> > the Hg packet, which GDBserver complies with, then that thread exits, > >> > and GDB, without knowing the thread is gone, tries to write to memor= y, > >> > through the context of the previously selected Hg thread. > >> > > >> > GDBserver's GDB-facing memory access routines, gdb_read_memory and > >> > gdb_write_memory, call set_desired_thread to make GDBserver re-select > >> > the thread that GDB has selected with the Hg packet. Since the thre= ad > >> > is gone, set_desired_thread returns false, and the memory access > >> > fails. > >> > > >> > Now, to access memory, it doesn't really matter which thread is > >> > selected. All we should need is the target process. Even if the > >> > thread that GDB previously selected is gone, and GDB does not yet kn= ow > >> > about that exit, it shouldn't matter, GDBserver should still know > >> > which process that thread belonged to. > >> > > >> > Fix this by making GDBserver track the current process separately, > >> > like GDB also does. Add a new set_desired_process routine that is > >> > similar to set_desired_thread, but just sets the current process, > >> > leaving the current thread as NULL. Use it in the GDB-facing memory > >> > read and write routines, to avoid failing if the selected thread is > >> > gone, but the process is still around. > >> > > >> > Change-Id: I4ff97cb6f42558efbed224b30d5c71f6112d44cd > >> > --- > >> > gdbserver/gdbthread.h | 1 + > >> > gdbserver/inferiors.cc | 26 +++++++++++++++++++------ > >> > gdbserver/server.cc | 4 ++-- > >> > gdbserver/target.cc | 44 +++++++++++++++++++++++++++++++++++++++= +-- > >> > gdbserver/target.h | 15 +++++++++++++- > >> > 5 files changed, 79 insertions(+), 11 deletions(-) > >> > > >> > diff --git a/gdbserver/gdbthread.h b/gdbserver/gdbthread.h > >> > index 10ae1cb7c87..8b897e73d33 100644 > >> > --- a/gdbserver/gdbthread.h > >> > +++ b/gdbserver/gdbthread.h > >> > @@ -252,6 +252,7 @@ class scoped_restore_current_thread > >> > > >> > private: > >> > bool m_dont_restore =3D false; > >> > + process_info *m_process; > >> > thread_info *m_thread; > >> > }; > >> > > >> > diff --git a/gdbserver/inferiors.cc b/gdbserver/inferiors.cc > >> > index 678d93c77a1..3d0a8b0e716 100644 > >> > --- a/gdbserver/inferiors.cc > >> > +++ b/gdbserver/inferiors.cc > >> > @@ -26,6 +26,11 @@ > >> > std::list all_processes; > >> > std::list all_threads; > >> > > >> > +/* The current process. */ > >> > +static process_info *current_process_; > >> > + > >> > +/* The current thread. This is either a thread of CURRENT_PROCESS,= or > >> > + NULL. */ > >> > struct thread_info *current_thread; > >> > > >> > /* The current working directory used to start the inferior. > >> > @@ -130,6 +135,7 @@ clear_inferiors (void) > >> > clear_dlls (); > >> > > >> > switch_to_thread (nullptr); > >> > + current_process_ =3D nullptr; > >> > } > >> > > >> > struct process_info * > >> > @@ -153,6 +159,8 @@ remove_process (struct process_info *process) > >> > free_all_breakpoints (process); > >> > gdb_assert (find_thread_process (process) =3D=3D NULL); > >> > all_processes.remove (process); > >> > + if (current_process () =3D=3D process) > >> > + switch_to_process (nullptr); > >> > delete process; > >> > } > >> > > >> > @@ -205,8 +213,7 @@ get_thread_process (const struct thread_info *th= read) > >> > struct process_info * > >> > current_process (void) > >> > { > >> > - gdb_assert (current_thread !=3D NULL); > >> > - return get_thread_process (current_thread); > >> > + return current_process_; > >> > >> A bit late I know, but I wonder if the assert, or something similar, > >> should have been retained here? > >> > >> The comment on this function currently (though Baris has a patch > >> proposed to change this), says this function should only be called in a > >> context where the result will not be nullptr. Given that, not of the > >> (many) existing uses check the return value of this function against > >> nullptr. > >> > >> Happy to roll a patch to add the assert back, just wondered if there w= as > >> a reason for its removal. > >> > >> Thanks, > >> Andrew > > > > Hi Andrew, > > > > I think the current process can in fact be null in some brief periods. > > For instance, in 'handle_detach' there is > > > > if (extended_protocol || target_running ()) > > { > > /* There is still at least one inferior remaining or > > we are in extended mode, so don't terminate gdbserver, > > and instead treat this like a normal program exit. */ > > cs.last_status.set_exited (0); > > cs.last_ptid =3D ptid_t (pid); > > > > switch_to_thread (nullptr); > > } > > > > So, if the current process exits, gdbserver reports the event to GDB and > > sets the current thread to nullptr, which also sets the current process > > to nullptr. > > > > I believe an invariant is this: > > > > (current_thread =3D=3D nullptr) || (current_process () =3D=3D get_thr= ead_process > (current_thread)) > > > = > I don't think this really addresses my question. Here's how I > understand things: > = > 1. Before this patch: > a. Comment on 'current_process' says: it's an error to call this > function when no thread is selected, and > b. The function asserted that a process was selected, > = > 2. This patch removed the assert, but left the comment unchanged, > = > 3. A patch was proposed to updated the comment, > = > 4. I couldn't see any reason in this patch _why_ the assert was > removed. > = > I agree that the process _could_ be nullptr, but it _could_ have been > nullptr before. > = > My question is: did something change such that there is now a location > where we might choose to call current_process when no thread is > selected? With Pedro's patch above, before reading/writing memory upon GDB's request, gdbserver does a `switch_to_process`. This sets current_thread to nullptr while setting current_process_ to non-null. At that moment, calling current_process would have violated the assertion. -Baris > = > Given the description of the original patch my guess was no, but maybe I > should just add the assert back locally and do a test run to find out? > = > Thanks, > Andrew Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva = Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928