GDB: aarch64: Add ability to step over a BR/BLR instruction

Message ID AM6PR08MB3157BC89EE961487966C1443E06A0@AM6PR08MB3157.eurprd08.prod.outlook.com
State New
Headers show
Series
  • GDB: aarch64: Add ability to step over a BR/BLR instruction
Related show

Commit Message

Matthew Malcomson July 3, 2020, 2:55 p.m.
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.


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


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

Comments

Mike Frysinger via Gdb-patches July 3, 2020, 3:36 p.m. | #1
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.
Alan Hayward July 3, 2020, 4:06 p.m. | #2
> 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.
Matthew Malcomson July 20, 2020, 11:13 a.m. | #3
> 

> 

>> On 3 Jul 2020, at 16:36, Luis Machado <luis.machado@linaro.org> wrote:

> >

>> On 7/3/20 11:55 AM, Matthew Malcomson wrote:

> >> 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.

> 


I've added tests for the BR and BLR instructions rather than for all
instructions due to time constraints ... I hope that's OK?

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


Done.




I'm not 100% confident on the approach of the testcase.
I tried to make it robust and clear, but may have missed a better approach.
(I used an assembly file to have easy control over the exact sequence of
instructions, and I think this is the nicest approach even though there are
only a few assembly files in the testsuite).


