From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by sourceware.org (Postfix) with ESMTPS id 179E93833012 for ; Tue, 27 Apr 2021 17:31:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 179E93833012 IronPort-SDR: fZkImZFxGKjuLRgNa6vd3b4tjsJwoHddVph/AfCBy/pmVxrip+IHqUfqxy16+3RUgcg3ATazQX wPUO91MDbvdg== X-IronPort-AV: E=McAfee;i="6200,9189,9967"; a="257860604" X-IronPort-AV: E=Sophos;i="5.82,254,1613462400"; d="scan'208";a="257860604" Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 27 Apr 2021 10:31:41 -0700 IronPort-SDR: ZPu2/hoOLYiNE6g0ihEVGifYedKlj+4qT50F7RLahYlg+VE+WDXzDFKRbG3WGF3y/wYCK1z312 s4ZLXRLvZDFA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.82,255,1613462400"; d="scan'208";a="403341181" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by orsmga002.jf.intel.com with ESMTP; 27 Apr 2021 10:31:39 -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.2106.2; Tue, 27 Apr 2021 10:31:38 -0700 Received: from fmsmsx606.amr.corp.intel.com (10.18.126.86) 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.2106.2; Tue, 27 Apr 2021 10:31:38 -0700 Received: from fmsedg601.ED.cps.intel.com (10.1.192.135) 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.2106.2 via Frontend Transport; Tue, 27 Apr 2021 10:31:38 -0700 Received: from NAM02-BL2-obe.outbound.protection.outlook.com (104.47.38.52) by edgegateway.intel.com (192.55.55.70) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2106.2; Tue, 27 Apr 2021 10:31:37 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=FmekFgka5TBkJ+wmWoszAL4j6fayG0S6qFYVSr1XITER8Jq4dY+lvXzqewsG3MwOVQmkR+QfzlrnWuAblFGQWekpjEz1oP3tsMJyZJGzR2QveAZ+fofO5HSb2AYXm/8ATJ0HsjNv17b5eJL+zgsGbEdKxmrkki/jyJeBIMMhD2wmKuNlxz6m2Ajl9841IrQDV4drqtD9t6iES0LVu0VYrsgmFWlzyb3s2Eh1/c7x6kR3RgLxqzLaAkux/xG1lUiYXHN+v3EKbQ65ixu/H8DRC05PS/8kVoxBLV0bn+Pkmq0KQIZK7BgKdqQmSviD1PSx+VGZmfPnsy2HVYPqoWMn6w== 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=4Go4HYtmYjgzwUehmNmK3WhnEfpGwcYd+wEns2DsMt0=; b=mQrwUl4R5fmzb0nqNHlTyKZFc4Z3qCNJB3cl4y10mDvvY2bJHgcN9/81HGmrSneupJFX2rU0/6azZlrDcK8I/P58B0Zgzpx/nIQeFrsf0x1fXfGeMToU8pvd/5E9QKpCoBZnptSLJfVXID/Kz1o5yQJ4F28fcp83RisPm9GLTqvU+iSpOEX4HLC16JcgiXC3a62/dGfdt3kbO0BYWb7rbFT36QaopEABCW6Qasb7jSjOEyl+7x7L52MJTIsZmVnD/6yHY7viaDC+7qsauhseyJhC9aFfPVrElgBH6Jx3WUyyBCFNNVvS+pJDQUKbJ0RySKgfgxkQrdg7S4fxQw4ewA== 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 DM5PR11MB1612.namprd11.prod.outlook.com (2603:10b6:4:b::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4087.25; Tue, 27 Apr 2021 17:31:35 +0000 Received: from DM5PR11MB1690.namprd11.prod.outlook.com ([fe80::b528:e890:900e:1b5c]) by DM5PR11MB1690.namprd11.prod.outlook.com ([fe80::b528:e890:900e:1b5c%12]) with mapi id 15.20.4065.027; Tue, 27 Apr 2021 17:31:35 +0000 From: "Metzger, Markus T" To: Zied Guermazi , "gdb-patches@sourceware.org" Subject: RE: [PATCH v5 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant Thread-Topic: [PATCH v5 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant Thread-Index: AQHXN4Et92XHHF79vE6wvZoJS7EF46rGzm4g Date: Tue, 27 Apr 2021 17:31:35 +0000 Message-ID: References: <20210422140921.175221-1-zied.guermazi@trande.de> <20210422140921.175221-4-zied.guermazi@trande.de> In-Reply-To: <20210422140921.175221-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: [91.89.55.195] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: e4213d6a-7dd1-4a0e-e526-08d909a24e92 x-ms-traffictypediagnostic: DM5PR11MB1612: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8273; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: RfFCQA444tZSy4Vo+ljNqGKD1Tw5UMPHVky4ldIUB9mpfeZx8F+/69JijuZTUAWaPz+dNniBk8VuBze41eGxgFSrcCz44VMQK+y7/T/I7IXBiJs7PIxf2fxU3L8OQanLjLt2EBKCBqiwR31117nwbl6TIReaH5nLSzdSFLMk5FUkqh18k8/1GiStGpGkOAG7j5JHbv4bIEc6+aF7pW6OUe/NbFsyyxFyyEMVhVg67hme2YN0UUDTS05f73JWjpD6kAY83kES14siOKu+G/ezbhjcFExiKU3WdIGoiGIZ4PKpJNJAWH4DvLGPLMAo26Qnk4trAIMv0LmQX+2Th8OjrJwPw4hESYhIn+qlO2NOhP3BYsYljtURS7okfDJvnTK0ZfoGDVedkgdzAKsIyxteorR+hjDF///BICPcG2UqD6NBfzNIz/xu3rrhR2pxlspmlKtl47u7wGugKEB9ctoHEkui3JyrTuKZxiYiWQhd6E0/bu8Pum1D39knjPDfGazV0g1/hnZvcge4iU8qs0ZDILixRLLZF5qcjVkC5jGuklrfiKKoa9b4VO5fmMHn7v0j6qxfy5tVibV4C48Yb4+aZJiPplrgm8rJ7p1BrPOJDiA= 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:(366004)(346002)(396003)(376002)(39860400002)(136003)(9686003)(5660300002)(86362001)(55016002)(6506007)(7696005)(122000001)(26005)(38100700002)(33656002)(186003)(64756008)(66446008)(316002)(76116006)(8676002)(66946007)(66476007)(110136005)(66556008)(83380400001)(8936002)(71200400001)(30864003)(52536014)(2906002)(478600001)(579004); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata: =?us-ascii?Q?grzvs9C4e7mKfop0CGXmL89CZgjK9CoJRH4Su3l3BAQtl9TF8aTe1zd+qgeV?= =?us-ascii?Q?BZrL/+ApFU0xalwR4HTnB4KR07jiAC3ERyFzclrvHNAHeQi3OrxS0oXM+rAs?= =?us-ascii?Q?fsJ4weFgUhtz2hFSlPIh7/WtO5lP6JwX9HOV1nJ2nZG0zT0OQx16RvYfAY4W?= =?us-ascii?Q?/INQn02BNxslhnyQVZWZyYb/fM8+eHm9e8BjXNu41HEnt7iZOJm2eEd8T8E7?= =?us-ascii?Q?y9bN1ed2D8UwAUTCzFobAk0IPYl4Ltl5UfJxkttqjSaTjH5pSw62YhOloATw?= =?us-ascii?Q?9soXhbgWNttzdANJT9/ACw2uEjsBLXtMQ7ryvA69rx87WtoO92TL/0Kw/BZ5?= =?us-ascii?Q?i45n+riUwaRBlvz7bqPbAQTQOnpzGXsa72Bv8H+hoKQSxIbdG5uGvl3wBLQ3?= =?us-ascii?Q?BP4VkHqBVk7uksc3q+KSjaB/HyKonTdbOsDmsR31upFhoLXxl9I3A1fx++OD?= =?us-ascii?Q?0X895qiEYtdL9u9k7AjF7zndp8WEvaAtoPkNQE3GspGsZLdbbHkQ5Fgc0PtB?= =?us-ascii?Q?7X2CgjjVpwMhoY6c5eopUZOtGZhUvjSiiS3gJy8c/jiWhM0c4O/necP+LX57?= =?us-ascii?Q?kRpn5Us0yRp0FCq0izxmL9ypSSQjM3LH1Tw7LnKleFNwk5tpYZMwBiWEKKZ9?= =?us-ascii?Q?6VfmYXzBrJX7GkNoHR65ozdm0/rD6Wt82ptwoJ8J0T9G5fB+WB4iY6F23fjJ?= =?us-ascii?Q?6qHEpOoVvUJg7nyhHGNPITssYPf5Bu+RbaakUdtZ7LrBSLSOftDSMXYA7/Sd?= =?us-ascii?Q?ZyczCAyD1A0eNyPiFH6LKt7CarcnFF0J9SV+OW/griONnHCZfdOmZOn6orBu?= =?us-ascii?Q?lbMGNHWHc6/dWmsFgofxOYjHPXnpzTdNvP1Cppr1E9zVJ9PPWJl2kdzLdt/+?= =?us-ascii?Q?R/dMg1WuDH//agHXigngYquZrtWxfS6hmyDq1qNDGTpjKDWTpMVW29ucBPwU?= =?us-ascii?Q?F1eNIlA/F14B2woRrvTj8k+K5WBzsLihRe+U8AminSg9tTTA15fLYbJTR8Ch?= =?us-ascii?Q?xB3LQD5N5wWyh/WzLJ6ltfpq9bMKMgmeinQNxVwLUfk6dNP3OAd96NCR+jfM?= =?us-ascii?Q?8yaBD0t+XcKLW8za+5IZDB8Fi4STBavgFK0BCMRP+UUk0QAsGj+7ipBignzl?= =?us-ascii?Q?8U8SaXc1kDDaU5QCl+CsYTxIJuyghnWu5rMWB7cYlC0SQC1VaM22t7fJZpL8?= =?us-ascii?Q?jVPVsSRB94WOWUhptASywCJvixr5POr69ZV+CbjhUTqbL8tGUyCamqgOmpJd?= =?us-ascii?Q?JurSKdVZ6Nh7CTAPSj5E9yf/zfX2HOlW0QSOYLMutVh9URQZY3p3QYswQoMm?= =?us-ascii?Q?zBF1igEFQLK5K85s6rq7bdTK?= 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: e4213d6a-7dd1-4a0e-e526-08d909a24e92 X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Apr 2021 17:31:35.6198 (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: mM8D8qPoYSj3PXaeegxXHt8+T4uNhIgPgQEa1wN86eDrDripj4PA5zCoHimqmVEm0LXpR6NQ0Iq0UsdfRQhJ2EerR0jATwSJFmOhzw4BHAI= X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1612 X-OriginatorOrg: intel.com Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, DKIM_INVALID, DKIM_SIGNED, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_BARRACUDACENTRAL, 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, 27 Apr 2021 17:31:48 -0000 Hello Zied, >This patch extend branch tracing by adding the functions needed >to collect parameters for decoding ETM traces and decoding them. >diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h >index fa589fd0582..2b5d9f2c9dc 100644 >--- a/gdb/arch/arm.h >+++ b/gdb/arch/arm.h >@@ -150,6 +150,39 @@ enum arm_m_profile_type { > #define BranchDest(addr,instr) \ > ((CORE_ADDR) (((unsigned long) (addr)) + 8 + (sbits (instr, 0, 23) << 2= ))) > >+/* Exception numbers as encoded in ETMv3.4 for ARMv7-M processors. >+ See IHI0014Q_etm_architecture_spec table 7.11. */ >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_USAGE_FAULT > 0x009 >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_NMI 0x00a >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_SVC 0x00b >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_DEBUG_MONITOR > 0x00c >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_MEM_USAGE > 0x00d >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_PEND_SV > 0x00e >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_SYS_TICK 0x00f >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_PROCESSOR_RESET > 0x011 >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_HARD_FAULT 0x013 >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_BUS_FAULT 0x015 >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_IRQ(n) \ >+ (n =3D=3D 0 ? 0x008 : n < 8 ? n : n + 0x010) >+#define CS_ETMV3_4_CORTEX_M_EXCEPTION_IS_IRQ(n) \ >+ ((n > 0x000 && n < 0x009)||(n > 0x017 && n < 0x200)) Space between macro name and (. Also spaces around ||. >diff --git a/gdb/btrace.c b/gdb/btrace.c >index c697f37f46c..d14ccd765b2 100644 >--- a/gdb/btrace.c >+++ b/gdb/btrace.c >@@ -42,6 +42,7 @@ > #include > #include > #include >+#include "arch/arm.h" GDB includes are in the first block. >@@ -257,6 +258,8 @@ ftrace_new_function (struct btrace_thread_info *btinfo, > } > > btinfo->functions.emplace_back (mfun, fun, number, insn_offset, level); >+ ftrace_debug (&btinfo->functions.back (), "new function"); There was no debug log on purpose as this function is typically called from other ftrace_new_* functions. It is only called directly at the beginning = and after a gap. >@@ -671,6 +674,39 @@ 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 user must ensure Nit: should this be 'caller' instead of 'user'? >+ that the instruction produces the same chaining. >+ An example of good case, is when the same removed instruction >+ is added later. */ >+ >+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; >+ >+ struct btrace_function *bfun; >+ bfun =3D &btinfo->functions.back (); Couldn't we simply declare BFUN as reference? >+ /* If we had a gap before, we return. */ >+ if (bfun->errcode !=3D 0) >+ return; >+ >+ 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")); >+ } >+} >+#endif /* defined (HAVE_LIBOPENCSD_C_API) */ >@@ -1502,6 +1538,560 @@ btrace_compute_ftrace_pt (struct thread_info *tp, >+/* Set the instruction flag to track ISA mode of ARM processor. */ >+ >+static btrace_insn_flags >+cs_etm_get_isa_flag (const ocsd_generic_trace_elem *elem) >+{ >+ switch (elem->isa) >+ { >+ case ocsd_isa_arm: >+ return BTRACE_INSN_FLAG_ISA_ARM; >+ >+ case ocsd_isa_thumb2: >+ return BTRACE_INSN_FLAG_ISA_THUMB2; >+ >+ case ocsd_isa_aarch64: >+ return BTRACE_INSN_FLAG_ISA_AARCH64; >+ >+ case ocsd_isa_tee: >+ return BTRACE_INSN_FLAG_ISA_TEE; >+ >+ case ocsd_isa_jazelle: >+ return BTRACE_INSN_FLAG_ISA_JAZELLE; >+ >+ 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.")); >+ } Nit: if you put the internal_error outside of the switch, the compiler will complain if the enum gets extended. >+/* Returns the instruction class. */ >+ >+static enum btrace_insn_class >+cs_etm_get_instruction_class (const ocsd_generic_trace_elem *elem) >+{ >+ switch (elem->last_i_type) >+ { >+ case OCSD_INSTR_BR: >+ case OCSD_INSTR_BR_INDIRECT: >+ switch (elem->last_i_subtype) >+ { >+ case OCSD_S_INSTR_V8_RET: >+ case OCSD_S_INSTR_V8_ERET: >+ case OCSD_S_INSTR_V7_IMPLIED_RET: >+ return BTRACE_INSN_RETURN; >+ >+ case OCSD_S_INSTR_BR_LINK: >+ return BTRACE_INSN_CALL; >+ >+ case OCSD_S_INSTR_NONE: >+ return BTRACE_INSN_JUMP; >+ } >+ return BTRACE_INSN_OTHER; >+ >+ case OCSD_INSTR_ISB: >+ case OCSD_INSTR_DSB_DMB: >+ case OCSD_INSTR_WFI_WFE: >+ case OCSD_INSTR_OTHER: >+ return BTRACE_INSN_OTHER; Why not merge this with the default below? >+ >+ default: >+ return BTRACE_INSN_OTHER; >+ We don't need an empty line, here. >+ } >+} >+ >+/* Update btrace in the case of an instruction range. */ >+ >+static void >+cs_etm_update_btrace_with_inst_range (const void *context, >+ const ocsd_generic_trace_elem *elem) >+{ >+ gdb_assert (elem->elem_type =3D=3D OCSD_GEN_TRC_ELEM_INSTR_RANGE); >+ >+ struct cs_etm_decoder *etm_decoder =3D (struct cs_etm_decoder *) contex= t; >+ if (etm_decoder->t_info =3D=3D nullptr) >+ return; >+ >+ struct thread_info *tp =3D etm_decoder->t_info; >+ >+ struct btrace_thread_info *btinfo =3D &tp->btrace; >+ >+ struct gdbarch *gdbarch =3D target_gdbarch (); >+ >+ struct btrace_insn insn; >+ CORE_ADDR pc =3D elem->st_addr; >+ int size; The distribution of empty lines between declarations is a bit unclear. >+ for (int i =3D 0; i< elem->num_instr_range; i++) >+ { >+ insn.pc =3D pc; >+ try >+ { >+ size =3D gdb_insn_length (gdbarch, pc); >+ } >+ catch (const gdb_exception_error &err) >+ { >+ error (_("Failed to get the size of the instruction.")); >+ } >+ >+ struct btrace_function *bfun; >+ bfun =3D ftrace_update_function (btinfo, pc); You may initialize BFUN as part of the declaration. >+ insn.iclass =3D BTRACE_INSN_OTHER; >+ insn.size =3D size; >+ if (etm_decoder->arch_version =3D=3D ARCH_V7) >+ insn.flags =3D cs_etm_get_isa_flag (elem); >+ >+ if (i =3D=3D elem->num_instr_range -1) >+ insn.iclass =3D cs_etm_get_instruction_class (elem); >+ >+ ftrace_update_insns (bfun, insn); >+ pc =3D pc + size; There doesn't seem to be a need for a separate PC variable. We can work directly on INSN and initialize INSN.ICLASS outside of the loop as it won't change until the last iteration. Same for SIZE. >+ 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) I'd add the empty line before the comment. The comment refers to the if, c= orrect? >+/* Update btrace in the case of a trace on. */ >+ >+static void >+cs_etm_update_btrace_with_trace_on (const void *context, >+ const ocsd_generic_trace_elem *elem) >+{ >+ gdb_assert (elem->elem_type =3D=3D OCSD_GEN_TRC_ELEM_TRACE_ON); >+ >+ struct cs_etm_decoder *etm_decoder =3D (struct cs_etm_decoder *)context; >+ if (etm_decoder->t_info =3D=3D nullptr) >+ return; >+ >+ struct thread_info *tp =3D etm_decoder->t_info; >+ >+ struct btrace_thread_info *btinfo =3D &tp->btrace; >+ >+ if (elem->trace_on_reason !=3D TRACE_ON_NORMAL) >+ ftrace_new_gap (btinfo, elem->trace_on_reason, etm_decoder->gaps); >+} Same comment about empty lines between declarations. Also, we don't really need TP or BTINFO until we decided to add a gap. Would this guarantee that we resume from the same PC where we stopped previously if the reason is TRACE_ON_NORMAL? >+ >+/* 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) >+ DEBUG ("ETM trace_element: index=3D %s, channel=3D 0x%x, %s", >+ pulongest (index), trace_chan_id, str_buffer); >+ } >+ >+ switch (elem->elem_type) >+ { >+ case OCSD_GEN_TRC_ELEM_TRACE_ON: >+ cs_etm_update_btrace_with_trace_on (context, elem); >+ break; >+ >+ case OCSD_GEN_TRC_ELEM_INSTR_RANGE: >+ cs_etm_update_btrace_with_inst_range (context, elem); >+ break; >+ >+ case OCSD_GEN_TRC_ELEM_EXCEPTION: >+ cs_etm_update_btrace_with_exception (context, elem); >+ break; It may make sense to cast CONTEXT here so we have proper types in all the other functions we're calling from here. >+ >+ /* Not yet handled types, but may be supported in the future. */ >+ case OCSD_GEN_TRC_ELEM_UNKNOWN: >+ case OCSD_GEN_TRC_ELEM_EO_TRACE: >+ case OCSD_GEN_TRC_ELEM_NO_SYNC: >+ case OCSD_GEN_TRC_ELEM_EXCEPTION_RET: >+ case OCSD_GEN_TRC_ELEM_TIMESTAMP: >+ case OCSD_GEN_TRC_ELEM_PE_CONTEXT: >+ case OCSD_GEN_TRC_ELEM_ADDR_NACC: >+ case OCSD_GEN_TRC_ELEM_CYCLE_COUNT: >+ case OCSD_GEN_TRC_ELEM_ADDR_UNKNOWN: >+ case OCSD_GEN_TRC_ELEM_EVENT: >+ case OCSD_GEN_TRC_ELEM_SWTRACE: >+ case OCSD_GEN_TRC_ELEM_CUSTOM: >+ default: >+ break; >+ >+ } No need for an empty line here. I also don't see a reason for listing all the unsupported types as long as we have a default. If it's an enum, you might want to remove the default to get compiler warnings when the enum changes. >+ return OCSD_RESP_CONT; >+} >+ >+/* Create a cs_etm_decoder and initialize it. */ >+ >+static bool >+cs_etm_create_decoder (struct cs_etm_trace_params *t_params, >+ struct cs_etm_decoder *decoder) >+{ >+ const char *decoder_name; >+ ocsd_etmv3_cfg config_etmv3; >+ ocsd_etmv4_cfg trace_config_etmv4; The naming seems inconsistent. One is with TRACE_ prefix, the other isn't. >+ void *trace_config; >+ switch (t_params->protocol) >+ { >+ case OCSD_PROTOCOL_ETMV3: >+ case OCSD_PROTOCOL_PTM: >+ cs_etm_get_etmv3_config (t_params, &config_etmv3); >+ decoder_name =3D (t_params->protocol =3D=3D OCSD_PROTOCOL_ETMV3) >+ ? OCSD_BUILTIN_DCD_ETMV3 : OCSD_BUILTIN_DCD_PTM; >+ trace_config =3D &config_etmv3; >+ decoder->arch_version =3D ARCH_V7; >+ break; >+ >+ case OCSD_PROTOCOL_ETMV4I: >+ cs_etm_get_etmv4_config (t_params, &trace_config_etmv4); >+ decoder_name =3D OCSD_BUILTIN_DCD_ETMV4I; >+ trace_config =3D &trace_config_etmv4; >+ decoder->arch_version =3D ARCH_V8; >+ break; >+ >+ default: >+ decoder->arch_version =3D ARCH_UNKNOWN; >+ DEBUG ("cs_etm_create_decoder: Unknown architecture version"); >+ return false; >+ >+ } >+ uint8_t csid; >+ if (ocsd_dt_create_decoder (decoder->dcd_tree, decoder_name, >+ OCSD_CREATE_FLG_FULL_DECODER, >+ trace_config, &csid)) Please add explicit comparisons against zero - unless this function is retu= rning bool, of course. >+ { >+ DEBUG ("ocsd_dt_create_decoder failed"); >+ return false; Is there some error code/message that would help the user understand why we failed to create a decoder? >+ } >+ >+ if (ocsd_dt_set_gen_elem_outfn (decoder->dcd_tree, >+ cs_etm_trace_element_callback, >+ decoder)) >+ { >+ DEBUG ("ocsd_dt_set_gen_elem_outfn failed"); >+ return false; Same here. >+ } >+ >+ decoder->prev_return =3D OCSD_RESP_CONT; >+ return true; >+} >+/* Allocate a cs_etm_decoder and initialize it. */ >+ >+static struct cs_etm_decoder * >+cs_etm_alloc_decoder (struct thread_info *tp, int cpu_count, >+ struct cs_etm_decoder_params d_params, Do we really want to pass the struct by-value? >+ std::vector * t_params) >+{ >+ ocsd_dcd_tree_src_t src_type =3D OCSD_TRC_SRC_SINGLE; >+ uint32_t deformatterCfgFlags =3D 0; >+ if (dcdtree_handle =3D=3D C_API_INVALID_TREE_HANDLE) >+ { >+ DEBUG ("ocsd_create_dcd_tree failed"); >+ return nullptr; >+ } >+ struct cs_etm_decoder *decoder; >+ >+ decoder =3D (struct cs_etm_decoder*) xmalloc (sizeof (struct cs_etm_dec= oder)); >+ decoder->dcd_tree =3D dcdtree_handle; >+ >+ bool ret; There is no need to declare this outside of the loop. We're only using it = inside. Is there a better name, maybe? 'if (!ret)' doesn't really describe what ha= ppens. >+ for (int i =3D 0; i < cpu_count; i++) >+ { >+ ret =3D cs_etm_create_decoder (&(t_params->at (i)), decoder); >+ if (!ret) >+ { >+ DEBUG ("cs_etm_create_decoder failed"); >+ cs_etm_free_decoder (decoder); We put all the error details in debug messages. This would be relevant for= the user, too. And if we could get some error code/message from the library th= at would help, as well. >+ return nullptr; >+ } >+ >+ } >+ decoder->t_info =3D tp; I'd put the empty line after the }, not before. >+ return decoder; >+} >+ >+/* A callback function to allow the trace decoder to read the inferior's >+ memory. */ >+ >+static uint32_t >+btrace_etm_readmem_callback (const void *p_context, const ocsd_vaddr_t >address, >+ const ocsd_mem_space_acc_t mem_space, >+ const uint32_t reqBytes, uint8_t *byteBuffer) >+{ >+ int result, errcode; Please declare on first use. >+ >+ result =3D (int) reqBytes; >+ try >+ { >+/* Process an etm traces data block. */ Please describe the return value. >+ >+static int >+cs_etm_process_data_block (struct cs_etm_decoder *decoder, >+ uint64_t index, const uint8_t *buf, >+ size_t len, size_t *consumed) >+{ >+ int ret =3D 0; >+ ocsd_datapath_resp_t cur =3D OCSD_RESP_CONT; What does CUR mean? After reading the code is looks like it means the current decoder's return = value. It also looks like we don't need two variables. The only case where this m= akes a difference is if len =3D=3D 0 and decoder->prev_return !=3D OCSD_RESP_CON= T. And the error case. >+ ocsd_datapath_resp_t prev_return =3D decoder->prev_return; >+ size_t processed =3D 0; >+ uint32_t count; >+ >+ while (processed < len) >+ { >+ if (OCSD_DATA_RESP_IS_WAIT (prev_return)) >+ { >+ cur =3D ocsd_dt_process_data (decoder->dcd_tree, OCSD_OP_FLUSH, >+ 0, 0, nullptr, nullptr); >+ } else if (OCSD_DATA_RESP_IS_CONT (prev_return)) The else goes onto the next line. >+ { >+ cur =3D ocsd_dt_process_data (decoder->dcd_tree, >+ OCSD_OP_DATA, >+ index + processed, len - processed, >+ &buf[processed], &count); >+ processed +=3D count; >+ } else >+ { >+ DEBUG_FTRACE ("ocsd_dt_process_data returned with %d.\n", cur); >+ ret =3D -EINVAL; >+ break; >+ } >+ >+ /* Return to the input code if the packet buffer is full. >+ Flushing will get done once the packet buffer has been >+ processed. */ >+ if (OCSD_DATA_RESP_IS_WAIT (cur)) >+ break; >+ >+ prev_return =3D cur; >+ } >+ >+ decoder->prev_return =3D cur; >+ *consumed =3D processed; >+ >+ return ret; >+} >+static void >+btrace_compute_ftrace_etm (struct thread_info *tp, >+ const struct btrace_data_etm *btrace, >+ std::vector &gaps) >+{ >+ DEBUG_FTRACE ("btrace->size is 0x%x for thread %s", >+ (unsigned int)(btrace->size), print_thread_id (tp)); >+ if (btrace->size =3D=3D 0) >+ return; >+ >+ struct btrace_thread_info *btinfo =3D &tp->btrace; I assume this won't change. We might declare a reference, instead. >+ if (btinfo->functions.empty ()) >+ btinfo->level =3D 0; >+ >+ struct cs_etm_decoder *decoder >+ =3D cs_etm_alloc_decoder (tp,btrace->config.cpu_count, Space after , >+ btrace->config.etm_decoder_params, >+ btrace->config.etm_trace_params); >+ if (decoder =3D=3D nullptr) >+ error (_("Failed to allocate ARM CoreSight ETM Trace decoder.")); >+ >+ ocsd_err_t ocsd_error >+ =3D cs_etm_add_mem_access_callback (decoder, >+ (CORE_ADDR)0x0L, (CORE_ADDR) -1L, Space after ) in the cast. >+ btrace_etm_readmem_callback); >+ if (ocsd_error !=3D OCSD_OK) >+ error (_("Failed to add CoreSight Trace decoder memory access callbac= k.")); I see we're just delaying the error () until here. Is there a reason we're= not throwing right away and instead return and then throw in the caller? Would it make sense to include the error code in the error message to give = more information to the user? >+ >+ size_t consumed; >+ int errcode >+ =3D cs_etm_process_data_block (decoder, 0, btrace->data, btrace->size, >+ &consumed); >+ if (errcode !=3D 0) >+ { >+ warning (_("Error 0x%" PRIx32 " in decoding ARM CoreSight ETM Trace= s" >+ " at offset %zu."), errcode, consumed); >+ if (errcode =3D=3D OCSD_RESP_FATAL_INVALID_DATA) >+ ftrace_new_gap (btinfo, errcode, decoder->gaps); A gap is used to indicate that the trace it not contiguous. How about the = other error codes? >+ } >+ >+ ftrace_compute_global_level_offset (btinfo); >+ btrace_add_pc (tp); >+ btrace_print_all (btinfo); >+ cs_etm_free_decoder (decoder); >+ >+} No need for an empty line before a }. > /* The fetch_registers method of target record-btrace. */ > > void >@@ -1580,15 +1613,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. */ >+ if (regno >=3D 0 && !(regno =3D=3D pcreg || regno =3D=3D ARM_PS_REG= NUM)) > return; This is generic code. ARM_PS_REGNUM may mean something different for other architectures. I see how we handle it below but it is still confusing at this point. Do we even need these two levels of checks? We're repeating the check again below since we need to do different things. It would be cleaner to first check the architecture but I see how this is more efficient. Let's keep it that way until we need to handle more registers or more architectures. >+ if (regno =3D=3D pcreg) That doesn't handle the -1 case for fetching all registers. >+ { > regcache->raw_supply (regno, &insn->pc); >+ return; >+ } >+ if (regno =3D=3D ARM_PS_REGNUM) Same here. >+ { >+ /* 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) Is that sufficient? >+ { >+ int cpsr; >+ cpsr =3D cs_etm_reconstruct_cpsr_iset_state (insn); >+ 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..d431a616a83 100644 >--- a/gdbsupport/btrace-common.h >+++ b/gdbsupport/btrace-common.h >@@ -187,6 +187,96 @@ struct btrace_data_pt > size_t size; > }; > >+/* Parameters needed for decoding ETMv3 traces. >+ See open source coresight trace decoder library (opencsd) >+ for further details. */ >+struct cs_etmv3_trace_params Aren't those structures declared in the decoder's header file? We shouldn't repeat those declarations here. >+/* Parameters of trace source. */ >+struct cs_etm_trace_params >+{ Same here and more below. If we do need to declare them in GDB, they need comments for each field. >+/* Configuration information to go with the etm trace data. */ >+struct btrace_data_etm_config >+{ >+ /* Count of the CPUs (trace sources). */ >+ int cpu_count; >+ std::vector *etm_trace_params; >+ struct cs_etm_decoder_params etm_decoder_params; >+}; Each field needs a comment. For etm_trace_params, for example, it would be interesting whether the vector is per-cpu. Why would we use a pointer to a vector instead of declaring the vector here? >+ >+/* Branch trace in ARM Processor Trace format. */ >+struct btrace_data_etm >+{ >+ /* Some configuration information to go with the data. */ >+ struct btrace_data_etm_config config; >+ >+ /* The trace data. */ >+ gdb_byte *data; >+ >+ /* The size of DATA in bytes. */ >+ size_t size; >+ >+ /* Trace id for this thread. Set to 0xFF to ignore it during parsing. = */ >+ int trace_id; Does that imply that trace_id's are in the range 0..0xff-1? In that case, = uint8_t would be more appropriate. Is that a realistic limit? How would we handle more threads than we have i= d's for? 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