Re: [Patch] GDB: aarch64: Add ability to step over a BR/BLR instruction

[Matthew Malcomson via Gdb-patches <gdb-patches@sourceware.org>]

[-- Attachment #1: Type: text/plain, Size: 14067 bytes --]

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?

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 <stdio.h>
> 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  <matthew.malcomson@arm.com>
> 
> 	* 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  <matthew.malcomson@arm.com>
> 
> 	* 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
>   };
>   
> 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: blr-with-new-tests.patch --]
[-- Type: text/x-patch; name="blr-with-new-tests.patch", Size: 5067 bytes --]

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
 };