From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by sourceware.org (Postfix) with ESMTPS id 0688C3950C59 for ; Tue, 22 Jun 2021 14:59:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0688C3950C59 IronPort-SDR: 6F/Fq7QDWdCSq5br5BJ3AbbVds4jFbj73HhLKzkLqpHJmZvovYY9oltxtGEk5tAIT1YZlCyt78 FP06kZK1a1RA== X-IronPort-AV: E=McAfee;i="6200,9189,10023"; a="187451364" X-IronPort-AV: E=Sophos;i="5.83,291,1616482800"; d="scan'208";a="187451364" Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jun 2021 07:59:27 -0700 IronPort-SDR: hWqfUtXH66YJrUq5TveHm+RIq6htzTSStSyXq4etG43XdjpnscOb9m/DCGvqW0QwRp9FcN/aHj V7AWD9cRXn3A== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.83,291,1616482800"; d="scan'208";a="417416269" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by fmsmga007.fm.intel.com with ESMTP; 22 Jun 2021 07:59:27 -0700 Received: from fmsmsx609.amr.corp.intel.com (10.18.126.89) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Tue, 22 Jun 2021 07:59:26 -0700 Received: from FMSEDG603.ED.cps.intel.com (10.1.192.133) by fmsmsx609.amr.corp.intel.com (10.18.126.89) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4 via Frontend Transport; Tue, 22 Jun 2021 07:59:26 -0700 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (104.47.55.104) 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.2242.4; Tue, 22 Jun 2021 07:59:25 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=S6LDc4qaXZmDC2I2DnUWnY+tkPplhTeWHvz7DyMF8ozx1qxipjqTOxpBsrOdMi1n+MU3cAJB9Paueop1bDJd0OPvdV9ZD80ivZPP8b1ajHHerJUSvtkXpyLc6U5GlTpD82YVHchysGt73IkeUM5lMb0XT8+zqKeoiv6sIRIQE6aCp+7HTiXeNCAKvNbavAKFavERV0P0IKNOGtIR10pIZQ80mK0V4gKpDlCc8nYwxmpOTJz86ILHqcNeUOvB53uj/rD5YfSHbLO5RbZasj+GqkmnMOkMQ6orV4psTnf+MuEvuE27NUD1WM9ibjShvzoku/GYDLM/3dWZQUg4kISZ/g== 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=YlrCEi+GiEP9xxJYeei0rTFQ01pAXTewjN0aoAkyZ1o=; b=kvuO7oxcY44Q9L0yOGjha12tN68vjlD0dPnfJZJ+HUz/Vu5qI2W7ifhCoQfBRGsum3R2Kk2o5tKNKcL+yL7Ul5a822R342WS5o8wEt/CanhrSdSWr34INiviY5b6M1X3u6UK+TDfmGaikQR98uhXDVptSD6eKKMPvX2vqRMoKS7HNjomiGpJzF9mIGuCHRhbXUttDCGRGiffAhCkVfb0ENcVMkT/FCHdRypnDM1vSFvmFrrBMjSeQTgDrZVKBrWBaK4dXQJW7fuedJnrgqBQ84fdE0uRvpcu29wxeIRB2dOCBoevtCgSC8RusyNwDP59TyEkFlCQWn/+LZryd8Njxg== 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 DM5PR11MB1690.namprd11.prod.outlook.com (2603:10b6:3:15::11) by DM5PR1101MB2233.namprd11.prod.outlook.com (2603:10b6:4:4f::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4242.23; Tue, 22 Jun 2021 14:59:24 +0000 Received: from DM5PR11MB1690.namprd11.prod.outlook.com ([fe80::b0e3:d229:174e:cc45]) by DM5PR11MB1690.namprd11.prod.outlook.com ([fe80::b0e3:d229:174e:cc45%9]) with mapi id 15.20.4242.023; Tue, 22 Jun 2021 14:59:24 +0000 From: "Metzger, Markus T" To: Zied Guermazi CC: "gdb-patches@sourceware.org" Subject: RE: [PATCH v6 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant Thread-Topic: [PATCH v6 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant Thread-Index: AQHXVmSnhc3zzHkYpUik2ndVTLpGDKsgG1tQ Date: Tue, 22 Jun 2021 14:59:24 +0000 Message-ID: References: <20210531213307.275079-1-zied.guermazi@trande.de> <20210531213307.275079-4-zied.guermazi@trande.de> In-Reply-To: <20210531213307.275079-4-zied.guermazi@trande.de> 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-originating-ip: [134.3.204.142] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 12d94750-a451-43e7-970a-08d9358e52ff x-ms-traffictypediagnostic: DM5PR1101MB2233: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 1UW+F7zOykHc5iyf2ACegZNk4NcTjoXsEDTS9ZRvIbaAf9nqnQZHOk+97ghXDDdX8jCbb0iSp5plzH/5qhjPEy+sDcNCRf4YxXUDZeyhtWLwy7IyoGMGAgymCF8mBm+7I80055TZNgbELtn5wSzRTJb8KFWT87KcludYewOuqwGUF79eL7+wYqNIVpCue6CbDmrkXoONjweVAByk1+hBBgfJyyaGYneINxFSBThZybiUZ3stcoFEp9gm2Etm/aefiDMBuAK9OYrkvpi5yEGMg1TUs/XfiMGhooUmBeVEeS2kzoj0W/yOsYl9PJ5rASSveBTCMW2yNmrA7e4ok9cvZWIFwX2366/n5FVE6Bot8+QUuO4v1MGaKQO5NQbVKud+GoqE6mEoRRhlEidfGZK4qOzy16ufPZyr0DcR1AXlri881EFXnG0DaVWpYgBtqtXFHEjyaTecK0YMls6nDmNB0vy5THYSuHMtVDkMg7lxLv74CP+bJjnYNBOvNkvJKnM0swRWU0M1yW4iZaULYpsyxvX6va2XcaFHqrOkIKI4R8Ic2Rpa92KgI8Nvvz6gmUNBbu8H5iyAMhP5+o22hN/OSjk+8DG1otCo9J6oSHZCMX4= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM5PR11MB1690.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(39860400002)(366004)(376002)(396003)(346002)(136003)(33656002)(6506007)(7696005)(4326008)(38100700002)(66476007)(26005)(478600001)(66556008)(76116006)(71200400001)(66946007)(64756008)(66446008)(86362001)(8676002)(6916009)(9686003)(186003)(52536014)(316002)(5660300002)(83380400001)(55016002)(30864003)(2906002)(122000001)(8936002); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?bWriNGdXTDlZoTNG38yhPfSJcYqNeCy27kEgQEB6AR0Juz70hlOUGCW4yeUm?= =?us-ascii?Q?2RwDj2zROb3qno40l+sZbn4r4tHv1RcbhVo7qy1Hg0oKxOOpZJfmGk9vennv?= =?us-ascii?Q?dDGZHfu+TLfwKkNctbp8IgPStBQXqVH4uNiwU091huHJkKpKtZKwrMY8cH26?= =?us-ascii?Q?ds8MiAwremN81zkain2Sn5DlTcqn3FhyhWcfl0SUR8QpwABe4qureDqKOcpy?= =?us-ascii?Q?qDJJ75vTNIguCp8dTcQcbo59ziQgFZ0ATt7QUKm4eiPfOHwOvqYc08pHSbgm?= =?us-ascii?Q?S8xinfAqrqmZwwa4tdJ4kYA4n85YhtuWSmurVLmXAed/u01D+nOrlAmvohH4?= =?us-ascii?Q?1fEZLQF9ln4Fe3RmwaxH0/xE/I0N1ZPwuMtNBlehfeuX5x/GvEMQyIun++t4?= =?us-ascii?Q?lnUrSvm2fqXy94R6Er+x5ElfSl4zEGTJ4jb4o9/2E3j0Ql38ZvDQifQLyYTG?= =?us-ascii?Q?YnvVCOPgPsoxM2C2YTqrnPGkVahNRrQDiavuPy1GFpG3Xw3ak4rlpl4kTKlE?= =?us-ascii?Q?MXM3HpdU08KOhCnQAXZiceue6Ohvy/yu/xhyY8ejGfRq9fHtBmEKMqgMrFdr?= =?us-ascii?Q?jecXNdHoZjcfE4LURsMyxxWfKhlHlsbUXum9Em9aevj7NycoPNMwvGxToQqo?= =?us-ascii?Q?9s9rZTk49YvPekoHP07njh0WgkT9ybiiUAkafdeCixpzcpwU5lDui5Tt/k08?= =?us-ascii?Q?Yl2xkRKp+7Av6q6lcY/b7/3NlYejx5D0G564xUgUH+gaRyjrQ7S1LFDA2wvb?= =?us-ascii?Q?3GifdcsQjJUtkmFkk2+0CSo0OSGog83IwOq7KZtVW5ak22kS64ofmblxEJN7?= =?us-ascii?Q?aLSLYvd60Wb+VKEvTWOlXTefFTsvlO39mpMuMPCk9I+mTWiutz4YLLoPlsCY?= =?us-ascii?Q?Yqs0QBOCM2StONb3mVzqrzrwoNJ0GVQHPKfsQIB8yUpSNYNnqo/JdOXOMsGO?= =?us-ascii?Q?u2bZmDx7Nb7QdbCdd9dd5Hgj1QKiSkjfulxXuxfbcGNpH/1kPHEzwb8sgoXl?= =?us-ascii?Q?UQydbZQdl0preN3gyYQQkRIySAqTMxN00kwsk8Cm2z2wICiCUcWhRAOA9csd?= =?us-ascii?Q?ci/08P7poOzrtlx1UAMdK8Yo37811IW9/6FsfoZlowgqHi/3zUDtGifJiGhD?= =?us-ascii?Q?2Vzk+7TTySwyJNG2sZs1r5VYRjJW98ksemQf8TTF64Mejbh4s7CdfnPoVEAH?= =?us-ascii?Q?oGYCszNPeSOvBuxbP9mv60B0fA95lHdo2MhxRsMRvw7NlKTSTXtKKxLiftgJ?= =?us-ascii?Q?nc1ViFAKclsUtAHJ1YbyuryAKDh/5hgW67+zgyp7IahcOYTo4fp4BXdw37g0?= =?us-ascii?Q?kKe+Aq59e6WYmMNKWBBOyw76?= Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1690.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 12d94750-a451-43e7-970a-08d9358e52ff X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Jun 2021 14:59:24.2876 (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: E5UBgpCFsYRAFFgIQvFqz02V/h5FYjh0tSoWeB1w0PbeLsvXXLqDVTeigExqQTdkiyL3/u5BM4uo8Rtk5Xp8SleSmkoofrkHMw0aVi+/HKU= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR1101MB2233 X-OriginatorOrg: intel.com Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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, 22 Jun 2021 14:59:33 -0000 Hello Zied, >This patch extend branch tracing by adding the functions needed >to collect parameters for decoding ETM traces and decoding them. Looks good, overall. Comments are mostly about simplifications. Shall we discuss them in this email thread before going to v7? >diff --git a/gdb/btrace.c b/gdb/btrace.c >index 5e689c11d4b..2676389b63e 100644 >--- a/gdb/btrace.c >+++ b/gdb/btrace.c >@@ -671,6 +672,38 @@ ftrace_update_insns (struct btrace_function *bfun, >const btrace_insn &insn) > ftrace_debug (bfun, "update insn"); > } > >+#if defined (HAVE_LIBOPENCSD_C_API) >+/* Remove last instruction from BFUN's list. >+ This function is not generic and does not undo functions chaining. >+ When adding an instruction after using it, the caller must ensure >+ that the instruction produces the same chaining. >+ An example of good case, is when the same removed instruction >+ is added later. */ This function is called in one place and the way I understood it, we remove an undefined instruction that was used as a breakpoint. I assume the behav= ior is that this undefined instruction is counted as executed in the trace and = we want to fix that up. We don't add that instruction back, though, do we? And if we wanted to support that remove-then-re-insert use-case, wouldn't we want to return the removed instruction? I'd rather we document exactly that one use-case in which we need this functionality. This allows documenting exactly what we're doing and why this is necessary. >+ >+static void >+ftrace_remove_last_insn (struct btrace_thread_info *btinfo) >+{ >+ /* If we didn't have a function, we return. */ >+ if (btinfo->functions.empty ()) >+ return; Should this be an error? >+ >+ struct btrace_function *bfun =3D &btinfo->functions.back (); >+ /* If we had a gap before, we return. */ >+ if (bfun->errcode !=3D 0) >+ return; In which case can we have a gap? >+ >+ if (!bfun->insn.empty ()) >+ bfun->insn.pop_back (); >+ else >+ { >+ /* A valid function must have at least one instruction. */ >+ internal_error (__FILE__, __LINE__, >+ _("Attempt to remove last instruction" >+ "from an empty function")); We just removed an instruction in the then statement, which could well result in BFUN->INSN to become empty(). The statement about valid functions above may be a bit too generic. Also, ftrace_new_* () may create empty function segments. Isn't it rather that in our use-case, the caller knows that the function segment cannot be empty? >+ case ocsd_isa_custom: >+ return BTRACE_INSN_FLAG_ISA_CUSTOM; >+ >+ case ocsd_isa_unknown: >+ return BTRACE_INSN_FLAG_ISA_UNKNOWN; >+ >+ default: >+ internal_error (__FILE__, __LINE__, >+ _("Undefined elem->isa value returned by OpenCsd.")); This internal error kills GDB. Should this be a normal error that just stops trace processing for this unknown ISA? >+ insn.iclass =3D BTRACE_INSN_OTHER; >+ insn.pc =3D elem->st_addr; >+ for (int i =3D 0; i< elem->num_instr_range; i++) >+ { >+ try >+ { >+ insn.size =3D gdb_insn_length (gdbarch, insn.pc); >+ } >+ catch (const gdb_exception_error &err) >+ { >+ error (_("Failed to get the size of the instruction.")); Isn't the original exception good enough? >+ } >+ >+ struct btrace_function *bfun =3D ftrace_update_function (btinfo, in= sn.pc); >+ if (etm_decoder->arch_version =3D=3D ARCH_V7) >+ insn.flags =3D cs_etm_get_isa_flag (elem); ELEM isn't changing so we could set this once outside of the loop. >+ >+ if (i =3D=3D elem->num_instr_range -1) >+ insn.iclass =3D cs_etm_get_instruction_class (elem); >+ >+ ftrace_update_insns (bfun, insn); >+ insn.pc =3D insn.pc + insn.size; >+ } >+} >+ >+/* Update btrace in the case of an exception. */ >+ >+static void >+cs_etm_update_btrace_with_exception (const struct cs_etm_decoder >*etm_decoder, >+ const ocsd_generic_trace_elem *elem) >+{ >+ gdb_assert (elem->elem_type =3D=3D OCSD_GEN_TRC_ELEM_EXCEPTION); >+ >+ struct thread_info *tp =3D etm_decoder->t_info; >+ struct btrace_thread_info *btinfo =3D &tp->btrace; >+ >+ /* Handle the implementation of breakpoints in gdb for arm (v7) archite= cture >+ using undefined instructions. */ >+ if (etm_decoder->arch_version =3D=3D ARCH_V7) >+ { >+ if (elem->exception_number >+ =3D=3D CS_ETMV3_4_CORTEX_A_R_EXCEPTION_UNDEFINED_INSTRUCTION) >+ { >+ DEBUG ("handle breakpoints implementation in gdb for ARMv7"); >+ ftrace_remove_last_insn (btinfo); Here we just discard the 'breakpoint' instruction, correct? We don't reall= y plan to re-insert it again. Can we ensure that we actually inserted that instruction into the current B= FUN? We wouldn't need the helper function and all those checks would become asse= rts. I like little helper functions but if we just removed the insn here it woul= d be more obvious why we're doing it. >+ } >+ } >+} >+ >+/* Update btrace in the case of a trace on. */ >+ >+static void >+cs_etm_update_btrace_with_trace_on (const struct cs_etm_decoder >*etm_decoder, >+ const ocsd_generic_trace_elem *elem) >+{ >+ gdb_assert (elem->elem_type =3D=3D OCSD_GEN_TRC_ELEM_TRACE_ON); >+ >+ if (elem->trace_on_reason !=3D TRACE_ON_NORMAL) >+ { >+ struct thread_info *tp =3D etm_decoder->t_info; >+ struct btrace_thread_info *btinfo =3D &tp->btrace; >+ ftrace_new_gap (btinfo, elem->trace_on_reason, etm_decoder->gaps); >+ } Even for normal trace off/on pairs, we'd want a gap if PC is moving. >+} >+ >+/* Callback function when a ocsd_generic_trace_elem is emitted. */ >+ >+static ocsd_datapath_resp_t >+cs_etm_trace_element_callback (const void *context, >+ const ocsd_trc_index_t index, >+ const uint8_t trace_chan_id, >+ const ocsd_generic_trace_elem *elem) >+{ >+ if (record_debug !=3D 0) >+ { >+ char str_buffer[128]; >+ if (ocsd_gen_elem_str (elem, str_buffer, 128) =3D=3D OCSD_OK) sizeof (str_buffer) >+/* Callback to print error log. */ >+ >+static void cs_etm_print_error_log (const void *p_context, const char >*psz_msg_str, const int str_len) >+{ >+ char string_buffer[128]; >+ memcpy (string_buffer, psz_msg_str, std::min(str_len, 127)); >+ if (str_len >127) >+ string_buffer[127] =3D 0; >+ else >+ string_buffer[str_len] =3D 0; We can simply adjust str_len before the memcpy. There's no reason to decla= re it const; it is passed by-value. >+ uint8_t csid; >+ errcode =3D ocsd_dt_create_decoder (decoder->dcd_tree, decoder_name, >+ OCSD_CREATE_FLG_FULL_DECODER, >+ trace_config, &csid); >+ if (errcode !=3D OCSD_OK) >+ { >+ warning (_("ocsd_dt_create_decoder failed with error: %d"), errcode= ); >+ return errcode; >+ } >+ >+ errcode =3D ocsd_dt_set_gen_elem_outfn (decoder->dcd_tree, >+ cs_etm_trace_element_callback, >+ decoder); >+ if (errcode !=3D OCSD_OK) >+ { >+ warning (_("ocsd_dt_set_gen_elem_outfn failed failed with error: %d= "), >+ errcode); >+ return errcode; >+ } >+ >+ errcode =3D ocsd_def_errlog_init (OCSD_ERR_SEV_ERROR, 1); >+ if (errcode !=3D OCSD_OK) >+ { >+ warning (_("ocsd_def_errlog_init failed failed with error: %d"), >+ errcode); >+ return errcode; >+ } >+ >+ /* Initialize error printer. */ >+ errcode =3D ocsd_def_errlog_config_output (C_API_MSGLOGOUT_FLG_NONE, >nullptr); >+ if (errcode !=3D OCSD_OK) >+ { >+ warning (_("ocsd_def_errlog_init failed failed with error: %d"), >+ errcode); >+ return errcode; >+ } >+ >+ errcode =3D ocsd_def_errlog_set_strprint_cb (decoder->dcd_tree, nullptr, >+ cs_etm_print_error_log); >+ if (errcode !=3D OCSD_OK) >+ { >+ warning (_("ocsd_def_errlog_set_strprint_cb failed failed with erro= r: %d"), >+ errcode); >+ return errcode; >+ } >+ >+ decoder->prev_return =3D OCSD_RESP_CONT; There are several error returns that leave this field uninitialized. I ass= ume that the decoder is not working in those cases and will not be used. Don't we n= eed to undo anything before returning with an error? I see that we call cs_etm_free_decoder () below on errors, which probably t= akes care of cleaning up partially complete decoders. It still feels odd to ret= urn an error and leave a decoder partially initialized. Let's at least note that in the comment that on error, the caller is expect= ed to call cs_etm_free_decoder (). >+ struct cs_etm_decoder *decoder; >+ >+ decoder =3D (struct cs_etm_decoder*) xmalloc (sizeof (struct cs_etm_dec= oder)); >+ decoder->dcd_tree =3D dcdtree_handle; >+ >+ for (int i =3D 0; i < cpu_count; i++) >+ { >+ ocsd_err_t errcode =3D cs_etm_create_decoder (&(t_params->at (i)), = decoder); >+ if (errcode !=3D OCSD_OK) >+ { >+ cs_etm_free_decoder (decoder); >+ return nullptr; >+ } >+ } >+/* Process an etm traces data block. >+ In case of an error it resets the decoder and tries to process further= . */ >+ >+static void >+cs_etm_process_data_block (struct btrace_thread_info *btinfo, >+ struct cs_etm_decoder *decoder, >+ uint64_t index, const uint8_t *buf, >+ size_t len, size_t *consumed) >+{ >+ ocsd_datapath_resp_t data_path_return =3D OCSD_RESP_CONT; >+ size_t processed =3D 0; >+ uint32_t count; >+ >+ while (processed < len) >+ { >+ if (OCSD_DATA_RESP_IS_CONT (data_path_return)) >+ { >+ data_path_return =3D ocsd_dt_process_data (decoder->dcd_tree, >+ OCSD_OP_DATA, >+ index + processed, len - processed, >+ &buf[processed], &count); >+ processed +=3D count; >+ >+ } There seems to be an extra empty line. Should we assert COUNT > 0 || ! OCSD_DATA_RESP_IS_CONT (data_path_return) to ensure forward progress? >+ else if (OCSD_DATA_RESP_IS_WAIT (data_path_return)) >+ { >+ data_path_return =3D ocsd_dt_process_data (decoder->dcd_tree, >+ OCSD_OP_FLUSH, >+ 0, 0, nullptr, nullptr); Same here for ! OCSD_DATA_RESP_IS_WAIT (data_path_return), although we still wouldn't be able to detect switching back and forth between those two. >+ } >+ else >+ { >+ warning (_("error %d in ocsd_dt_process_data after processing %zu"), >+ data_path_return, processed); >+ ftrace_new_gap (btinfo, data_path_return, decoder->gaps); >+ data_path_return =3D ocsd_dt_process_data (decoder->dcd_tree, >+ OCSD_OP_RESET, >+ 0, 0, nullptr, nullptr); >+ //todo: shall we increase processed? by 1, or 4 do we need to manually >align to 16 bytes? Please use /* */ for comments. Most people use FIXME instead of todo. We may end up in an infinite loop if trying to reset the decoder triggers a= nother error indefinitely. We should still be able to interrupt GDB itself using ^C on the CLI if we e= ver actually run into an infinite decode loop. So maybe this is all overkill. >+ ocsd_err_t ocsd_error >+ =3D cs_etm_add_mem_access_callback (decoder, >+ (CORE_ADDR) 0x0L, (CORE_ADDR) -1L, >+ btrace_etm_readmem_callback); >+ if (ocsd_error !=3D OCSD_OK) >+ error (_("Failed to add CoreSight Trace decoder memory access callbac= k.")); Could we print ICSD_ERROR in the error message? Even printing it as an int= would help. >diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c >index 16ffb76272b..57b9c8ec487 100644 >--- a/gdb/record-btrace.c >+++ b/gdb/record-btrace.c >@@ -1557,6 +1558,38 @@ record_btrace_target::remove_breakpoint (struct >gdbarch *gdbarch, > return ret; > } > >+/* Reconstruct the instruction set state bits of CPSR register >+ according to instruction flags. >+ See Table A2-1 in DDI0406B_arm_architecture_reference_manual >+ for more details. */ So we do not need to store CPSR after all? We can infer the relevant bits from ISA information we get from the trace? Nice. >+ >+static unsigned int >+cs_etm_reconstruct_cpsr_iset_state (const struct btrace_insn *insn) >+{ >+ switch (insn->flags & BTRACE_INSN_FLAG_ISA_MASK) >+ { >+ case BTRACE_INSN_FLAG_ISA_ARM: >+ /* ARM state: J and T bits are not set. */ >+ return 0; >+ >+ case BTRACE_INSN_FLAG_ISA_THUMB2: >+ /* THUMB state: J bit is not set, T bit is set. */ >+ return 0x20; >+ >+ case BTRACE_INSN_FLAG_ISA_TEE: >+ /* THUMB EE state: J and T bits are set. */ >+ return 0x1000020; >+ >+ case BTRACE_INSN_FLAG_ISA_JAZELLE: >+ /* JAZELLE state: J bit is set, T bit is not set. */ >+ return 0x1000000; >+ >+ default: >+ /* Default is ARM mode. */ >+ return 0; How can we run into this default case? Should this be an error in case we add new ISA modes in the future? >+ } >+} >+ > /* The fetch_registers method of target record-btrace. */ > > void >@@ -1581,16 +1614,32 @@ record_btrace_target::fetch_registers (struct >regcache *regcache, int regno) > pcreg =3D gdbarch_pc_regnum (gdbarch); > if (pcreg < 0) > return; >- >- /* We can only provide the PC register. */ >- if (regno >=3D 0 && regno !=3D pcreg) >+ /* We can only provide the PC or CPSR registers here. */ Let's extend the comment to say that CPSR is ARM and that we're checking the architecture below to avoid redundant checks when we want to fetch other registers. >+ if (regno >=3D 0 && !(regno =3D=3D pcreg || regno =3D=3D ARM_PS_REG= NUM)) > return; > > insn =3D btrace_insn_get (replay); >- gdb_assert (insn !=3D NULL); >+ gdb_assert (insn !=3D nullptr); > >+ if ((regno < 0) || (regno =3D=3D pcreg)) >+ { > regcache->raw_supply (regno, &insn->pc); > } >+ if ((regno < 0) || (regno =3D=3D ARM_PS_REGNUM)) >+ { >+ /* Provide CPSR register in the case of an armv7 target. */ >+ const struct target_desc *tdesc =3D gdbarch_target_desc (gdbarch); >+ >+ const char *tdesc_name =3D tdesc_architecture_name (tdesc); >+ if (strcmp (tdesc_name, "arm") =3D=3D 0) >+ { >+ int cpsr; >+ cpsr =3D cs_etm_reconstruct_cpsr_iset_state (insn); Please initialize in the declaration. >+ regcache->raw_supply (regno, &cpsr); >+ } >+ } >+ return; >+ } > else > this->beneath ()->fetch_registers (regcache, regno); > } >diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h >index 153b977723a..ee05ecb8b10 100644 >--- a/gdbsupport/btrace-common.h >+++ b/gdbsupport/btrace-common.h >+/* Parameters of trace source. */ >+struct cs_etm_trace_params >+{ >+ /* Architecture version of trace source. */ >+ int arch_ver; >+ /* Core profile of the trace source. */ >+ int core_profile; >+ /* Traces protocol. */ >+ int protocol; >+ union { >+ struct cs_etmv3_trace_params etmv3; >+ struct cs_etmv4_trace_params etmv4; >+ }; >+}; Please add empty lines between members. >+ >+/* Configuration information to go with the etm trace data. */ >+struct btrace_data_etm_config >+{ >+ /* Count of the CPUs (trace sources). */ >+ int cpu_count; >+ /* List of traces sources parameters. */ >+ std::vector *etm_trace_params; >+ /* Trace sink parameters. */ >+ struct cs_etm_decoder_params etm_decoder_params; >+}; Also here. >+ >+/* Branch trace in ARM Processor Trace format. */ >+struct btrace_data_etm Is that the correct term? 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