###############     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/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
new file mode 100644
index 0000000000000000000000000000000000000000..54eae61358c084d9342318591b7dbc57aa265ee4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
@@ -0,0 +1,65 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test displaced stepping over BR and BLR instructions.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile ".s"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+gdb_breakpoint "*blr_teststart"
+gdb_breakpoint "*blr_testcheck"
+gdb_breakpoint "*br_teststart"
+gdb_breakpoint "*br_testcheck"
+
+
+# Test for displaced stepping over the BLR instruction.
+gdb_test "run" \
+  "Starting program.*Breakpoint $decimal.*" \
+  "Run until BLR test start"
+
+set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BLR test."
+
+gdb_continue_to_breakpoint "BLR test check"
+
+gdb_test "print/x \$lr == $expected_lr" \
+  ".. = 0x1" \
+  "Ensure LR is set to just after BLR."
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BLR test."
+
+
+# Test for displaced stepping over the BR instruction.
+gdb_continue_to_breakpoint "BR test start"
+
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BR test."
+gdb_continue_to_breakpoint "BR test check"
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BR test."
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000000000000000000000000000000000000..fb67333271e649388a613fb9558f39ff30297697
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,92 @@
+// Instructions not yet tested.
+// -  B
+// -  BL
+// -  B.COND
+// -  CBZ
+// -  CBNZ
+// -  TBZ
+// -  TBNZ
+// -  ADR
+// -  ADRP
+// -  LDR (literal)
+// -  RET
+
+// Function testing stepping over BLR instruction.
+	.text
+	.align	2
+	.global	test_blr_stepping
+	.type	test_blr_stepping, %function
+test_blr_stepping:
+	.cfi_startproc
+	// x2 Stores the old LR.
+	mov	x2,x30
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS
+	movk	x1, :abs_g2_nc:.LJUMPPOS
+	movk	x1, :abs_g1_nc:.LJUMPPOS
+	movk	x1, :abs_g0_nc:.LJUMPPOS
+blr_teststart:
+	blr	x1
+	b	blr_testcheck
+.LJUMPPOS:
+	mov	x0, #1
+blr_testcheck:
+	// Put the old LR value back into the LR register.
+	// Do this for both successful jump and unsuccessful jump since the LR
+	// will have changed both times and we want the program to continue
+	// properly both times.
+	mov	x30, x2
+	ret
+	.cfi_endproc
+	.size	test_blr_stepping, .-test_blr_stepping
+
+
+// Function testing stepping over BR instruction.
+	.text
+	.align	2
+	.global	test_br_stepping
+	.type	test_br_stepping, %function
+test_br_stepping:
+	.cfi_startproc
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS2
+	movk	x1, :abs_g2_nc:.LJUMPPOS2
+	movk	x1, :abs_g1_nc:.LJUMPPOS2
+	movk	x1, :abs_g0_nc:.LJUMPPOS2
+br_teststart:
+	br	x1
+	b	br_testcheck
+.LJUMPPOS2:
+	mov	x0, #1
+br_testcheck:
+	ret
+	.cfi_endproc
+	.size	test_br_stepping, .-test_br_stepping
+
+
+
+// Main function calling all test functions above.
+	.text
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	.cfi_startproc
+	stp	x29, x30, [sp, -16]!
+	.cfi_def_cfa_offset 16
+	.cfi_offset 29, -16
+	.cfi_offset 30, -8
+	bl	test_blr_stepping
+	bl	test_br_stepping
+	ldp	x29, x30, [sp], 16
+	.cfi_restore 30
+	.cfi_restore 29
+	.cfi_def_cfa_offset 0
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits
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/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
new file mode 100644
index 0000000000000000000000000000000000000000..54eae61358c084d9342318591b7dbc57aa265ee4
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
@@ -0,0 +1,65 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test displaced stepping over BR and BLR instructions.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile ".s"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+gdb_breakpoint "*blr_teststart"
+gdb_breakpoint "*blr_testcheck"
+gdb_breakpoint "*br_teststart"
+gdb_breakpoint "*br_testcheck"
+
+
+# Test for displaced stepping over the BLR instruction.
+gdb_test "run" \
+  "Starting program.*Breakpoint $decimal.*" \
+  "Run until BLR test start"
+
+set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BLR test."
+
+gdb_continue_to_breakpoint "BLR test check"
+
+gdb_test "print/x \$lr == $expected_lr" \
+  ".. = 0x1" \
+  "Ensure LR is set to just after BLR."
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BLR test."
+
+
+# Test for displaced stepping over the BR instruction.
+gdb_continue_to_breakpoint "BR test start"
+
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BR test."
+gdb_continue_to_breakpoint "BR test check"
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BR test."
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000000000000000000000000000000000000..fb67333271e649388a613fb9558f39ff30297697
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,92 @@
+// Instructions not yet tested.
+// -  B
+// -  BL
+// -  B.COND
+// -  CBZ
+// -  CBNZ
+// -  TBZ
+// -  TBNZ
+// -  ADR
+// -  ADRP
+// -  LDR (literal)
+// -  RET
+
+// Function testing stepping over BLR instruction.
+	.text
+	.align	2
+	.global	test_blr_stepping
+	.type	test_blr_stepping, %function
+test_blr_stepping:
+	.cfi_startproc
+	// x2 Stores the old LR.
+	mov	x2,x30
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS
+	movk	x1, :abs_g2_nc:.LJUMPPOS
+	movk	x1, :abs_g1_nc:.LJUMPPOS
+	movk	x1, :abs_g0_nc:.LJUMPPOS
+blr_teststart:
+	blr	x1
+	b	blr_testcheck
+.LJUMPPOS:
+	mov	x0, #1
+blr_testcheck:
+	// Put the old LR value back into the LR register.
+	// Do this for both successful jump and unsuccessful jump since the LR
+	// will have changed both times and we want the program to continue
+	// properly both times.
+	mov	x30, x2
+	ret
+	.cfi_endproc
+	.size	test_blr_stepping, .-test_blr_stepping
+
+
+// Function testing stepping over BR instruction.
+	.text
+	.align	2
+	.global	test_br_stepping
+	.type	test_br_stepping, %function
+test_br_stepping:
+	.cfi_startproc
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS2
+	movk	x1, :abs_g2_nc:.LJUMPPOS2
+	movk	x1, :abs_g1_nc:.LJUMPPOS2
+	movk	x1, :abs_g0_nc:.LJUMPPOS2
+br_teststart:
+	br	x1
+	b	br_testcheck
+.LJUMPPOS2:
+	mov	x0, #1
+br_testcheck:
+	ret
+	.cfi_endproc
+	.size	test_br_stepping, .-test_br_stepping
+
+
+
+// Main function calling all test functions above.
+	.text
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	.cfi_startproc
+	stp	x29, x30, [sp, -16]!
+	.cfi_def_cfa_offset 16
+	.cfi_offset 29, -16
+	.cfi_offset 30, -8
+	bl	test_blr_stepping
+	bl	test_br_stepping
+	ldp	x29, x30, [sp], 16
+	.cfi_restore 30
+	.cfi_restore 29
+	.cfi_def_cfa_offset 0
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits
Alan Hayward July 23, 2020, 4:13 p.m. | #4
> On 20 Jul 2020, at 12:13, Matthew Malcomson <Matthew.Malcomson@arm.com> wrote:

