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

[Alan Hayward <Alan.Hayward@arm.com>]

> On 3 Jul 2020, at 16:36, Luis Machado <luis.machado@linaro.org> wrote:
> 
> 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.

+1. Was going to write a very similar comment. A test for all the cases would be great.

> 
>> ------#####
>> 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)

Maybe the 0xfffffc1f mask belongs in aarch64-insn.h

>> +  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,

Not for this patch - but I wonder if this enum could be deleted and instead use
code from ../opcodes/aarch64-tbl.h
Random hex values make me nervous, and opcodes/ is already used for the disassembler.

>>    BLR             = 0xd63f0000,
>>    /* RET            1101 0110 0101 1111 0000 00rr rrr0 0000 */
>>    RET             = 0xd65f0000,
> The patch looks good to me.