From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nx218.node01.secure-mailgate.com (nx218.node01.secure-mailgate.com [89.22.108.218]) by sourceware.org (Postfix) with ESMTPS id 99DBE3851C2E for ; Fri, 19 Mar 2021 15:29:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 99DBE3851C2E Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=trande.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=zied.guermazi@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 1lNH4G-003qR4-Lu; Fri, 19 Mar 2021 16:29:47 +0100 X-SecureMailgate-Identity: host202.checkdomain.de Received: from [192.168.178.48] (x4db6ae3b.dyn.telefonica.de [77.182.174.59]) (Authenticated sender: zied.guermazi@trande.de) by host202.checkdomain.de (Postfix) with ESMTPSA id D008D280433; Fri, 19 Mar 2021 16:29:39 +0100 (CET) X-SecureMailgate-Identity: host202.checkdomain.de Subject: Re: [PATCH 3/8] start/stop btrace with coresight etm and parse etm buffer. nat independant To: "Metzger, Markus T" , "gdb-patches@sourceware.org" References: <20210226024602.55057-1-zied.guermazi@trande.de> <20210226024602.55057-4-zied.guermazi@trande.de> From: Zied Guermazi Message-ID: <5df0fe3e-d50f-6991-393a-cad3365a2ad4@trande.de> Date: Fri, 19 Mar 2021 16:29:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-PPP-Message-ID: <20210319152940.3727037.79425@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: Combined (0.02) X-Recommended-Action: accept X-Filter-ID: Pt3MvcO5N4iKaDQ5O6lkdGlMVN6RH8bjRMzItlySaT8cBq1/+GN7IvNKgwr60weePUtbdvnXkggZ 3YnVId/Y5jcf0yeVQAvfjHznO7+bT5xepKJyYLZISh0ScmnJxtOQL++T0fzDkcbraJCnPSgcQrqW ghmgrvbuYBMbgPFqhEjKF590ee3aGyyzch48U9RYPumU89c8OaxcWLmp7ynpEjwozWu5xQh8c9QC v86DJVLasE0yYSwoSJtDBJTXEF44gg6L6tZ9uyr4wDjgXI+ZB2vJfOI+MmJ4cnbgLVfroSeZn7S9 PqmI7CxqPRD6uYFuQu2n3IRyXLYlJgH48npubNP5+9/JM2q4j+X8V5gNTgs3oto0tveWGbkxfhWt KX3eP651KZsadlJw9tyLLsw53LDY27xZiOmTUqT1Bu490izs/sb16ESDOWr/ATs5NT2b+zcdijFn YDwIvvgDwGlGNiMmrde0MvyP/ieh0v1VEtZe4NVbVEBU/8Tv9RjL1Pyue/jZMLlnZ5MyaJ9KSvAz 5neD9ZsF6TMHcgrxFju40AQOZW5Q0/D9wtf4L4mgsdDWiJnlkDz1xp8CrEz5W8nNv+WBOXp8nHKe 0R+FkIqN7hk9jtlS+hZEa0BD3QngxL/Z5WpvB2uzDYVk9RmxleFDghbAiblZVZAoQGrzL+lZC3bY 5Qx4fJOk03R5fJtf/Dv/gSdxjDi1ltP+wptveb2IjRBXBDaqFyX//9NC0J3OHT5ahxBGEe+FeSYj j1w6u4H3615pX5GkO9BM31KMJtnO5MCUxsQMrT3BK1P+UJXdbhVuo+/KsciW8DPtDEJEhFTR+Xmr wCgCbBUKVlMqUBUk3y8qT34v9uqDtnGrhww/+pGkWxo327cJwOSgD7+KmHC1UEIUN3F7U6mYOs2L /vXn7AgnwW5GiYDiNonPFKAeTvCHy2rvLerdCgFKngogi08iXAX6th4fosZAOAQypco/XJ1VgdXM IrLYZ5omU0WhZIb5B00pWiU75qicA8Y7Yswwfqb5R4VemuUI6bcEARsm0I5puxcFdnwkiPj+80yp GW5W6DCz5wPRMYd5BHdH4y1y4R8m1R1p7pEOSdzWX90raOAQGodDd6jxMFMmDPl6FNIDyj6YxUpp 4q379O5Q8jb91ZihD16QiAnNTlvd1f1m3ZmyCtlKNfU+5VB9yzvTGUJCzVDtBsnwuV43B4iNxlyD X-Report-Abuse-To: spam@node04.secure-mailgate.com X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, FOREIGN_BODY1, GIT_PATCH_0, HTML_MESSAGE, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, NICE_REPLY_A, 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 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: Fri, 19 Mar 2021 15:29:52 -0000 hi here is the status of the rework of the patchset. Changes will be published in the next version. On 16.03.21 11:16, Metzger, Markus T wrote: > Hello Zied, > >> I reworked the patch for C coding style. To my knowledge there is not coding styles issues left. > Thanks a lot. That should make it easier to review. [Zied] sorry for the inconvenience and thanks for sharing the script to check patches for formatting issues. > > >> For the point concerning the use of a vector of registers in btrace_insn: >> a vector of registers is added to the btrace_insn structure. This adds an overhead of 3 pointers when the vector is empty. It will be the overhead when not used (intel pt). I think this should be acceptable. > That's doubling the current size; tripling it if we declared ICLASS 8b (as it > should be). I don't think that's acceptable. > > >> on ARMv7 this is need for all instructions, to allow step and next to set the breakpoints properly in the code. > Is this to distinguish ARM and Thumb modes? How would the information > be used? [Zied] let me give you a better insight on this topic to find a better solution: this information is needed for two purposes: - when stepping in the code (next, step, finish), on ARMv7, and due to the limitations of debug block, the debugger (GDB) need to calculate the addresses of  the "landing" instructions. this is implemented in arm_get_next_pcs (arm-get-next-pcs.c) this class request the content of the register cpsr - when setting a breakpoint, either directly by the user, or indirectly by stepping commands (next, nexti, step, stepi, finish), the debugger (GDB) requests the content of the register cpsr. when in replay mode, forwards direction, we relay on the beneath stratum to calculate the "landing" addresses for stepping commands, and we relay also on the beneath stratum for setting breakpoints. it is here about providing the content of this register at any instruction and this while stepping in the code as well as when navigating in the code (record goto command). I have tried to use the debug info from elf files. I was limited by the fact that not all elf files contains a dwarf section (especially the libraries), and that the code can change the mode (arm <-> thumb for example) within the same section though the usage of veneer code. a second constraint to keep in mind, is that once we finish decoding the traces, we lose the cpsr content information. so we have to capture it during decoding. One possibility for "compressing" the cpsr content is to only store it at a change of its content or after a gap. And then take care of providing the right content when stepping and navigating in the traces. One solution is to extend the btrace_function with a vector of btrace_registers i.e. a vector of (instruction number and vector of registers) and to extend btrace_thread_info with iterators for btrace_registers_history .  When the register is requested, we will have to get the instruction, iterate backwards until an entry is found in the btrace_registers of current or previous functions or a gap is found. this solution trades run-time for memory, requires examination of previous instruction each time we decode a new instruction, and has a considerable level of complexity.... considering the complexity of this solution, my recommendation is to keep current implementation and schedule the above proposed approach as an optimization, or to drop support for armv7 until support for data tracing is added. > >> on ARMv8, this is currently not used. >> This implementation paves the way to handling ETM traces with data, so that gdb can have access in addition, to the registers altered during an instruction. This is very useful in execution record and replay since we can then reconstruct (some of) the variables. > I see how this can be very useful. We should definitely support that, but > we need to find a better way of storing the information. > > >> + case ocsd_isa_thumb2: >> + cpsr = 0x20; >> + break; >> + case ocsd_isa_tee: >> + cpsr = 0x1000020; >> + break; >> + case ocsd_isa_jazelle: >> + cpsr = 0x1000000; >> + break; >> + default: >> + cpsr = 0; >> + } >> It might make sense to turn this if into a small helper function. >> [Zied] it can be done, I am keeping it as is since there is no reuse. > It reduces the size of the function. Even if there is no reuse it improves > readability. [Zied] done > > >> + ARM_PS_REGNUM, (gdb_byte *)&cpsr); >> + insn.registers.push_back (reg); >> + } >> + if (i == elem->num_instr_range -1) >> + { >> + 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: >> + insn.iclass=BTRACE_INSN_RETURN; >> + break; >> + case OCSD_S_INSTR_BR_LINK: >> + insn.iclass=BTRACE_INSN_CALL; >> + break; >> + case OCSD_S_INSTR_NONE: >> + insn.iclass=BTRACE_INSN_JUMP; >> + } >> + break; >> + case OCSD_INSTR_ISB: >> + case OCSD_INSTR_DSB_DMB: >> + case OCSD_INSTR_WFI_WFE: >> + case OCSD_INSTR_OTHER: >> + insn.iclass=BTRACE_INSN_OTHER; >> + break; >> + default: >> + break; >> + } >> This switch could be a small helper function, too. >> [Zied] it can be done, I am keeping it as is since there is no reuse. > Same here. All this text could be just one line. > > [Zied] done >> + } >> + ftrace_update_insns (bfun, insn); >> + pc = pc + size; >> + } >> + } >> +} >> +#undef ARM_PS_REGNUM >> + >> +#define ARM_EXCEPTION_UNDEFINED_INSTRUCTION 9 >> Should this go into arch/arm.h? >> [Zied] hardware exceptions are not listed in this header file. > If we need to enumerate them somewhere, I think it should be in this file. [Zied] this define is for the exception numbers as defined in the etm traces, and is different for the "normal" exception number. Let me check if  arch/arm.h is the right file for it and fix it. > > >> + if (elem->exception_number == >> ARM_EXCEPTION_UNDEFINED_INSTRUCTION) >> + { >> + DEBUG ("handle breakpoints implementation in gdb for armv7"); >> + ftrace_remove_last_insn(btinfo); >> Could we detect this before we added the instruction? >> [Zied] Unfortunately not. On armv7 the processor attempts first to execute (and logs) the instruction, then it raises the exception. > I meant before we added the instruction to the trace. [Zied] no, it is not possible without distording the code. removing previous inserted but not executed command is more appropriate. below is an illustration of the outcome of the decoder. [btrace] ETM trace_element: index= 12, channel= 0x10, OCSD_GEN_TRC_ELEM_INSTR_RANGE(exec range=0x40050a:[0x40050c] num_i(1) last_sz(2) (ISA=T32) E --- ) <-- here the processor attempt to execute the instruction and traces it, the original instruction is patched with an undefined instruction opcode btrace] ETM trace_element: index= 13, channel= 0x10, OCSD_GEN_TRC_ELEM_EXCEPTION(excep num (0x09) ) <-- here the exception is raised btrace] ETM trace_element: index= 54, channel= 0x10, OCSD_GEN_TRC_ELEM_TRACE_ON( [begin or filter]) <-- restarting traces after coming back from kernel space [btrace] ETM trace_element: index= 54, channel= 0x10, OCSD_GEN_TRC_ELEM_PE_CONTEXT((ISA=T32) N; 32-bit; ) <-- setting the context again [btrace] ETM trace_element: index= 60, channel= 0x10, OCSD_GEN_TRC_ELEM_INSTR_RANGE(exec range=0x40050a:[0x40050c] num_i(1) last_sz(2) (ISA=T32) E --- ) <-- here the processor attempt to execute the instruction and succeed since the original opcode is written back to the location. when the first instruction is decoded we do not know if it will fail and raise an exception or not so we add it. when the exception gets raised we remove it. > > >> + default: >> + decoder->arch_version = ARCH_UNKNOWN; >> + return false; >> Wouldn't it make more sense to error () or return an error code? >> There are different things that can go wrong, it seems. >> [Zied] yes, we can add some error code and add some logs according to the error code. Nevertheless, I prefer rather adding logs in the place where it can go wrong, and raise a go/no-go (it is not possible to recover from some errors or handle errors differently) . > I'm fine with issuing warnings right where we detect non-fatal errors and with > error()ing out on fatal errors. > > There's not point in a bool return type, though; void would do just fine. > > >> + decoder->dcd_tree = dcdtree_handle; >> + >> + for (i = 0; i < num_cpu; i++) { >> + ret = cs_etm_create_decoder(&(t_params->at(i)), >> + decoder); >> Is this overwriting the same decoder object? >> [Zied] yes and No :). It updates the dcd_tree with a new node. It overwrites arch_version. This is safe, since there is no Linux kernel running on an ARM multi core MPU with cores belonging to different architectures. > But there's nothing preventing that, either, as long as the kernel sticks to some > common base ISA. > > It seems cleaner to not partially overwrite things. And this doesn't seem to be > performance critical. > > >> + if (ret == false) >> + { >> + ocsd_destroy_dcd_tree(decoder->dcd_tree); >> + free(decoder); >> Do we need to undo anything for the other cpus from 0 to i-1? >> [Zied] no, decoder->dcd_tree holds the nodes of all previous cpus >> >> >> There's a cs_etm_free_decoder function defined below. Should >> this be called to free the decoder? >> [Zied] yes it can be called instead of this peace of code. this is not really needed. > Let's do that. > > >> DEBUG ("enable thread %s (%s)", print_thread_id (tp), >> target_pid_to_str (tp->ptid).c_str ()); >> @@ -1791,6 +2370,9 @@ btrace_stitch_trace (struct btrace_data *btrace, struct >> thread_info *tp) >> case BTRACE_FORMAT_PT: >> /* Delta reads are not supported. */ >> return -1; >> + case BTRACE_FORMAT_ETM: >> Please add an empty line before a new case. >> [Zied] this was a bad conflict handling in an rebase, it is corrected in the next commit. I do not know how to handle it > You'd simply fixup that commit. [Zied] done > > >> diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c >> index 39ac9aafc28..107abd7e653 100644 >> --- a/gdb/record-btrace.c >> +++ b/gdb/record-btrace.c >> @@ -3020,7 +3029,6 @@ cmd_record_btrace_start (const char *args, int from_tty) >> } >> } >> >> - >> Hmmm, I do not see two empty lines at that location. >> [Zied] yes, it is. it was an error during a merge in a rebase. it is now hidden in the history and I do not find a way to fix it. when you squash the patches, the code will look fine. > We're not going to squash the patches. You could rebase and edit the parent commit > to not introduce those empty lines. The change will propagate to this patch and the > hunk will be removed. > > >> +/* The following enum must be aligned with the >> + open source coresight trace decoder library (opencsd). */ >> Could we get them from a header file? >> [Zied] this was intentionally done so, since btrace-common.h is included in many files. This is to allow compiling gdb if opencsd library is not present. > We could conditionally include the header and all code that references anything > declared in it. Would that be feasible? [Zied] done. > > 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 -- *Zied Guermazi* founder Trande UG Leuschnerstraße 2 69469 Weinheim/Germany Mobile: +491722645127 mailto:zied.guermazi@trande.de *Trande UG* 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.