> 

>> 

>> 

>>> On 3 Jul 2020, at 16:36, Luis Machado <luis.machado@linaro.org> wrote:

>>> 

>>> On 7/3/20 11:55 AM, Matthew Malcomson wrote:

>>>> 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.

>> 

> 

> I've added tests for the BR and BLR instructions rather than for all

> instructions due to time constraints ... I hope that's OK?


Sure, that’s fine.

> 

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

> 

> Done.

> 

> 

> 

> 

> I'm not 100% confident on the approach of the testcase.

> I tried to make it robust and clear, but may have missed a better approach.

> (I used an assembly file to have easy control over the exact sequence of

> instructions, and I think this is the nicest approach even though there are

> only a few assembly files in the testsuite).


Your approach ensures that the functions have the correct instructions, so I
think that’s ok. Alternatively asm in a C file might have worked too.


Two minor, but important, nits to fix:

> 

> 

> ###############     Attachment also inlined for ease of reply    ###############

> 


Missing 2x Changelogs.

> 

> 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/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp

> new file mode 100644

> index 0000000000000000000000000000000000000000..54eae61358c084d9342318591b7dbc57aa265ee4

> --- /dev/null

> +++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp

> @@ -0,0 +1,65 @@

> +# Copyright 2020 Free Software Foundation, Inc.

> +#

> +# This program is free software; you can redistribute it and/or modify

> +# it under the terms of the GNU General Public License as published by

> +# the Free Software Foundation; either version 3 of the License, or

> +# (at your option) any later version.

> +#

> +# This program is distributed in the hope that it will be useful,

> +# but WITHOUT ANY WARRANTY; without even the implied warranty of

> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the

> +# GNU General Public License for more details.

> +#

> +# You should have received a copy of the GNU General Public License

> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.

> +#

> +# This file is part of the gdb testsuite.

> +

> +# Test displaced stepping over BR and BLR instructions.

> +

> +if {![is_aarch64_target]} {

> +    verbose "Skipping ${gdb_test_file_name}."

> +    return

> +}

> +

> +standard_testfile ".s"

> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {

> +    return -1

> +}

> +

> +gdb_breakpoint "*blr_teststart"

> +gdb_breakpoint "*blr_testcheck"

> +gdb_breakpoint "*br_teststart"

> +gdb_breakpoint "*br_testcheck"

> +

> +

> +# Test for displaced stepping over the BLR instruction.

> +gdb_test "run" \

> +  "Starting program.*Breakpoint $decimal.*" \

> +  "Run until BLR test start"

> +

> +set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]

> +gdb_test "print/x \$x0" \

> +  ".. = 0x0" \

> +  "Ensure x0 is 0 before BLR test."

> +

> +gdb_continue_to_breakpoint "BLR test check"

> +

> +gdb_test "print/x \$lr == $expected_lr" \

> +  ".. = 0x1" \

> +  "Ensure LR is set to just after BLR."

> +gdb_test "print/x \$x0" \

> +  ".. = 0x1" \

> +  "Ensure x0 is 1 after BLR test."

> +

> +

> +# Test for displaced stepping over the BR instruction.

