From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by sourceware.org (Postfix) with ESMTPS id 6DF45385B837 for ; Fri, 13 Aug 2021 10:36:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6DF45385B837 X-IronPort-AV: E=McAfee;i="6200,9189,10074"; a="202699787" X-IronPort-AV: E=Sophos;i="5.84,318,1620716400"; d="scan'208";a="202699787" Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Aug 2021 03:36:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.84,318,1620716400"; d="scan'208";a="674272700" Received: from orsmsx604.amr.corp.intel.com ([10.22.229.17]) by fmsmga006.fm.intel.com with ESMTP; 13 Aug 2021 03:36:18 -0700 Received: from orsmsx610.amr.corp.intel.com (10.22.229.23) by ORSMSX604.amr.corp.intel.com (10.22.229.17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10; Fri, 13 Aug 2021 03:36:17 -0700 Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) 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.2242.10; Fri, 13 Aug 2021 03:36:17 -0700 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.10 via Frontend Transport; Fri, 13 Aug 2021 03:36:17 -0700 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.103) 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.2242.10; Fri, 13 Aug 2021 03:36:15 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=j3ivJVb7kbZpGWpLsf2G//uCmRpj95l9Fiyb3VCAgs2AlYAluleECqQH4Iw95UNv6zztAIwYZ9RTYFt889o1me8ZwvYHveRI59fLMttuoiYT7zVwQAUckIdjIS/vBIS9L9GeZvwnF+4NYIPbfdWp1jq7owdX7VbMtbb8401Se8Z/FMEWboRreA+V7uEhIh4EsfGRkF1NfRI9ZmAZUlhFZc1GMzqFElcEllAIVxwtCuwynVgdrLMSKM1E2GAfKxuKIKFMaPE3s8itAGd4DRqcrXjX7ifn1V0V7966TKtcWHxBlZO9Ood970DCfMQ8t/WWIYocescIXUGQ7sXn5zQQYA== 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-SenderADCheck; bh=Xt0DMQwoe2UsNUYomvGZpl8o22wKdJHLapW1fK5kYKo=; b=dorsr5BMjDdx0onrVI/L3AQaa0vx6LNGsz9wCo82FRnn5j1Xq24lScqjS6GJ32NUlRdjB8tSLg9+Cw5syrHQaOWSXyYaa93H3WA/1L6Q8LY7N00W4CttkcvQ0pyF/F7iL30dv/IJ3MEqxogRMsqPmUiwb1HPILErwFKc7YexZLrxHDPDxXMedZL6k+SJ7Q5Gvy1NcxdFMXKmyUuBqz5LFvw1/gIGYKb4pttYuPOqiaBw3sz+wbCwVOWefwyS1CQKyKDcv6zsk+n6/PPiiYlbxMngwDugEjVO4w2t0ZrCUWmlDtjJCWDXQPmEe693EeDV176N7GqyzHCdK2pbxFSr4g== 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 DM8PR11MB5749.namprd11.prod.outlook.com (2603:10b6:8:10::15) by DM4PR11MB5423.namprd11.prod.outlook.com (2603:10b6:5:39b::20) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4415.15; Fri, 13 Aug 2021 10:36:12 +0000 Received: from DM8PR11MB5749.namprd11.prod.outlook.com ([fe80::392c:516a:cc52:963]) by DM8PR11MB5749.namprd11.prod.outlook.com ([fe80::392c:516a:cc52:963%9]) with mapi id 15.20.4415.016; Fri, 13 Aug 2021 10:36:12 +0000 From: "Metzger, Markus T" To: "Willgerodt, Felix" CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH v3 09/12] btrace, python: Enable ptwrite listener registration. Thread-Topic: [PATCH v3 09/12] btrace, python: Enable ptwrite listener registration. Thread-Index: AQHXYoO//gc5wo/BZE6F8oTkwifYrqtxfvZQ Date: Fri, 13 Aug 2021 10:36:12 +0000 Message-ID: References: <20210616074205.1129553-1-felix.willgerodt@intel.com> <20210616074205.1129553-10-felix.willgerodt@intel.com> In-Reply-To: <20210616074205.1129553-10-felix.willgerodt@intel.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-reaction: no-action dlp-version: 11.5.1.3 dlp-product: dlpe-windows x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: d2517ee7-9a41-42f1-60dd-08d95e462ba7 x-ms-traffictypediagnostic: DM4PR11MB5423: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: R4Pdet3HqqdTdtMwWBrbvldBYM6JWretr2K27TGMcOBsklkEhmDeh26yJ0dL07MLRjqk1MMny9it8mrk66WGKM0TqsJIntTxt3KTZx4XvspOJqSZ1RsfOBFuodD0BWiKs0ZhluccBNfSFSiGvBgRReNXBXUS6b4jpGKojrcXne4f0Hz/T2dka23KJO5ijaKhAGO3AmMgI2Wmg0zpKlMvfD5pLpFWaaUBL1DZRmooKIPD2Tt8Q4Gz0Pe0JwvRXjfcpyJk/isLnFrBeE8XocGy2zEMJeSYqSFnY6JrBVkhZ3LhcZdkmNYF1igSDexslksKfKJ+azRBC76qtmuHvv+++mWoNdKela/NlkpvibUxWt1Bm9v6dfS9Tx3/blKucrCtWyW9mtVRGHxmGwCZHR9f/zocGHNuFW9mNvAyGzfzUWnzBDuJwpIeAkXjboJQsNyuwqsSxVQPIpRt/AO5GniNfLoieUyfc2lUJADmMSkPmM1T0RvzIW+L2jKrs7Z5L+EqLffBc3PpW7F1+3m+5wvly6cbq1QrU5c7cGvZd91t+DGGMQv6OObvmDUpKMkp+CdJIReTMfGhiKN9JuoDufAttlj9aJPi6SOTWVEXmGIYBbHspbW3dyRnKX7eUItuvoJzYFgiI+vusbxC/ooLl4qdkCSt9QH541eOmhewfqhyft4kyINANRA5q7IBykD+s4MEBPbBOZpSJCcTjP79K2kBF28pcqrt1wavGUNGSQ1Ik5E= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM8PR11MB5749.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(376002)(39860400002)(366004)(346002)(396003)(136003)(52536014)(76116006)(38100700002)(7696005)(71200400001)(66446008)(6636002)(66556008)(66476007)(122000001)(64756008)(66946007)(478600001)(316002)(2906002)(86362001)(33656002)(4326008)(6862004)(8936002)(8676002)(5660300002)(186003)(83380400001)(26005)(9686003)(38070700005)(6506007)(55016002)(2004002); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?aHLr9KBgQd8J6V2LIvVUvt4tnwdpKosySv6RygDS31J2P3NjwVRKIONZIoLQ?= =?us-ascii?Q?hgzX1NAnzn8MVnL8thB8SCvLp0qs8ekm0k/1TCsKLQ2KzDEZqljhDxSZ69kz?= =?us-ascii?Q?tUTespYIR/+FSwsW4z2NUa0ERvbZrwFUfNSH7MGCrxrjCacMF0WFgTo1JUoV?= =?us-ascii?Q?9s8n9ZuxTJBeqKXOd9HbSnaHoEMwLEUos9lbXLrIcOuj6pnK3H7vtesrCWNP?= =?us-ascii?Q?YhFhsSMKzjrSyit+puGEGcWZYWC8kRIJBKG5k687KmwXcbCafrkbWVrt/mY4?= =?us-ascii?Q?34pZuZa59P5bir9w0/SXueQOk7hAhWxP3ZOomAePJ1PrgIPkpGRH4r6QeyfU?= =?us-ascii?Q?ngelL7Lq5XpODPcpF/61FRWU5B2T0N4ePVeMww4+mUd/PHIgWf5yzE1CMpH6?= =?us-ascii?Q?5L5DQ82Go3uttMCrvOKjmqVsi1Bxlt1Bxsw8dnvN31Is4zw9R4LdyheR1iGh?= =?us-ascii?Q?NkXsPOFTDYsbWEKeHKysVHZCGS2sGvmu7HG/Uz2+NU3Z/xKdDt8sObpt8kPX?= =?us-ascii?Q?KwitV2gQANQSFhnuz1/tPPxl/7b/pA0eJvEZeZFty+9HFDpcj4OfdSEJTXH0?= =?us-ascii?Q?HcScWd4y1+scghnu9CY05fSGg77OvBxJLiIb0HQA6EMHhtAZECkmTJSA5dwW?= =?us-ascii?Q?oxVoWqZqACGhFTe8DTcXQk98SFV0h4VVYhjwF4WIifaiGgRTI7olxaR/TFCR?= =?us-ascii?Q?5hYYGBQdwDVv3NvaFKTUN/rZfAALr5CEJhM33AGK2wxKkQ0SYwBO6IxlZH2E?= =?us-ascii?Q?bjv1qeogt+BYu+PmiNvSICeO1ECUKhd/kT9GjpTFGNw8fXHSScpT+zKrVDqY?= =?us-ascii?Q?JzaYTwMcC4g5YPzYHWYc3LsyuZGAbm0+3DPrpGcfixvxYbZQBb6ej0tYbxct?= =?us-ascii?Q?/0IqkobLMWHj9BAfMqXHGbP1aP6vUiHnDOZ/zuHR+i3gvJcKj66QyIMlTG64?= =?us-ascii?Q?dYK78itQrPVgG4/EvVctCJcKKQs1sYKiVO6DriUWdAwpxo5UMRLHGfwmqeVS?= =?us-ascii?Q?3BioINTrTU8VdCjZyaguzrpnEdtftxuwcVYsqtIs6/M3KfU1shi5IBvUfasZ?= =?us-ascii?Q?HFopaB67rp5bJthb9mwVzyMCPTEhl1w3uUTrE7Jv/p/T8kJMgucbC1wGsTyL?= =?us-ascii?Q?bXcyDk7FVzH6PLnGcR5m/QHiWkYk46a2jrIaeWidNXNG+dPqrQJrZOKuh7zU?= =?us-ascii?Q?wyZbgS2bgghXGyaa9YyTubWrmB6vCFkmP3GyE+ahX7nbjl9VUvabyJuK0rG4?= =?us-ascii?Q?kM5MTc+3gNtqQXoWlrDsK6Af511gEcqoRXXKK8P3izEiW5OGvK6YWUfJsbnE?= =?us-ascii?Q?a08=3D?= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM8PR11MB5749.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: d2517ee7-9a41-42f1-60dd-08d95e462ba7 X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Aug 2021 10:36:12.1691 (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: CIlFC0kZ7COv0rfuNELU56CY9tDOuUOPnuW+Ua+CkhQqa7L10NCwii5qySDmfX/KQhwk2RzZSUsz5DAtNlkMayfeO4xMLGjC8jOC6+mUg3Q= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5423 X-OriginatorOrg: intel.com Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_NONE, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Aug 2021 10:36:31 -0000 Thanks, Felix, >+ apply_ext_lang_ptwrite_listener (inferior_ptid); Should this be called ptwrite_filter? >+ /* PyObject pointer to the ptwrite listener function. */ >+ void *ptw_listener =3D nullptr; >+ The comment says that this is a PyObject. Why is it declared void *? >diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h >index 77f23e0f911..36da4211738 100644 >--- a/gdb/extension-priv.h >+++ b/gdb/extension-priv.h >@@ -183,6 +183,10 @@ struct extension_language_ops > enum ext_lang_frame_args args_type, > struct ui_out *out, int frame_low, int frame_high); > >+ /* Used for registering the ptwrite listener to the current thread. */ >+ enum ext_lang_bt_status (*apply_ptwrite_listener) >+ (const struct extension_language_defn *, ptid_t inferior_ptid); There is a parameter name for the second but not for the first parameter. = Also, the name shadows a global variable. Let's call it just 'ptid'. >+/* Used for registering the ptwrite listener to the current thread. */ >+ >+enum ext_lang_bt_status This enum is specifically for frame filters. We'd need to either generaliz= e it or add our own for ptwrite filters. >+apply_ext_lang_ptwrite_listener (ptid_t inferior_ptid) >+{ >+ for (const struct extension_language_defn *extlang : extension_language= s) >+ { >+ enum ext_lang_bt_status status; >+ >+ if (extlang->ops =3D=3D nullptr >+ || extlang->ops->apply_ptwrite_listener =3D=3D NULL) >+ continue; >+ status =3D extlang->ops->apply_ptwrite_listener (extlang, inferior_= ptid); >+ >+ if (status !=3D EXT_LANG_BT_NO_FILTERS) >+ return status; I would have inserted the empty line after the 'continue' from the if state= ment since obtaining the status and checking it belong together. IIUC, this installs the ptwrite filter. The code that actually calls it on= a PTWRITE event is not part of this patch. I think the installation of th= e filter and calling it should be inside one patch to make it clear how thi= s works. >+# Ptwrite utilities. >+# Copyright (C) 2020 Free Software Foundation, Inc. Year. >+def default_listener(payload, ip): >+ """Default listener that is active upon starting GDB.""" >+ return "{:x}".format(payload) >+ >+# This dict contains the per thread copies of the listener function and t= he >+# global template listener, from which the copies are created. >+_ptwrite_listener =3D {"global" : default_listener} >+ >+ >+def _update_listener_dict(thread_list): >+ """Helper function to update the listener dict. >+ >+ Discards listener copies of threads that already exited and registers >+ copies of the listener for new threads.""" >+ # thread_list[x].ptid returns the tuple (pid, lwp, tid) >+ lwp_list =3D [i.ptid[1] for i in thread_list] >+ >+ # clean-up old listeners >+ for key in _ptwrite_listener.keys(): >+ if key not in lwp_list and key !=3D "global": >+ _ptwrite_listener.pop(key) >+ >+ # Register listener for new threads >+ for key in lwp_list: >+ if key not in _ptwrite_listener.keys(): >+ _ptwrite_listener[key] =3D deepcopy(_ptwrite_listener["global= "]) So we can only update all at once? In non-stop mode, I think we want to be= able to update just the one for the current thread. What's the purpose of this deepcopy? For the default filter, this doesn't = seem necessary. For user-defined filters, this is initializing the per-thr= ead filter from a template, correct? Should 'global' be spelled 'template'= in that case? Should the template filter be part of the dict, at all? >+def _clear_traces(thread_list): >+ """Helper function to clear the trace of all threads.""" The comment doesn't match the code. The function clears the recordings of = threads in THREAD_LIST. >+ current_thread =3D gdb.selected_thread() >+ >+ recording =3D gdb.current_recording() >+ >+ if (recording is not None): >+ for thread in thread_list: >+ thread.switch() >+ recording.clear() >+ >+ current_thread.switch() Should there be a try around the above to ensure we switch back in case of = exceptions? >+def register_listener(listener): >+ """Register the ptwrite listener function.""" >+ if listener is not None and not callable(listener): >+ raise TypeError("Listener must be callable!") >+ >+ thread_list =3D gdb.Inferior.threads(gdb.selected_inferior()) >+ _clear_traces(thread_list) Please add a comment as to why we need to clear the recordings for all thre= ads? >+def get_listener(): >+ """Returns the listeners of the current thread.""" The comment says 'listeners' but there can be only one, correct? >+ thread_list =3D gdb.Inferior.threads(gdb.selected_inferior()) >+ _update_listener_dict(thread_list) Why do we need to update the filters for all threads? >+/* Helper function returning the current ptwrite listener. Returns nullp= tr >+ in case of errors. */ >+ >+PyObject * >+get_ptwrite_listener () >+{ >+ PyObject *module =3D PyImport_ImportModule ("gdb.ptwrite"); >+ >+ if (PyErr_Occurred ()) >+ { >+ gdbpy_print_stack (); >+ return nullptr; >+ } >+ >+ PyObject *default_ptw_listener =3D PyObject_CallMethod (module, > "get_listener", >+ NULL); >+ >+ gdb_Py_DECREF (module); >+ >+ if (PyErr_Occurred ()) >+ gdbpy_print_stack (); >+ >+ return default_ptw_listener; This is not necessarily the default listener, correct? >+/* Helper function to initialize the ptwrite listener. Returns nullptr in >+ case of errors. */ >+ >+PyObject * >+recpy_initialize_listener (ptid_t inferior_ptid) The argument name shadows a global variable. Let's just call it 'ptid'. >+ process_stratum_target *proc_target =3D current_inferior ()->process_ta= rget (); Any chance we can get here without having a current inferior? >+ struct thread_info * const tinfo =3D find_thread_ptid (proc_target, inf= erior_ptid); >+ >+ tinfo->btrace.ptw_listener =3D get_ptwrite_listener (); Let's check tinfo. Or pass it in as parameter. >+ return (PyObject *) tinfo->btrace.ptw_listener; It is called only once and that call ignores the result. >+/* Used for registering the default ptwrite listener to the current threa= d. A >+ pointer to this function is stored in the python extension interface. = */ >+ >+enum ext_lang_bt_status >+gdbpy_load_ptwrite_listener (const struct extension_language_defn *extlan= g, >+ ptid_t inferior_ptid) >+{ >+ struct gdbarch *gdbarch =3D NULL; >+ >+ if (!gdb_python_initialized) >+ return EXT_LANG_BT_NO_FILTERS; >+ >+ try >+ { >+ gdbarch =3D target_gdbarch (); Is this a precaution or did you run into cases where target_gdbarch() throw= s? Should the rest also be done inside a try block? Regards, Markus. 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