From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by sourceware.org (Postfix) with ESMTPS id 37110380DF9A for ; Tue, 28 Jun 2022 13:59:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 37110380DF9A X-IronPort-AV: E=McAfee;i="6400,9594,10391"; a="307229625" X-IronPort-AV: E=Sophos;i="5.92,227,1650956400"; d="scan'208";a="307229625" Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 28 Jun 2022 06:59:24 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.92,227,1650956400"; d="scan'208";a="767178792" Received: from fmsmsx606.amr.corp.intel.com ([10.18.126.86]) by orsmga005.jf.intel.com with ESMTP; 28 Jun 2022 06:59:24 -0700 Received: from fmsmsx604.amr.corp.intel.com (10.18.126.84) by fmsmsx606.amr.corp.intel.com (10.18.126.86) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27; Tue, 28 Jun 2022 06:59:23 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx604.amr.corp.intel.com (10.18.126.84) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.27 via Frontend Transport; Tue, 28 Jun 2022 06:59:23 -0700 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.106) by edgegateway.intel.com (192.55.55.68) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2308.27; Tue, 28 Jun 2022 06:59:22 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=YZr3jD1wDATujeKZG+LP2E9+FAxeqWcUm6VWghXUCd/LC5cEXPHBI3fwm1iQQTcvMKyyNIY6MxW+z41Ic4K6257uBLEMzyJr6RbVWHsIHP3hT30wwOGQE8VlVSNCaJhAthEC6xmIj9WyMjx8Tq+7fC+IJWwdbKsii9WfFyON+3PpEd9B2tMd/9ESwp03uK1L7wAKyVXdJzsX/E/wz9bdAGwZ8Z2sAnqaMBt6F1qZ837wjRsVau0YYxYfilirQqnv+ZjKbXIa0uqCQzhoOuiTvrWsSH4FIfT416EzprcjjhpHNr8Fk2mX5azEXXB0JU5nV4wKk3jyGU/GV3uR/O3E6g== 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=c44HK3ARphSdafGSdKZdrDL6iUQni7b+DEZY6IPmlts=; b=BYHzB2BMo+v2T0mqpup4WVqmY4m/GEj9TF01MiI09j2Sviek+C46XVolkcf0jyyHx3E5icywsmdyyVh8RAeerbr3cHiJ67DcSE2dkqG47cUN/LVcHY2pVG+/OEvSJsWM3SIFrJ6V9oUGBVN3PK6ZyANXxoWSphqIMsgqp5ytXMJHgrE3nfK55zFRbP4169QUdZ3Uj7HUKY4I5F5g0bTYWIXc88TqVkXee9QqK+DmYPLOFhOjUEL6PYTEVeBcVMDhHsL4L5CDuvA42oY8/a+8PATOyeZunmkflBhEi+KaLtr5sftYScg1UIjDOeiPm+rQdclw80yY2NiK+l96AWURDg== 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 BY5PR11MB3960.namprd11.prod.outlook.com (2603:10b6:a03:185::30) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5373.18; Tue, 28 Jun 2022 13:59:21 +0000 Received: from DM8PR11MB5749.namprd11.prod.outlook.com ([fe80::f9d8:e8a8:94ec:58db]) by DM8PR11MB5749.namprd11.prod.outlook.com ([fe80::f9d8:e8a8:94ec:58db%6]) with mapi id 15.20.5373.022; Tue, 28 Jun 2022 13:59:21 +0000 From: "Metzger, Markus T" To: "Willgerodt, Felix" , "gdb-patches@sourceware.org" Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration. Thread-Topic: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration. Thread-Index: AQHYhi3CNcWdISaVfEuwssdNAP0kAq1k1sEA Date: Tue, 28 Jun 2022 13:59:20 +0000 Message-ID: References: <20220622114340.55830-1-felix.willgerodt@intel.com> <20220622114340.55830-10-felix.willgerodt@intel.com> In-Reply-To: <20220622114340.55830-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.6.500.17 dlp-product: dlpe-windows x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 7bf96c67-3d2a-4463-8d75-08da590e667d x-ms-traffictypediagnostic: BY5PR11MB3960:EE_ x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: z+UKkZLnR9M0tFLns3xczN3vNXsmewy4iqXwcJw3nQkdkcugDMIhBNJZWItI1a9iiddqubasvHwVHMdvXcEDJvFVe4esDU58WfwUG65MFT2DZLzHF3OuiS01LFZggujjZSIiT177T9TSK/dbweMrjoX5618JzQZCtvni9yhPyt0tTMOOfscI0QdVLFlQ/4/UUfezocFauS+MoLvaGc5ZClB/HyY7NOxnF0Rfh/atnAOt8imG8Dg7OLl/xOk2DM6mGYPTjGvrEkhE4aVEH2jpBd923RmGZGpvkItoixQJW47LAbdf1M71X13rrnaJU+RJalgm6sTEIWIAKUFCdt8FxEelwOsCZi5BjGJwGW80Py22XWrCJuwBGlqIYY6ddSUUpi6bn2mRc8YxoOvw/LU8as2V/bLjG67Qo2G59LSuzvWb2zE2ZE1riP9RGPyoabUUoP8aWheBtEw1FiLnr8ryNLrZvLE8EHLx3VT1cKlv1YmpCAco8tnemLm8UwJ13PJ7gNCwdyayDZUi47memIPryZeH3vOhcaPikQCADN7yFxeeqUKJnkrrEGTztOD5voXRj1aUwzknzM3MOy/ARdjf10L7wUQXPjyZ9UInmxO3HapojqJirE08dLIU8jPaVTsFAlyg6SZ+5eAU4lMca07yXDeQjlUdFFtShGaOvDSk4/oJrzPL9JRUcUHtxHalaYDG1smV78IWRyZ4Uc2Iy6jUo0myoUR+mvGdp5C27SXQtqbyGLsm4XqwuySAL9jPa+F6xPPk20+GiJ9k45IcEGqnRsioczFDgg62qpkKs2cXF6I= 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:(13230016)(346002)(366004)(136003)(396003)(39860400002)(376002)(26005)(186003)(6506007)(41300700001)(8676002)(316002)(7696005)(86362001)(66556008)(38070700005)(82960400001)(76116006)(9686003)(52536014)(8936002)(83380400001)(33656002)(5660300002)(64756008)(110136005)(2906002)(71200400001)(66476007)(122000001)(38100700002)(66446008)(66946007)(55016003)(478600001); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?cXmTnRrb6JSipDVWQ9oXbFxcw5MeWSAtIol+Dw6R++EnMIDr+BRfSAx5Uata?= =?us-ascii?Q?1T86v9RRrRbeWbyx0OYIgyYU6pN42u12WrtAYNl1ZnaHvZIRwDzFInqOPRW2?= =?us-ascii?Q?MkUPRVA3dXRzD4kpY10Xp+7vVuuM6kal0Zw+/bVqlH2wPOGy441pdsYMXem/?= =?us-ascii?Q?PfU/1WUqtrBDR21mmc2IlpXoHwaqjwoQgvYYjxnVXEgi1u0UVsnsXupTBCKd?= =?us-ascii?Q?s8HpctxJBPDW44Aa3jqZAFixwrmUXBQWRJX6eNc5jhvH3bP2h+LynzOaxkJN?= =?us-ascii?Q?+QoNlVQhFeFcoKbcokYqRDy3qr+Q2779cKB/mhE3baFRwJHRJ+KjtNHwu+hk?= =?us-ascii?Q?J92lXg+0obY2pXpUS/WJ6isQLIMAxWIMR7xMZqgrQ6t9wIUfI6UUoGhK1NeV?= =?us-ascii?Q?cZ73Oa0CeqpxheE5BYNkBiEol/MGY8USgcly6yGfjvuCxw+pdxHGHdhAejKZ?= =?us-ascii?Q?jro+8OvfAD8mVP70B+BYW822dyg/kgGMPl6Po6UkKc5wy3b3jkqlNOsrHHrx?= =?us-ascii?Q?GBJhSAyOLnGWWo1jrqaj2I5bqFKIZiPtc82x4cwYR4Y1j9qLZowcY6WPJX3L?= =?us-ascii?Q?3fMiDDVAchn8VpmBxzUsMoVfPoyoefjHBctjPOtL6xn76cmGlWd4hOUYsqoV?= =?us-ascii?Q?yPyk7EGc4aBYvYWGPDP2E9kQe9FA7JhrTXiMZuCfsVerXRApqrD83IZv3WZM?= =?us-ascii?Q?C1vSj7dLBPicWymhSNOgEXZr8jr2mu4nEGaQEJl7QUMv/JzuYE2WaLaaMlTw?= =?us-ascii?Q?AL0r5vsrTlvNE7f8qXNoWe8HstnryqnDSXtSvrFP8DL2ICylmcUYebKy8kvA?= =?us-ascii?Q?o/DBQMfes1tK5aoXpMcUOHfhh456T55BoQqF5nG52+TWeROXt9K9z9N1NrB3?= =?us-ascii?Q?3/zBh8JMD6pzMBEwfmQ16Y2Bdhe+CsgbW8Fm9trdVFkDPTFZ0mBJpI5iBanc?= =?us-ascii?Q?HeExJnzd/KlkqnmTedwpjwC83XjsAQlWDfqwt12ooLe8bTAGuGdcoopqNRC1?= =?us-ascii?Q?wsqdgkH632NylrcP3mY2wfG+qLDt9RI2BklpmA9edlmR2QP2A6iHxaKRpxTB?= =?us-ascii?Q?1xFZEzctNQKs4RWaC3Fsv3YpIZnTy3d+Y97bUdOS5zkuckFsDD4ncVjUPVL1?= =?us-ascii?Q?c0y8PN8yVTHl51D4BuLQ/LmRmxruDsfwabG+IYO/+YjciV4c+fNGexU8i2bE?= =?us-ascii?Q?qM1nDEPZ/7tQ/l/ldxbF0mA/U4EG8++NHjP7yKPg+OhxGFsR/wg22mH0O3Dx?= =?us-ascii?Q?NHC4WM1t9kOGShfneUl/EcRqTn+cEqgt5HBVd2AycZfh+Kn0oTDTZbrAtxJW?= =?us-ascii?Q?oL6Tc3qmgJkP0qlRnHgFsHUHwmm+P8bymzer7UMQMN8R+9kdGiEglNSlKDBJ?= =?us-ascii?Q?P37f1k0iR7JJbgsJRhPEy8FHeRHRjnQenETx7iMqq6mXQ1nMsRyuZMVUZ+EU?= =?us-ascii?Q?CZ5Dm8KYAhJ7YzduIaJdM+hcSvsepweE4uKF8Cf4IOga0zL+68fgdLByXOH2?= =?us-ascii?Q?u7o/ptJNLxoMvjBZ0yWJGcyQgZFNXRATy3U2U6Fq55HZ5FCRN2/6DzoHdw3z?= =?us-ascii?Q?vFHVbQSvry+r898hZ6OHimpb3qjCPdiiDpUP/69h?= 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: 7bf96c67-3d2a-4463-8d75-08da590e667d X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jun 2022 13:59:21.0108 (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: +5YJUevp9k4DTvZK+HpK7JlaIB/eURfITacXlKmzvDbDv1CTrgvJB2/cYa6YmgipKRsdIj8GPmXHJwnXb6SxBMKRVQc9klbktgXk8kd5XN8= X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3960 X-OriginatorOrg: intel.com Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-10.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, 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 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: Tue, 28 Jun 2022 13:59:29 -0000 Hello Felix, >With this patch a default ptwrite filter is registered upon start of GDB. >It prints the plain ptwrite payload as hex. The default filter can be >overwritten by registering a custom filter in python or by registering >"None", for no output at all. Registering a filter function creates per >thread copies to allow unique internal states per thread. >--- > gdb/btrace.c | 3 + > gdb/btrace.h | 9 +++ > gdb/data-directory/Makefile.in | 1 + > gdb/extension-priv.h | 5 ++ > gdb/extension.c | 13 ++++ > gdb/extension.h | 3 + > gdb/guile/guile.c | 1 + > gdb/python/lib/gdb/ptwrite.py | 86 +++++++++++++++++++++++++ > gdb/python/py-record-btrace.c | 111 +++++++++++++++++++++++++++++++++ > gdb/python/py-record-btrace.h | 8 +++ > gdb/python/python-internal.h | 3 + > gdb/python/python.c | 2 + > 12 files changed, 245 insertions(+) > create mode 100644 gdb/python/lib/gdb/ptwrite.py > /* For maintenance commands. */ >@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo, > uint64_t offset; > int status; > >+ apply_ext_lang_ptwrite_filter (btinfo); A comment would be nice, here, to explain what this does. >+ /* Function pointer to the ptwrite callback. Returns the string return= ed >+ by the ptwrite filter function or nullptr if no string is supposed to >+ be printed. */ >+ std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t= ip, >+ const void *ptw_filter); The comment doesn't match the code. It cannot return nullptr. Why to we call the void * parameter ptw_filter instead of the usual context? We probably want to call the callback itself ptw_filter and the void * argu= ment context. We also seem to mix the terms ptwrite callback and ptwrite filter. >+ >+ /* Function pointer to the ptwrite filter function. */ >+ void *ptw_filter =3D nullptr; Is this the function or some context for the function? >diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c >index 14b191ded62..86f92a476af 100644 >--- a/gdb/guile/guile.c >+++ b/gdb/guile/guile.c >@@ -124,6 +124,7 @@ static const struct extension_language_ops >guile_extension_ops =3D > gdbscm_apply_val_pretty_printer, > > NULL, /* gdbscm_apply_frame_filter, */ >+ NULL, /* gdbscm_load_ptwrite_listener, */ This should probably be called ptwrite_filter. >+def default_filter(payload, ip): >+ """Default filter that is active upon starting GDB.""" >+ return "{:x}".format(payload) >+ >+# This dict contains the per thread copies of the filter function and the >+# global template filter, from which the copies are created. >+_ptwrite_filter =3D {"global" : default_filter} Why those leading underscores? >+def _update_filter_dict(thread_list): >+ """Helper function to update the filter dict. >+ >+ Discards filter copies of threads that already exited and registers >+ copies of the filter 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 filters >+ for key in _ptwrite_filter.keys(): >+ if key not in lwp_list and key !=3D "global": >+ _ptwrite_filter.pop(key) >+ >+ # Register filter for new threads >+ for key in lwp_list: >+ if key not in _ptwrite_filter.keys(): >+ _ptwrite_filter[key] =3D deepcopy(_ptwrite_filter["global"]) This function is called two times: once after we cleared all filters, and once when looking up the filter for a given thread. The first time, we know that there are no existing filters; the second time, we are really only interested in a single filter. Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't exist, create a new one? That leaves removing obsolete filters. Could this be done with some thread notification? >+def _clear_traces(thread_list): >+ """Helper function to clear the trace of all threads in THREAD_LIST."= "" >+ current_thread =3D gdb.selected_thread() >+ >+ recording =3D gdb.current_recording() >+ >+ if (recording is not None): Let's remove the empty line to group the statements. >+/* Helper function that calls the ptwrite filter PTW_FILTER with >+ PAYLOAD and IP as arguments. Returns a pointer to the string that will >+ be printed or nullptr if nothing should be printed. IP can be nullptr, >+ PAYLOAD must point to a valid integer. */ >+std::string >+recpy_call_filter (const uint64_t payload, const uint64_t ip, >+ const void *ptw_filter) The comment doesn't match the code. It cannot return nullptr. Also, IP and PAYLOAD are integers, not pointers. I think we can shorten "calls the ptwrite filter PTW_FILTER" to just "calls PTW_FILTER". >+{ >+ std::string result; >+ >+ if ((PyObject *) ptw_filter =3D=3D Py_None) >+ return result; >+ else if ((PyObject *) ptw_filter =3D=3D nullptr) >+ error (_("No valid ptwrite filter.")); No need for else. Should we check nullptr first? >+/* Helper function returning the current ptwrite filter. Returns nullptr >+ in case of errors. */ >+ >+PyObject * >+get_ptwrite_filter () >+{ >+ PyObject *module =3D PyImport_ImportModule ("gdb.ptwrite"); >+ >+ if (PyErr_Occurred ()) >+ { >+ gdbpy_print_stack (); >+ return nullptr; >+ } Do we want to print the stack without throwing an error? >+/* Used for registering the default ptwrite filter to the current thread.= A What does 'default' mean, here? We're registering the callback that btrace calls on PTW. From btrace's perspective, this is the ptwrite filter. From python's perspective, this is maybe a proxy for the python filter. >+ pointer to this function is stored in the python extension interface. = */ >+ >+void >+gdbpy_load_ptwrite_filter (const struct extension_language_defn *extlang, >+ struct btrace_thread_info *btinfo) >+{ >+ if (!gdb_python_initialized || btinfo =3D=3D nullptr) >+ return; Isn't this an error or even an internal error case? >+/* Callback function for the ptwrite filter. */ >+extern std::string recpy_call_filter (const uint64_t payload, >+ const uint64_t ip, >+ const void *ptw_filter); Should the comment say something like 'proxy for the python ptwrite filter'? 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