> +gdb_continue_to_breakpoint "BR test start"

> +

> +gdb_test "print/x \$x0" \

> +  ".. = 0x0" \

> +  "Ensure x0 is 0 before BR test."

> +gdb_continue_to_breakpoint "BR test check"

> +gdb_test "print/x \$x0" \

> +  ".. = 0x1" \

> +  "Ensure x0 is 1 after BR test."

> diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s

> new file mode 100644

> index 0000000000000000000000000000000000000000..fb67333271e649388a613fb9558f39ff30297697

> --- /dev/null

> +++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s


Missing copyright file header.


> @@ -0,0 +1,92 @@

> +// Instructions not yet tested.

> +// -  B

> +// -  BL

> +// -  B.COND

> +// -  CBZ

> +// -  CBNZ

> +// -  TBZ

> +// -  TBNZ

> +// -  ADR

> +// -  ADRP

> +// -  LDR (literal)

> +// -  RET

> +

> +// Function testing stepping over BLR instruction.

> +	.text

> +	.align	2

> +	.global	test_blr_stepping

> +	.type	test_blr_stepping, %function

> +test_blr_stepping:

> +	.cfi_startproc

> +	// x2 Stores the old LR.

> +	mov	x2,x30

> +	// x0 is the indicator value to show whether the jump happened.

> +	mov	x0, #0

> +	// Load the jump position into register x1

> +	movz	x1, :abs_g3:.LJUMPPOS

> +	movk	x1, :abs_g2_nc:.LJUMPPOS

> +	movk	x1, :abs_g1_nc:.LJUMPPOS

> +	movk	x1, :abs_g0_nc:.LJUMPPOS

> +blr_teststart:

> +	blr	x1

> +	b	blr_testcheck

> +.LJUMPPOS:

> +	mov	x0, #1

> +blr_testcheck:

> +	// Put the old LR value back into the LR register.

> +	// Do this for both successful jump and unsuccessful jump since the LR

> +	// will have changed both times and we want the program to continue

> +	// properly both times.

> +	mov	x30, x2

> +	ret

> +	.cfi_endproc

> +	.size	test_blr_stepping, .-test_blr_stepping

> +

> +

> +// Function testing stepping over BR instruction.

> +	.text

> +	.align	2

> +	.global	test_br_stepping

> +	.type	test_br_stepping, %function

> +test_br_stepping:

> +	.cfi_startproc

> +	// x0 is the indicator value to show whether the jump happened.

> +	mov	x0, #0

> +	// Load the jump position into register x1

> +	movz	x1, :abs_g3:.LJUMPPOS2

> +	movk	x1, :abs_g2_nc:.LJUMPPOS2

> +	movk	x1, :abs_g1_nc:.LJUMPPOS2

> +	movk	x1, :abs_g0_nc:.LJUMPPOS2

> +br_teststart:

> +	br	x1

> +	b	br_testcheck

> +.LJUMPPOS2:

> +	mov	x0, #1

> +br_testcheck:

> +	ret

> +	.cfi_endproc

> +	.size	test_br_stepping, .-test_br_stepping

> +

> +

> +

> +// Main function calling all test functions above.

> +	.text

> +	.align	2

> +	.global	main

> +	.type	main, %function

> +main:

> +	.cfi_startproc

> +	stp	x29, x30, [sp, -16]!

> +	.cfi_def_cfa_offset 16

> +	.cfi_offset 29, -16

> +	.cfi_offset 30, -8

> +	bl	test_blr_stepping

> +	bl	test_br_stepping

> +	ldp	x29, x30, [sp], 16

> +	.cfi_restore 30

> +	.cfi_restore 29

> +	.cfi_def_cfa_offset 0

> +	ret

> +	.cfi_endproc

> +	.size	main, .-main

> +	.section	.note.GNU-stack,"",@progbits

> 

> <blr-patch.patch>
Matthew Malcomson July 23, 2020, 4:48 p.m. | #5
> 

