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

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

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