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

[Luis Machado <luis.machado@linaro.org>]

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 <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-07-03  Matthew Malcomson  <matthew.malcomson@arm.com>
> 
> 	* 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.