> Two minor, but important, nits to fix:

> 

> Missing 2x Changelogs.

> 

...
> 

> Missing copyright file header.

> 

> 



Apologies, the ChangeLog is below, and I've added the copyright header that has
gone in aarch64-disp-stepping.s inline so you can check it easily.

The patch (created from `git format-patch`) is attached.


I don't have commit rights to GDB, so if this is OK could you apply the patch?


Thanks,
Matthew


ChangeLogs to apply:
------

gdb/ChangeLog:
2020-07-23  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-07-23  Matthew Malcomson  <matthew.malcomson@arm.com>

	* gdb.arch/aarch64-disp-stepping.exp: New test runner.
	* gdb.arch/aarch64-disp-stepping.s: New test.


-------
Additional Copyright header added to aarch64-disp-stepping.s:


diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000000000000000000000000000000000000..c7a67a12f250abee4250a1edfca411f25bc3a113
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,111 @@
+/* Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.
+
+   Test displaced stepping over AArch64 instructions.  */
+
From 872cec9f53482429fbd547e13ac552d472bd002b Mon Sep 17 00:00:00 2001
From: Matthew Malcomson <matthew.malcomson@arm.com>
Date: Sun, 28 Jun 2020 13:06:36 +0100
Subject: [PATCH] [Patch] GDB: aarch64: Add ability to displaced step over a
 BR/BLR instruction

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.

Manually tested on AArch64 and ensured new testcase passes.
Regression tests showed nothing untoward.

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

	* gdb.arch/aarch64-disp-stepping.exp: New test runner.
	* gdb.arch/aarch64-disp-stepping.s: New test.
