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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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