From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id F6LWEK6cEWDBSwAAWB0awg (envelope-from ) for ; Wed, 27 Jan 2021 12:02:38 -0500 Received: by simark.ca (Postfix, from userid 112) id 355941EF80; Wed, 27 Jan 2021 12:02:38 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on simark.ca X-Spam-Level: X-Spam-Status: No, score=0.2 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_NONE,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.2 Received: from sourceware.org (unknown [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 97CAB1E945 for ; Wed, 27 Jan 2021 12:02:37 -0500 (EST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DF7C0386F020; Wed, 27 Jan 2021 17:02:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DF7C0386F020 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1611766956; bh=C8LfiMeHE52wuie4fl+NwQbF0eLKMCKKeRXkS1k1b5g=; 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=iazEbrSV+EnJyBeRCPG5MhCzD3kV1cfUCneqPObccyt/ujMHSfbhzHGoHSbztrazr nRTEW7s2vVCBhBdYfPyO/weqQBGfDiVqqoxQM/i85RGoWjPrGmnHVE6rryuWaEHykO MkgdnTvF0owdPXkUBC5bM+ItD6MHBxTsObvD3P84= Received: from mail-qk1-x734.google.com (mail-qk1-x734.google.com [IPv6:2607:f8b0:4864:20::734]) by sourceware.org (Postfix) with ESMTPS id 08B0A386F020 for ; Wed, 27 Jan 2021 17:02:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 08B0A386F020 Received: by mail-qk1-x734.google.com with SMTP id v126so2365290qkd.11 for ; Wed, 27 Jan 2021 09:02:34 -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=C8LfiMeHE52wuie4fl+NwQbF0eLKMCKKeRXkS1k1b5g=; b=HJPt3JEGhEMoyQ9DH8qktolPSKolV6gdPrZB8i5G1yRT97p+/4x4u4QACQnUYE3Mb5 S8mc6S1psjP0uNR4/1zXSANXF31aqzDwgnBu/FEA8czAUCze6QvnZLnXQ5k2f/8yEbpK qhnVo5XEuOAc7MGPzru6BSp0SvnG6MeDusNCthxrPuLBZSa4an2Xesgb0vyS7HeoeS0F Yg/y1wkqmPljG1clBeghljpxytWJUuTpZBMepSNm9mcQuKsX9IWoUu0wSUmgO3ltl6AX dqNNyaJwDX4HQ6AGpOAnp+qY6r9oka2D2VOEIW6ubIp4Tq1OvZFDSX4UQOAO/dp2RY0N BToA== X-Gm-Message-State: AOAM532Y6uJ/0aINCE1WPOd3aM2PZgOWJLKZTYBKlQmLjvbUQ20XOOvD oytlqiiCZqOeQsN++UEUMLA2VQUNXxF7Gw== X-Google-Smtp-Source: ABdhPJzD/V0VFeTXWYRfPp8DPQzfWkPV+3I6UxTKS85qFyjcOMGaPiUaHaJVgDlyligqPP5qtVCRnQ== X-Received: by 2002:a37:788:: with SMTP id 130mr11611476qkh.390.1611766951975; Wed, 27 Jan 2021 09:02:31 -0800 (PST) Received: from ?IPv6:2804:7f0:8284:874d:b82c:87fc:4324:adab? ([2804:7f0:8284:874d:b82c:87fc:4324:adab]) by smtp.gmail.com with ESMTPSA id s5sm1566106qke.71.2021.01.27.09.02.29 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 27 Jan 2021 09:02:31 -0800 (PST) Subject: Re: [Patch] GDB: aarch64: Add ability to displaced 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> <9efc84c3-d4fb-03d6-8612-600cc2f74e65@linaro.org> <4e5f7899-6a44-c18b-45ae-31228a6f5c20@arm.com> Message-ID: Date: Wed, 27 Jan 2021 14:02:27 -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/27/21 1:42 PM, Matthew Malcomson wrote: > Hi Luis, > > Thanks for the catch on those linker errors. > I managed to reproduce those errors using `-pie', and that seems to explain > the error messages quite well. > > I've used the testcase in the updated patch attached and it seems to work for > RUNTESTFLAGS="--target_board=native-gdbserver", > RUNTESTFLAGS="--target_board=unix/-pie", and without a provided RUNTESTFLAGS on > Ubuntu 16.04 (where no provided RUNTESTFLAGS compiles without PIE). > > Hopefully that testcase will be a bit more robust. Thanks. This fixes the errors I saw. Just one small formatting nit. When you declare enum aarch64_masks, you put a couple newlines after it. So remove one of those. I see you've pushed things to binutils/gas before. I suppose you can push changes to GDB as well, given it is the same repository? If not, please let me know and I can push it. > > Thanks, > Matthew. > > > ------ PROPOSED COMMIT MESSAGE > > > 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 (though it > took a while for me to get the testcase to account for PIE). > > ------##### > 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 > ~ [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) runenum aarch64_masks > 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 d1e15497a46ca250d606f4da77ad653fecc41e1c..3ac0564dd9a1bf01a7df60059b3a7b353e3b042f 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -3099,15 +3099,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 1e8c5eac940ee017f4b6d2685bbcbaeed0617e3a..87e2f5875a133a347c4be48b255a04282647be12 100644 > --- a/gdb/arch/aarch64-insn.h > +++ b/gdb/arch/aarch64-insn.h > @@ -61,7 +61,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, > @@ -128,6 +130,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 bfbb3161b3aa3dcf8a5ff00e4341c4d68e473f26..62f13a9275439c8e2abdedd5cfbd88d06d863d03 100644 > --- a/gdb/testsuite/gdb.arch/insn-reloc.c > +++ b/gdb/testsuite/gdb.arch/insn-reloc.c > @@ -512,6 +512,84 @@ 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). > + fail() > + return > + target: > + pass() > + end > + > + */ > + > +static void > +can_relocate_br (void) > +{ > + int ok = 0; > + > + asm goto (" adr x0, %l0\n" > + "set_point12:\n" > + " br x0\n" > + : > + : > + : "x0" > + : madejump); > + > + fail (); > + return; > +madejump: > + pass (); > +} > + > +/* 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 ("set_point13:\n" > + " blr %[address]\n" > + : : [address] "r" (&pass) : "x30"); > +} > + > +static void > +can_relocate_blr_sets_lr (void) > +{ > + int ok = 0; > + > + /* Test BLR sets the LR correctly. */ > + asm ("set_point14:\n" > + " blr %[address]\n" > + " bl pass\n" > + : : [address] "r" (&foo) : "x30"); > +} > + > #endif > > /* Functions testing relocations need to be placed here. GDB will read > @@ -536,6 +614,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 > }; > >