---
 gdb/aarch64-tdep.c                               |  19 ++--
 gdb/arch/aarch64-insn.h                          |  10 ++
 gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp |  65 +++++++++++++
 gdb/testsuite/gdb.arch/aarch64-disp-stepping.s   | 111 +++++++++++++++++++++++
 4 files changed, 199 insertions(+), 6 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
 create mode 100644 gdb/testsuite/gdb.arch/aarch64-disp-stepping.s

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 5e7d0d0..d247108 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 6a63ce9..f261363 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/aarch64-disp-stepping.exp b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
new file mode 100644
index 0000000..54eae61
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.exp
@@ -0,0 +1,65 @@
+# Copyright 2020 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# This file is part of the gdb testsuite.
+
+# Test displaced stepping over BR and BLR instructions.
+
+if {![is_aarch64_target]} {
+    verbose "Skipping ${gdb_test_file_name}."
+    return
+}
+
+standard_testfile ".s"
+if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
+    return -1
+}
+
+gdb_breakpoint "*blr_teststart"
+gdb_breakpoint "*blr_testcheck"
+gdb_breakpoint "*br_teststart"
+gdb_breakpoint "*br_testcheck"
+
+
+# Test for displaced stepping over the BLR instruction.
+gdb_test "run" \
+  "Starting program.*Breakpoint $decimal.*" \
+  "Run until BLR test start"
+
+set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BLR test."
+
+gdb_continue_to_breakpoint "BLR test check"
+
+gdb_test "print/x \$lr == $expected_lr" \
+  ".. = 0x1" \
+  "Ensure LR is set to just after BLR."
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BLR test."
+
+
+# Test for displaced stepping over the BR instruction.
+gdb_continue_to_breakpoint "BR test start"
+
+gdb_test "print/x \$x0" \
+  ".. = 0x0" \
+  "Ensure x0 is 0 before BR test."
+gdb_continue_to_breakpoint "BR test check"
+gdb_test "print/x \$x0" \
+  ".. = 0x1" \
+  "Ensure x0 is 1 after BR test."
diff --git a/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
new file mode 100644
index 0000000..c7a67a1
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/aarch64-disp-stepping.s
@@ -0,0 +1,111 @@
+/* Copyright 2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+   This file is part of the gdb testsuite.
+
+   Test displaced stepping over AArch64 instructions.  */
+
+// Instructions not yet tested.
+// -  B
+// -  BL
+// -  B.COND
+// -  CBZ
+// -  CBNZ
+// -  TBZ
+// -  TBNZ
+// -  ADR
+// -  ADRP
+// -  LDR (literal)
+// -  RET
+
+// Function testing stepping over BLR instruction.
+	.text
+	.align	2
+	.global	test_blr_stepping
+	.type	test_blr_stepping, %function
+test_blr_stepping:
+	.cfi_startproc
+	// x2 Stores the old LR.
+	mov	x2,x30
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS
+	movk	x1, :abs_g2_nc:.LJUMPPOS
+	movk	x1, :abs_g1_nc:.LJUMPPOS
+	movk	x1, :abs_g0_nc:.LJUMPPOS
+blr_teststart:
+	blr	x1
+	b	blr_testcheck
+.LJUMPPOS:
+	mov	x0, #1
+blr_testcheck:
+	// Put the old LR value back into the LR register.
+	// Do this for both successful jump and unsuccessful jump since the LR
+	// will have changed both times and we want the program to continue
+	// properly both times.
+	mov	x30, x2
+	ret
+	.cfi_endproc
+	.size	test_blr_stepping, .-test_blr_stepping
+
+
+// Function testing stepping over BR instruction.
+	.text
+	.align	2
+	.global	test_br_stepping
+	.type	test_br_stepping, %function
+test_br_stepping:
+	.cfi_startproc
+	// x0 is the indicator value to show whether the jump happened.
+	mov	x0, #0
+	// Load the jump position into register x1
+	movz	x1, :abs_g3:.LJUMPPOS2
+	movk	x1, :abs_g2_nc:.LJUMPPOS2
+	movk	x1, :abs_g1_nc:.LJUMPPOS2
+	movk	x1, :abs_g0_nc:.LJUMPPOS2
+br_teststart:
+	br	x1
+	b	br_testcheck
+.LJUMPPOS2:
+	mov	x0, #1
+br_testcheck:
+	ret
+	.cfi_endproc
+	.size	test_br_stepping, .-test_br_stepping
+
+
+
+// Main function calling all test functions above.
+	.text
+	.align	2
+	.global	main
+	.type	main, %function
+main:
+	.cfi_startproc
+	stp	x29, x30, [sp, -16]!
+	.cfi_def_cfa_offset 16
+	.cfi_offset 29, -16
+	.cfi_offset 30, -8
+	bl	test_blr_stepping
+	bl	test_br_stepping
+	ldp	x29, x30, [sp], 16
+	.cfi_restore 30
+	.cfi_restore 29
+	.cfi_def_cfa_offset 0
+	ret
+	.cfi_endproc
+	.size	main, .-main
+	.section	.note.GNU-stack,"",@progbits
Pedro Alves July 23, 2020, 6:58 p.m. | #6
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"

> +set expected_lr [get_hexadecimal_valueof "\$pc + 4" 0]

> +gdb_test "print/x \$x0" \

> +  ".. = 0x0" \

> +  "Ensure x0 is 0 before BLR test."


The ".." at the start of the pattern are not necessary,
gdb_test will match anything that appears before your
pattern anyway.

Use lowercase, and no period at end.  Throughout.

"Ensure" is redundant, IMHO.  Every test is ensuring something.

So, I'd write:

 gdb_test "print/x \$x0" \
   " = 0x0" \
   "x0 is 0 before BLR test"

> +gdb_test "print/x \$lr == $expected_lr" \

> +  ".. = 0x1" \

> +  "Ensure LR is set to just after BLR."


Is set to ... ?

Please cat the testsuite/gdb.sum file after running
your testcase in isolation and skim it to make sure
it all makes sense.

You can also use 

 with_test_prefix "BLR" {
  ...
 }

 with_test_prefix "BR" {
  ...
 }

to group tests.

