From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x832.google.com (mail-qt1-x832.google.com [IPv6:2607:f8b0:4864:20::832]) by sourceware.org (Postfix) with ESMTPS id 995D83844079 for ; Fri, 3 Jul 2020 15:36:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 995D83844079 Received: by mail-qt1-x832.google.com with SMTP id w27so1108339qtb.7 for ; Fri, 03 Jul 2020 08:36:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=/ogC01Y+jnAHSbi7DV6Ruv1a8kcDNHhCxwmLr167DUs=; b=CIg4NDutZUfXfp5tyeeCR9T0iTArfu/t4vXU/exgsEGd+f93PrTfOV07mIP95jfLtL CDRfq+Md1TG4IIBhx19DJf1Ko2OfXxdHGgx7XtF4IAUTed2rvSxjBPrlzSeHCgpc6iBZ hXWPNieN/WJbPjFB2Rc9BwmlEI3fAi6ZGTKaX3y0tbRXrMKPSMoxUb94A7gigbE3h//O Riw/k6tf1xiCnqWZEWa14FW3MQfXVYjTkxGBuvhSqu6SLgKDIBz4lwawj1NDx4AaLCgD p1bl2ovt3UvtE4D3i4RMQkw/W2ef77K+YXeLDWiNtxiv1kQE9zpDSqR6M0XyhIY7e13L 9iKg== X-Gm-Message-State: AOAM532j7ecCXO8gCkc0Hd/8ENeC495Pw6bFSJafUNggO7/+10kXirZi uhYHA7yPwFeL7uWLHT0xnt+A71cGJ3E= X-Google-Smtp-Source: ABdhPJwCdeHK1W/ywH6AiQKv+eZDEeGissHBoa0BwoR2xMkxkjq/qyid3UREyBhG4oe2MUiDlfFtyg== X-Received: by 2002:ac8:47ce:: with SMTP id d14mr36906975qtr.285.1593790592019; Fri, 03 Jul 2020 08:36:32 -0700 (PDT) Received: from ?IPv6:2804:7f0:8283:20c3:e864:896a:f2c8:27a0? ([2804:7f0:8283:20c3:e864:896a:f2c8:27a0]) by smtp.gmail.com with ESMTPSA id z17sm12464525qth.24.2020.07.03.08.36.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Jul 2020 08:36:31 -0700 (PDT) Subject: Re: [Patch] GDB: aarch64: Add ability to step over a BR/BLR instruction To: Matthew Malcomson , gdb-patches@sourceware.org, Alan Hayward References: From: Luis Machado Message-ID: <9226b8ae-aaea-65c3-3e86-f607b11fd375@linaro.org> Date: Fri, 3 Jul 2020 12:36:27 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, 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: Fri, 03 Jul 2020 15:36:34 -0000 cc-ing Alan. On 7/3/20 11:55 AM, Matthew Malcomson wrote: > Enable displaced stepping over a BR/BLR instruction > > Displaced stepping over an instruction executes a instruction in a > scratch area and then manually fixes up the PC address to leave > execution where it would have been if the instruction were in its > original location. > > The BR instruction does not need modification in order to run correctly > at a different address, but the displaced step fixup method should not > manually adjust the PC since the BR instruction sets that value already. > > The BLR instruction should also avoid such a fixup, but must also have > the link register modified to point to just after the original code > location rather than back to the scratch location. Nice catch. > > This patch adds the above functionality. > We add this functionality by modifying aarch64_displaced_step_others > rather than by adding a new visitor method to aarch64_insn_visitor. > We choose this since it seems that visitor approach is designed > specifically for PC relative instructions (which must always be modified > when executed in a different location). > > It seems that the BR and BLR instructions are more like the RET > instruction which is already handled specially in > aarch64_displaced_step_others. > > This also means the gdbserver code to relocate an instruction when > creating a fast tracepoint does not need to be modified, since nothing > special is needed for the BR and BLR instructions there. > > > Manually tested on AArch64 (it doesn't look like there are tests for > displaced stepping on the other instructions that are manually handled, > so I figured adding a testcase for BR and BLR would be out of place). Not out of place, but those just did not get added. A test that exercises displaced stepping over those two instructions would be a good addition. That or some unit testing code to make sure the function handled the instruction in the expected way. > > > ------##### > Observed (mis)behaviour before was that displaced stepping over a BR or > BLR instruction would not execute the function they called. Most easily > seen by putting a breakpoint with a condition on such an instruction and > a print statement in the functions they called. > When run with the breakpoint enabled the function is not called and > "numargs called" is not printed. > When run with the breakpoint disabled the function is called and the > message is printed. > > --- GDB Session > hw-a20-10:gcc-source [15:57:14] % gdb ../using-blr > Reading symbols from ../using-blr...done. > (gdb) disassemble blr_call_value > Dump of assembler code for function blr_call_value: > ... > 0x0000000000400560 <+28>: blr x2 > ... > 0x00000000004005b8 <+116>: ret > End of assembler dump. > (gdb) break *0x0000000000400560 > Breakpoint 1 at 0x400560: file ../using-blr.c, line 22. > (gdb) condition 1 10 == 0 > (gdb) run > Starting program: /home/matmal01/using-blr > [Inferior 1 (process 33279) exited with code 012] > (gdb) disable 1 > (gdb) run > Starting program: /home/matmal01/using-blr > numargs called > [Inferior 1 (process 33289) exited with code 012] > (gdb) > > Test program: > ---- using-blr ---- > \#include > typedef int (foo) (int, int); > typedef void (bar) (int, int); > struct sls_testclass { > foo *x; > bar *y; > int left; > int right; > }; > > __attribute__ ((noinline)) > int blr_call_value (struct sls_testclass x) > { > int retval = x.x(x.left, x.right); > if (retval % 10) > return 100; > return 9; > } > > __attribute__ ((noinline)) > int blr_call (struct sls_testclass x) > { > x.y(x.left, x.right); > if (x.left % 10) > return 100; > return 9; > } > > int > numargs (__attribute__ ((unused)) int left, __attribute__ ((unused)) int right) > { > printf("numargs called\n"); > return 10; > } > > void > altfunc (__attribute__ ((unused)) int left, __attribute__ ((unused)) int right) > { > printf("altfunc called\n"); > } > > int main(int argc, char **argv) > { > struct sls_testclass x = { .x = numargs, .y = altfunc, .left = 1, .right = 2 }; > if (argc > 2) > { > blr_call (x); > } > else > blr_call_value (x); > return 10; > } > > ------ > > gdb/ChangeLog: > > 2020-07-03 Matthew Malcomson > > * aarch64-tdep.c (aarch64_displaced_step_others): Account for > BR and BLR instructions. > * arch/aarch64-insn.h (enum aarch64_opcodes): Add BR instruction. > > > > ############### Attachment also inlined for ease of reply ############### > > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 5e7d0d0b8682af04ce4f01fd999d26c9eb459932..640a3e302f8e2b5fac3575e2f37212d40441d318 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -2974,15 +2974,22 @@ aarch64_displaced_step_others (const uint32_t insn, > struct aarch64_displaced_step_data *dsd > = (struct aarch64_displaced_step_data *) data; > > - aarch64_emit_insn (dsd->insn_buf, insn); > - dsd->insn_count = 1; > - > - if ((insn & 0xfffffc1f) == 0xd65f0000) > + uint32_t masked_insn = (insn & 0xfffffc1f); > + if (masked_insn == BLR) > { > - /* RET */ > - dsd->dsc->pc_adjust = 0; > + /* Emit a BR to the same register and then update LR to the original > + address (similar to aarch64_displaced_step_b). */ > + aarch64_emit_insn (dsd->insn_buf, insn & 0xffdfffff); > + regcache_cooked_write_unsigned (dsd->regs, AARCH64_LR_REGNUM, > + data->insn_addr + 4); > } > else > + aarch64_emit_insn (dsd->insn_buf, insn); > + dsd->insn_count = 1; > + > + if (masked_insn == RET || masked_insn == BR || masked_insn == BLR) > + dsd->dsc->pc_adjust = 0; > + else > dsd->dsc->pc_adjust = 4; > } > > diff --git a/gdb/arch/aarch64-insn.h b/gdb/arch/aarch64-insn.h > index 6a63ce9c2005acd6fe018a12c640f1be01751d6b..6b8721139f8446d82aecac243501d31137c885a5 100644 > --- a/gdb/arch/aarch64-insn.h > +++ b/gdb/arch/aarch64-insn.h > @@ -40,7 +40,9 @@ enum aarch64_opcodes > CBNZ = 0x21000000 | B, > TBZ = 0x36000000 | B, > TBNZ = 0x37000000 | B, > + /* BR 1101 0110 0001 1111 0000 00rr rrr0 0000 */ > /* BLR 1101 0110 0011 1111 0000 00rr rrr0 0000 */ > + BR = 0xd61f0000, > BLR = 0xd63f0000, > /* RET 1101 0110 0101 1111 0000 00rr rrr0 0000 */ > RET = 0xd65f0000, > The patch looks good to me.