From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id eUPwHZwRD2B/CwAAWB0awg (envelope-from ) for ; Mon, 25 Jan 2021 13:44:44 -0500 Received: by simark.ca (Postfix, from userid 112) id 6CD521EF80; Mon, 25 Jan 2021 13:44:44 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-1.1 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.2 Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 6EF341E945 for ; Mon, 25 Jan 2021 13:44:42 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B55ED3857812; Mon, 25 Jan 2021 18:44:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B55ED3857812 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1611600281; bh=bJ6JmVZsXMUnuf9dc4Bvu0vKk5opPBaluyJQI/UOmbE=; h=Subject:To:References:Date:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=c0FqnUzic48juuJ92HkWXGFxHgghPKO1bSZ+FgLEHlx3BwISdNeuPioFtEyl05Fxi WBNsV+EBoNdFnrPVOmRtzY+Z9VQhDl7v5CwGgcxx8uQbXAqEhPSmci9LRHAHUcYt9A ey26VJGPaVLHvKv3uV6gqHj4rR8YNfI7nTzKCC0I= Received: from mail-qt1-x829.google.com (mail-qt1-x829.google.com [IPv6:2607:f8b0:4864:20::829]) by sourceware.org (Postfix) with ESMTPS id 603A33857812 for ; Mon, 25 Jan 2021 18:44:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 603A33857812 Received: by mail-qt1-x829.google.com with SMTP id v3so10435504qtw.4 for ; Mon, 25 Jan 2021 10:44:38 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=bJ6JmVZsXMUnuf9dc4Bvu0vKk5opPBaluyJQI/UOmbE=; b=V6IaXqrz4vs4xiOoooY+Np2J6jcUsIwDct7/K0fwFoDH/W/LqLJS8N14TUCcwuX/eH 7I1/aoIOjhqiiW5OiQctjaS7nCifXa4eBET1VAjsPh/DDs1GBnU3lvXV3fwcW6H1tC5h m9uYJ0suuj3zb4xCOjdsaMd9yhgu6sPr2axg4AK5yWJ6mzRxVfkQi0FdLSzLqURgzyxP kFZV/6nMd5/p8tmwwCCVxY2zu31EfsUK0hlYfJLyO48CIFGmEqk6Pt1IZG7Yl0X2Bg8h jC5+UR4aJtLuU5LDatMI2KFn6nOpClrkJsx9NjpyPAeqBtTEjbdQq2gK4QVnbGU3TRK9 4SIQ== X-Gm-Message-State: AOAM531DrtklhLogu2JCOPP68Cl7VTAAvU1E2TT8oc7gBKslGclQX8+P MHB+yyMnJoilwrxCHC4Xie1kQw== X-Google-Smtp-Source: ABdhPJx+NsBjbmpln76q7ZEo6oeCCH6dFrqCkrWb89fmz3YkK7oGxsgrPmpVdft/zz3orem4VJua/Q== X-Received: by 2002:ac8:e82:: with SMTP id v2mr1791923qti.164.1611600277768; Mon, 25 Jan 2021 10:44:37 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:874d:fc80:9138:c52e:e4ca? ([2804:7f0:8284:874d:fc80:9138:c52e:e4ca]) by smtp.gmail.com with ESMTPSA id c12sm12007246qtq.76.2021.01.25.10.44.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 25 Jan 2021 10:44:37 -0800 (PST) Subject: Re: [Patch] GDB: aarch64: Add ability to step over a BR/BLR instruction To: Matthew Malcomson References: <9226b8ae-aaea-65c3-3e86-f607b11fd375@linaro.org> <9EACDC38-BB8D-4804-AD19-057E3309819A@arm.com> <3d86bcb9-dedd-6eb2-7cff-e8349d4b20da@palves.net> Message-ID: <9efc84c3-d4fb-03d6-8612-600cc2f74e65@linaro.org> Date: Mon, 25 Jan 2021 15:44:33 -0300 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-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: , From: Luis Machado via Gdb-patches Reply-To: Luis Machado Cc: nd , gdb-patches Errors-To: gdb-patches-bounces@sourceware.org Sender: "Gdb-patches" Hi Matthew, On 1/25/21 3:31 PM, Matthew Malcomson wrote: > I'd like to ping the below patch. > > N.b. When I last sent it up running with `--target_board=native-gdbserver` was not working, but I ran the tests after a rebase just now and everything now passes. > > Given the problem I noticed before was not in this patch (explanation in the previous email), and this patch applies cleanly now, is this good to go in? Sorry. That must have flown under the radar. The attached patch does not contain the new test that was submitted before. Was that an oversight? I think the patch looks good. I just want to make sure I know what pieces to apply. Luis > > Regards, > Matthew > > On 20/08/2020 13:41, Matthew Malcomson wrote: >>> On 7/23/20 5:48 PM, Matthew Malcomson wrote: >>>> + >>>> +# Test for displaced stepping over the BLR instruction. >>>> +gdb_test "run" \ >>>> + "Starting program.*Breakpoint $decimal.*" \ >>>> + "Run until BLR test start" >>>> + >>> >>> Please don't use "run" directly. Use one of runto, runto_main, >>> gdb_run_cmd instead. See amd64-disp-step.exp for example. >>> >>> If you use "run" directly, then the testcase won't run against >>> gdbserver. Please make sure this passes cleanly: >>> >>> $ make check \ >>> RUNTESTFLAGS="--target_board=native-gdbserver" \ >>> TESTS="gdb.arch/aarch64-disp-stepping.exp" >>> >> >> >> Thanks for the suggestion, as it turns out trying to use this meant I noticed a >> bunch of other things, and I couldn't get this to pass cleanly ... >> >> I have now found some existing cases for displaced stepping on AArch64 in >> insn-reloc.c driven by disp-step-insn-reloc.exp. >> Hence I've added the BR and BLR testcases there rather than making my own test >> driver. >> >> However, it seems the existing tests already show there are some problems >> with AArch64 displaced stepping on gdbserver -- it seems there's some problem >> with ensuring the context is the same when running using >> `--target_board=native-gdbserver`. >> I see errors on the existing cbz, tbnz, bcond_true, and bcond_false tests. >> The bl test fails because of an illegal instruction in the bcond_false test >> that only gets run when the test is failing (swithing `b.eq 0b` in that >> function to `b.eq 0f` works for me and I'll make that switch in a different >> patch). >> The new BR and BLR tests also fail from what seems to be using the values of >> the registers as seen by `info registers` which don't appear to be getting >> updated correctly as the program proceeds. >> I can see the same problem on the instruction `mov x1, x2` (that the value of >> x2 used is what GDB prints out with `info registers` rather than the value it >> should be based on the code. >> >> So, the testcase does not pass cleanly with the command you suggested, but I >> think it's not a problem with the changes I've made. >> >> -------- MOV Testcase that fails under gdbserver >> Putting this function in the insn-reloc.c (and placing it in the test array so >> it gets called before the program exits from a broken test) demonstrates that >> displaced stepping doesn't seem to use the correct values from the gdbserver >> context. >> >> >> static void >> can_relocate_mov (void) >> { >> int ok = 0; >> asm (" mov x1, #1\n" >> "set_point15:\n" >> " mov %[ok], x1\n" >> : [ok] "=r" (ok) >> : : "x1"); >> if (ok == 1) >> pass(); >> else >> fail(); >> } >> ------- >> >> >> >> If Ok could someone apply this for me (I don't have commit rights)? >> >> >> ###### Proposed commit message and patch below >> >> 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. >> >> 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. >> >> Regression tests showed nothing untoward on native aarch64. >> I noticed that the disp-step-insn-reloc.exp test produces quite a few >> errors when running with RUNTESTFLAGS="--target_board=native-gdbserver" >> (bcond_true, cbz, tbnz, bcond_false, blr, br). >> There are existing errors, and the BLR and BR tests also fail. >> It seems the context is not preserved properly for displaced >> stepping(for the Conditional instructions the condition flags are not >> preserved, and for BLR/BR the general registers are not preserved). >> The same problem can be observed when using displaced stepping on a >> `mov %[ok], x1` instruction, so I'm confident this is not a problem with >> my patch. >> >> ------##### >> Original 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-08-19 Matthew Malcomson >> >> * aarch64-tdep.c (aarch64_displaced_step_others): Account for >> BLR and BR instructions. >> * arch/aarch64-insn.h (enum aarch64_opcodes): Add BR opcode. >> (enum aarch64_masks): New. >> >> gdb/testsuite/ChangeLog: >> 2020-08-19 Matthew Malcomson >> >> * gdb.arch/insn-reloc.c: Add tests for BR and BLR. >> >> >> >> ############### Attachment also inlined for ease of reply ############### >> >> >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index 5e7d0d0b8682af04ce4f01fd999d26c9eb459932..d247108f53bf045a018b2bf85284088563868ae0 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 & CLEAR_Rn_MASK); >> + 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..f261363feefe4e93e155434ba6d3df8e4b994c9f 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, >> @@ -107,6 +109,14 @@ enum aarch64_opcodes >> NOP = (0 << 5) | HINT, >> }; >> >> +/* List of useful masks. */ >> +enum aarch64_masks >> +{ >> + /* Used for masking out an Rn argument from an opcode. */ >> + CLEAR_Rn_MASK = 0xfffffc1f, >> +}; >> + >> + >> /* Representation of a general purpose register of the form xN or wN. >> >> This type is used by emitting functions that take registers as operands. */ >> diff --git a/gdb/testsuite/gdb.arch/insn-reloc.c b/gdb/testsuite/gdb.arch/insn-reloc.c >> index 106fd6ed1e8cb146863ff767130a82814ee89f86..9e7cf7a12df387e85881e19bdef7372046ba2861 100644 >> --- a/gdb/testsuite/gdb.arch/insn-reloc.c >> +++ b/gdb/testsuite/gdb.arch/insn-reloc.c >> @@ -512,6 +512,99 @@ can_relocate_bl (void) >> : : : "x30"); /* Test that LR is updated correctly. */ >> } >> >> +/* Make sure we can relocate a BR instruction. >> + >> + ... Set x0 to target >> + set_point12: >> + BR x0 ; jump to target (tracepoint here). >> + MOV %[ok], #0 >> + B end >> + target: >> + MOV %[ok], #1 >> + end >> + >> + */ >> + >> +static void >> +can_relocate_br (void) >> +{ >> + int ok = 0; >> + >> + asm (" movz x0, :abs_g3:0f\n" >> + " movk x0, :abs_g2_nc:0f\n" >> + " movk x0, :abs_g1_nc:0f\n" >> + " movk x0, :abs_g0_nc:0f\n" >> + "set_point12:\n" >> + " br x0\n" >> + " mov %[ok], #0\n" >> + " b 1f\n" >> + "0:\n" >> + " mov %[ok], #1\n" >> + "1:\n" >> + : [ok] "=r" (ok) >> + : >> + : "0"); >> + >> + if (ok == 1) >> + pass (); >> + else >> + fail (); >> +} >> + >> +/* Make sure we can relocate a BLR instruction. >> + >> + We use two different functions since the test runner expects one breakpoint >> + per function and we want to test two different things. >> + For BLR we want to test that the BLR actually jumps to the relevant >> + function, *and* that it sets the LR register correctly. >> + >> + Hence we create one testcase that jumps to `pass` using BLR, and one >> + testcase that jumps to `pass` if BLR has set the LR correctly. >> + >> + -- can_relocate_blr_jumps >> + ... Set x0 to pass >> + set_point13: >> + BLR x0 ; jump to pass (tracepoint here). >> + >> + -- can_relocate_blr_sets_lr >> + ... Set x0 to foo >> + set_point14: >> + BLR x0 ; jumps somewhere else (tracepoint here). >> + BL pass ; ensures the LR was set correctly by the BLR. >> + >> + */ >> + >> +static void >> +can_relocate_blr_jumps (void) >> +{ >> + int ok = 0; >> + >> + /* Test BLR indeed jumps to the target. */ >> + asm (" movz x0, :abs_g3:pass\n" >> + " movk x0, :abs_g2_nc:pass\n" >> + " movk x0, :abs_g1_nc:pass\n" >> + " movk x0, :abs_g0_nc:pass\n" >> + "set_point13:\n" >> + " blr x0\n" >> + : : : "x0","x30"); >> +} >> + >> +static void >> +can_relocate_blr_sets_lr (void) >> +{ >> + int ok = 0; >> + >> + /* Test BLR sets the LR correctly. */ >> + asm (" movz x0, :abs_g3:foo\n" >> + " movk x0, :abs_g2_nc:foo\n" >> + " movk x0, :abs_g1_nc:foo\n" >> + " movk x0, :abs_g0_nc:foo\n" >> + "set_point14:\n" >> + " blr x0\n" >> + " bl pass\n" >> + : : : "x0","x30"); >> +} >> + >> #endif >> >> /* Functions testing relocations need to be placed here. GDB will read >> @@ -536,6 +629,9 @@ static testcase_ftype testcases[] = { >> can_relocate_ldr, >> can_relocate_bcond_false, >> can_relocate_bl, >> + can_relocate_br, >> + can_relocate_blr_jumps, >> + can_relocate_blr_sets_lr, >> #endif >> }; >> >> >