Thanks,
Pedro Alves
Matthew Malcomson Aug. 20, 2020, 12:41 p.m. | #7
> 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
 };
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
 };
Mike Frysinger via Gdb-patches Jan. 25, 2021, 6:31 p.m. | #8
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
>   };
>   
>
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
 };
Mike Frysinger via Gdb-patches Jan. 25, 2021, 6:44 p.m. | #9
Hi Matthew,

On 1/25/21 3:31 PM, Matthew Malcomson wrote:
> 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?


Sorry. That must have flown under the radar.

The attached patch does not contain the new test that was submitted 
before. Was that an oversight?

I think the patch looks good. I just want to make sure I know what 
pieces to apply.

Luis

> 

> 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

>>    };

>>    

>>

>
Mike Frysinger via Gdb-patches Jan. 26, 2021, 11:13 a.m. | #10
On 25/01/2021 18:44, Luis Machado wrote:
> Hi Matthew,

> 

> On 1/25/21 3:31 PM, Matthew Malcomson wrote:

>> 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?

> 

> Sorry. That must have flown under the radar.

> 

> The attached patch does not contain the new test that was submitted 

> before. Was that an oversight?

> 

> I think the patch looks good. I just want to make sure I know what 

> pieces to apply.

> 

> Luis


Hi Luis,

Thanks for looking at it.  The lack of the original test is on purpose 
-- when I was following requests from reviews I noticed that there was 
already a testcase for displaced stepping that I had missed originally.

The attached patch should contain a change to `gdb.arch/insn-reloc.c` 
which is where I've added tests for the new functionality (rather than 
as a separate test as before).

Thanks,
Matthew

> 

>>

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

>>> �� };

>>>

>>
Mike Frysinger via Gdb-patches Jan. 26, 2021, 11:46 a.m. | #11
On 1/26/21 8:13 AM, Matthew Malcomson wrote:
> On 25/01/2021 18:44, Luis Machado wrote:

>> Hi Matthew,

>>

>> On 1/25/21 3:31 PM, Matthew Malcomson wrote:

>>> 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?

>>

>> Sorry. That must have flown under the radar.

>>

>> The attached patch does not contain the new test that was submitted 

>> before. Was that an oversight?

>>

>> I think the patch looks good. I just want to make sure I know what 

>> pieces to apply.

>>

>> Luis

> 

> Hi Luis,

> 

> Thanks for looking at it.  The lack of the original test is on purpose 

> -- when I was following requests from reviews I noticed that there was 

> already a testcase for displaced stepping that I had missed originally.

> 

> The attached patch should contain a change to `gdb.arch/insn-reloc.c` 

> which is where I've added tests for the new functionality (rather than 

> as a separate test as before).


Thanks for the explanation. I gave this a try on Ubuntu 20.04 and I ran 
into the following errors. It seems the current test's assumptions are 
not valid for Ubuntu's toolchain configurations.

Could you please check if it can be fixed so it runs OK there as well?

Running 
/gdb/testsuite/../../../../repos/binutils-gdb/gdb/testsuite/gdb.arch/disp-step-insn-reloc.exp 
...
gdb compile failed, /usr/bin/ld: 
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o: 
relocation R_AARCH64_MOVW_UABS_G3 against `a local symbol' can not be 
used when making a shared object; recompile with -fPIC
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:/gdb/testsuite/gdb.arch/insn-reloc.c:615:(.data.rel.local+0x0): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x8): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x10): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x18): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x20): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x28): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x30): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x38): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x40): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x48): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x50): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x58): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x60): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x68): 
dangerous relocation: unsupported relocation
/gdb/testsuite/outputs/gdb.arch/disp-step-insn-reloc/disp-step-insn-reloc0.o:(.data.rel.local+0x70): 
dangerous relocation: unsupported relocation
collect2: error: ld returned 1 exit status

> 

> Thanks,

> Matthew

> 

>>

>>>

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

>>>> �� };

>>>>

>>>

>

Patch

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,