From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nx235.node01.secure-mailgate.com (nx235.node01.secure-mailgate.com [89.22.108.235]) by sourceware.org (Postfix) with ESMTPS id 8412F3858407 for ; Thu, 7 Apr 2022 16:33:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8412F3858407 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=trande.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=trande.de Received: from host202.checkdomain.de ([185.137.168.148]) by node01.secure-mailgate.com with esmtps (TLSv1.2:AES128-GCM-SHA256:128) (Exim 4.92) (envelope-from ) id 1ncV4g-006vtr-H4; Thu, 07 Apr 2022 18:33:36 +0200 X-SecureMailgate-Identity: zied.guermazi@trande.de;host202.checkdomain.de Received: from [192.168.178.42] (unknown [77.189.107.161]) (Authenticated sender: zied.guermazi@trande.de) by host202.checkdomain.de (Postfix) with ESMTPSA id BA0E3280114; Thu, 7 Apr 2022 18:33:31 +0200 (CEST) X-SecureMailgate-Identity: zied.guermazi@trande.de;host202.checkdomain.de Message-ID: <94c5034f-c042-9e24-8120-17b3ca59287b@trande.de> Date: Thu, 7 Apr 2022 18:33:31 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v6 3/7] start/stop btrace with coresight etm and parse etm buffer. nat independant Content-Language: en-US To: "Metzger, Markus T" Cc: "gdb-patches@sourceware.org" References: <20210531213307.275079-1-zied.guermazi@trande.de> <20210531213307.275079-4-zied.guermazi@trande.de> From: Zied Guermazi In-Reply-To: X-PPP-Message-ID: <20220407163332.3525949.59026@host202.checkdomain.de> X-PPP-Vhost: trande.de X-Originating-IP: 185.137.168.148 X-SecureMailgate-Domain: host202.checkdomain.de X-SecureMailgate-Username: 185.137.168.148 Authentication-Results: secure-mailgate.com; auth=pass smtp.auth=185.137.168.148@host202.checkdomain.de X-SecureMailgate-Outgoing-Class: ham X-SecureMailgate-Outgoing-Evidence: SB/global_tokens (0.000426873337045) X-Recommended-Action: accept X-Filter-ID: Pt3MvcO5N4iKaDQ5O6lkdGlMVN6RH8bjRMzItlySaT9J9tL6Kw0b9fUQd7fV0xEiPUtbdvnXkggZ 3YnVId/Y5jcf0yeVQAvfjHznO7+bT5xepKJyYLZISh0ScmnJxtOQL++T0fzDkcbraJCnPSgcQrqW ghmgrvbuYBMbgPFqhEjKF590ee3aGyyzch48U9RYPumU89c8OaxcWLmp7ynpEm6Y8sotIaA0keip PE7FQyLasE0yYSwoSJtDBJTXEF44+qKA9WNimSzzMEEnsLmJnD3bQmEqUddg4pDcHxi62gxzSOZO aYOlkYc6lCBjGxXRtrdIeJGUu23VykQIijZdTFmpY3ZEt1L/xHpIrLXSv09dmhE0Cjtkk/HF/Kxy ZirNDU5uCP5D/CUxYBzL65HHiH6YFrhxVw5i30CZMe0+JKoB15eMIME6iZU0/33rc4pTBldNoc4/ ugpbufbubK+x4vA/QHsSr98zh4ylHHSOvF4EizIlMC06U8aFKFbkglvNo7UbuWzwvM5eRjzlfUYD MWFgOK8gW+hazh84oBeMtGGs+REr5it4yuwWmXN5FmKajBUnmlmmmmIH8Dg80WcRp+fC5fGvBaRW nWe6WFte+h1HBQQL7n9+OTZ37LZfI/sW30YZMgUX6Z4ThlOFxTu4LUsF4GHuFWly2YtZO0AGnGQ3 T2YxzxB8woiuExubdcsml481x58OeqpgxEwDRkEDKwPt8rNcco5Lt6ku63DkksyWUQhsiRV7ToHt 3HSONnNjcxw3qqhc+N6cuEg4XWh5Fqb0FldYqp+kcqm+1aM0u7RGQPqAhPdEtGO1frzSxSH3RtxP 5WoBRP816PbH6q4jf+gZ8kRW3yj6wqwjQ36QzRPL7Rp7sTyiVa8HG50AexxrzznCpn9MRUzzcEJz OzmIv9M+m4WpRRDP6YzwkAPgQJbZw2WK4NzRiZTDVGmt15BzNCflaPsIqsBW58vnzO6f80mTBSgy W5TU2KcEYyxOlX4SCYiqA9Jn2sfHWQIwAWSvctgzcDoFd+96Xw4QUNtTnZZhiw7af43YPLSeyfeI qT7OqoqMIvCpZajrvxuS+2n0bmim7U+d9AZiaR2WxIbRcfP4wHlqBE95R7lrKCg7FGkTm3FqRDxU q50C08gBiOmGBiFMXZsNOH+1R01VtZLf6H6YFrhxVw5i30CZMe0+JKqWmKu/Exva/4bO4tpXcmDO vZQjnSGPGpsSE/xM+RnyGGWi2HOzq/csyFjkk1RUX5M= X-Report-Abuse-To: spam@node04.secure-mailgate.com X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00, BODY_8BITS, FOREIGN_BODY1, GIT_PATCH_0, HTML_MESSAGE, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, NICE_REPLY_A, RCVD_IN_BARRACUDACENTRAL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Thu, 07 Apr 2022 16:33:43 -0000 Hello Markus, thanks for your feedback. Below are the answers to your queries. On 22.06.21 16:59, Metzger, Markus T wrote: > 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 behavior > 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. [Zied] yes, the code is patched with an undefined instruction that rises an exception to trigger the breakpoint. the traces decoder gets the attempt to execute the instruction in the trace and the exception, but decodes it as the original instruction, since the code was 'unpatched' in between. we do not add the instruction back manually. but when the execution continue, and depending on the user action, the same instruction may be re-executed. we do not re-insert it manually, the decoder will decode it from the traces. therefore we can ignore the removed instruction. following comment is added in cs_etm_update_btrace_with_exception "Handle the implementation of breakpoints in gdb for arm (v7) architecture      using undefined instructions.      In arm (v7), breakpoints are implemented by patching the code with an      undefined instruction. When the breakpoint is hit the code is 'un-patched'.      In the traces we see the exception of the undefined instruction      and we get the original instruction from the decoder.      When we continue the execution the original instruction will be      re-executed, and we will get it again in the traces. To solve      getting the instruction twice, the first occurrence is removed,      since it was actually not executed." > >> + >> +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 = &btinfo->functions.back (); >> + /* If we had a gap before, we return. */ >> + if (bfun->errcode != 0) >> + return; > In which case can we have a gap? [Zied] this is to make sure that following calls are safe to be executed. >> + >> + 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? [Zied] yes, we are sure that we have at least the last executed instruction that raised the breakpoint execption. > >> + 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? [Zied] since I listed all possible values that may get returned by the decoder, the default case means that gdb and the decoder are doing something wrong and this should be handled as a fatal error. This code is to be revisited if in the future, ARM adds other ISAs, and we would like to support them. > > >> + insn.iclass = BTRACE_INSN_OTHER; >> + insn.pc = elem->st_addr; >> + for (int i = 0; i< elem->num_instr_range; i++) >> + { >> + try >> + { >> + insn.size = 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 = ftrace_update_function (btinfo, insn.pc); >> + if (etm_decoder->arch_version == ARCH_V7) >> + insn.flags = cs_etm_get_isa_flag (elem); > ELEM isn't changing so we could set this once outside of the loop. [Zied] moved outside > >> + >> + if (i == elem->num_instr_range -1) >> + insn.iclass = cs_etm_get_instruction_class (elem); >> + >> + ftrace_update_insns (bfun, insn); >> + insn.pc = 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 == OCSD_GEN_TRC_ELEM_EXCEPTION); >> + >> + struct thread_info *tp = etm_decoder->t_info; >> + struct btrace_thread_info *btinfo = &tp->btrace; >> + >> + /* Handle the implementation of breakpoints in gdb for arm (v7) architecture >> + using undefined instructions. */ >> + if (etm_decoder->arch_version == ARCH_V7) >> + { >> + if (elem->exception_number >> + == 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 really plan > to re-insert it again. [Zied] we will not insert it manually. it will be inserted as part of the executed code when the user continues the execution > > Can we ensure that we actually inserted that instruction into the current BFUN? > We wouldn't need the helper function and all those checks would become asserts. > > I like little helper functions but if we just removed the insn here it would 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 == OCSD_GEN_TRC_ELEM_TRACE_ON); >> + >> + if (elem->trace_on_reason != TRACE_ON_NORMAL) >> + { >> + struct thread_info *tp = etm_decoder->t_info; >> + struct btrace_thread_info *btinfo = &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. [Zied] yes, but I do not have here a mean for checking if the PC has moved. The pc field is not valid in this kind of elements > > >> +} >> + >> +/* 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 != 0) >> + { >> + char str_buffer[128]; >> + if (ocsd_gen_elem_str (elem, str_buffer, 128) == OCSD_OK) > sizeof (str_buffer) [Zied] done. > >> +/* 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] = 0; >> + else >> + string_buffer[str_len] = 0; > We can simply adjust str_len before the memcpy. There's no reason to declare > it const; it is passed by-value. > > >> + uint8_t csid; >> + errcode = ocsd_dt_create_decoder (decoder->dcd_tree, decoder_name, >> + OCSD_CREATE_FLG_FULL_DECODER, >> + trace_config, &csid); >> + if (errcode != OCSD_OK) >> + { >> + warning (_("ocsd_dt_create_decoder failed with error: %d"), errcode); >> + return errcode; >> + } >> + >> + errcode = ocsd_dt_set_gen_elem_outfn (decoder->dcd_tree, >> + cs_etm_trace_element_callback, >> + decoder); >> + if (errcode != OCSD_OK) >> + { >> + warning (_("ocsd_dt_set_gen_elem_outfn failed failed with error: %d"), >> + errcode); >> + return errcode; >> + } >> + >> + errcode = ocsd_def_errlog_init (OCSD_ERR_SEV_ERROR, 1); >> + if (errcode != OCSD_OK) >> + { >> + warning (_("ocsd_def_errlog_init failed failed with error: %d"), >> + errcode); >> + return errcode; >> + } >> + >> + /* Initialize error printer. */ >> + errcode = ocsd_def_errlog_config_output (C_API_MSGLOGOUT_FLG_NONE, >> nullptr); >> + if (errcode != OCSD_OK) >> + { >> + warning (_("ocsd_def_errlog_init failed failed with error: %d"), >> + errcode); >> + return errcode; >> + } >> + >> + errcode = ocsd_def_errlog_set_strprint_cb (decoder->dcd_tree, nullptr, >> + cs_etm_print_error_log); >> + if (errcode != OCSD_OK) >> + { >> + warning (_("ocsd_def_errlog_set_strprint_cb failed failed with error: %d"), >> + errcode); >> + return errcode; >> + } >> + >> + decoder->prev_return = OCSD_RESP_CONT; > There are several error returns that leave this field uninitialized. I assume that > the decoder is not working in those cases and will not be used. Don't we need to > undo anything before returning with an error? [Zied] all those functions are setting some fields in the decoder. If something goes wrong the caller frees the decoder. > > I see that we call cs_etm_free_decoder () below on errors, which probably takes > care of cleaning up partially complete decoders. It still feels odd to return an error > and leave a decoder partially initialized. > > Let's at least note that in the comment that on error, the caller is expected to > call cs_etm_free_decoder (). [Zied] Documented in the comment. > > >> + struct cs_etm_decoder *decoder; >> + >> + decoder = (struct cs_etm_decoder*) xmalloc (sizeof (struct cs_etm_decoder)); >> + decoder->dcd_tree = dcdtree_handle; >> + >> + for (int i = 0; i < cpu_count; i++) >> + { >> + ocsd_err_t errcode = cs_etm_create_decoder (&(t_params->at (i)), decoder); >> + if (errcode != 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 = OCSD_RESP_CONT; >> + size_t processed = 0; >> + uint32_t count; >> + >> + while (processed < len) >> + { >> + if (OCSD_DATA_RESP_IS_CONT (data_path_return)) >> + { >> + data_path_return = ocsd_dt_process_data (decoder->dcd_tree, >> + OCSD_OP_DATA, >> + index + processed, len - processed, >> + &buf[processed], &count); >> + processed += 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 = 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 = 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 another > error indefinitely. > > We should still be able to interrupt GDB itself using ^C on the CLI if we ever actually > run into an infinite decode loop. So maybe this is all overkill. > [Zied] after discussion with OpenCSD engineer. the current implementation is fine and the comment can be removed > >> + ocsd_err_t ocsd_error >> + = cs_etm_add_mem_access_callback (decoder, >> + (CORE_ADDR) 0x0L, (CORE_ADDR) -1L, >> + btrace_etm_readmem_callback); >> + if (ocsd_error != OCSD_OK) >> + error (_("Failed to add CoreSight Trace decoder memory access callback.")); > Could we print ICSD_ERROR in the error message? Even printing it as an int would help. [Zied] done > > >> 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. [Zied]  few bits  in the insn->flags are used for this purpose, no dedicated field for the cspr is used nymore. > >> + >> +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? [Zied] this should not happens. changed to  internal_error > >> + } >> +} >> + >> /* The fetch_registers method of target record-btrace. */ >> >> void >> @@ -1581,16 +1614,32 @@ record_btrace_target::fetch_registers (struct >> regcache *regcache, int regno) >> pcreg = gdbarch_pc_regnum (gdbarch); >> if (pcreg < 0) >> return; >> - >> - /* We can only provide the PC register. */ >> - if (regno >= 0 && regno != 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. [Zied] done, comment changed to "      /* We can only provide the PC or PS registers here.          PS is provided in the case of an ARMv7 target and it maps to CSPR.          A check for the architecture is done below before providing it */ " > >> + if (regno >= 0 && !(regno == pcreg || regno == ARM_PS_REGNUM)) >> return; >> >> insn = btrace_insn_get (replay); >> - gdb_assert (insn != NULL); >> + gdb_assert (insn != nullptr); >> >> + if ((regno < 0) || (regno == pcreg)) >> + { >> regcache->raw_supply (regno, &insn->pc); >> } >> + if ((regno < 0) || (regno == ARM_PS_REGNUM)) >> + { >> + /* Provide CPSR register in the case of an armv7 target. */ >> + const struct target_desc *tdesc = gdbarch_target_desc (gdbarch); >> + >> + const char *tdesc_name = tdesc_architecture_name (tdesc); >> + if (strcmp (tdesc_name, "arm") == 0) >> + { >> + int cpsr; >> + cpsr = cs_etm_reconstruct_cpsr_iset_state (insn); > Please initialize in the declaration. [Zied] done > >> + 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. [Zied] done > >> + >> +/* 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? [Zied] done. changed to "/* Branch trace in ARM Processor Extended Trace Macrocell format.  */" > > > 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 > Kind Regards Zied Guermazi -- *Zied Guermazi* founder Trande GmbH Leuschnerstraße 2 69469 Weinheim/Germany Mobile: +491722645127 mailto:zied.guermazi@trande.de *Trande GmbH* Leuschnerstraße 2, D-69469 Weinheim; Telefon: +491722645127 Sitz der Gesellschaft: Weinheim- Registergericht: AG Mannheim HRB 736209 - Geschäftsführung: Zied Guermazi *Confidentiality Note* This message is intended only for the use of the named recipient(s) and may contain confidential and/or privileged information. If you are not the intended recipient, please contact the sender and delete the message. Any unauthorized use of the information contained in this message is prohibited.