[PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[Tom Tromey]

The internal AdaCore testsuite has a test that checks that an
out-of-scope watchpoint is deleted.  This fails on some aarch64
configurations, reporting an extra stop:

    (gdb) continue
    Continuing.

    Thread 3 hit Watchpoint 2: result

    Old value = 64
    New value = 0
    0x0000000040021648 in pck.get_val (seed=0, off_by_one=false) at [...]/pck.adb:13
    13	   end Get_Val;

I believe what is happening here is that the variable is stored at:

    <efa>   DW_AT_location    : 2 byte block: 91 7c 	(DW_OP_fbreg: -4)

and the extra stop is reported just before a return, when the ldp
instruction is executed:

   0x0000000040021644 <+204>:	ldp	x29, x30, [sp], #48
   0x0000000040021648 <+208>:	ret

This instruction modifies the frame base calculation, and so the test
picks up whatever memory is pointed to in the callee frame.

Implementing the gdbarch hook gdbarch_stack_frame_destroyed_p fixes
this problem.

As usual with this sort of patch, it has passed internal testing, but
I don't have a good way to try it with dejagnu.  So, I don't know
whether some existing test covers this.  I suspect there must be one,
but it's also worth noting that this test passes for aarch64 in some
configurations -- I don't know what causes one to fail and another to
succeed.
---
 gdb/aarch64-tdep.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b3efb3ebaff..5effa95d71b 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -3379,6 +3379,25 @@ aarch64_cannot_store_register (struct gdbarch *gdbarch, int regnum)
 	  || regnum == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base));
 }
 
+/* Implement the stack_frame_destroyed_p gdbarch method.  */
+
+static int
+aarch64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  CORE_ADDR func_start, func_end;
+  if (!find_pc_partial_function (pc, NULL, &func_start, &func_end))
+    return 0;
+
+  enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
+  uint32_t insn = read_memory_unsigned_integer (pc, 4, byte_order_for_code);
+
+  aarch64_inst inst;
+  if (aarch64_decode_insn (insn, &inst, 1, nullptr) != 0)
+    return 0;
+
+  return streq (inst.opcode->name, "ret");
+}
+
 /* Initialize the current architecture based on INFO.  If possible,
    re-use an architecture from ARCHES, which is a list of
    architectures already created during this debugging session.
@@ -3585,6 +3604,9 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
   set_gdbarch_long_double_format (gdbarch, floatformats_ia64_quad);
   set_gdbarch_type_align (gdbarch, aarch64_type_align);
 
+  /* Detect whether PC is at a point where the stack has been destroyed.  */
+  set_gdbarch_stack_frame_destroyed_p (gdbarch, aarch64_stack_frame_destroyed_p);
+
   /* Internal <-> external register number maps.  */
   set_gdbarch_dwarf2_reg_to_regnum (gdbarch, aarch64_dwarf_reg_to_regnum);
 
-- 
2.34.1

Re: [PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[Luis Machado]

Hi Tom,

Thanks for the patch.

On 3/11/22 16:31, Tom Tromey via Gdb-patches wrote:
> The internal AdaCore testsuite has a test that checks that an
> out-of-scope watchpoint is deleted.  This fails on some aarch64
> configurations, reporting an extra stop:
> 
>      (gdb) continue
>      Continuing.
> 
>      Thread 3 hit Watchpoint 2: result
> 
>      Old value = 64
>      New value = 0
>      0x0000000040021648 in pck.get_val (seed=0, off_by_one=false) at [...]/pck.adb:13
>      13	   end Get_Val;
> 
> I believe what is happening here is that the variable is stored at:
> 
>      <efa>   DW_AT_location    : 2 byte block: 91 7c 	(DW_OP_fbreg: -4)
> 
> and the extra stop is reported just before a return, when the ldp
> instruction is executed:
> 
>     0x0000000040021644 <+204>:	ldp	x29, x30, [sp], #48
>     0x0000000040021648 <+208>:	ret
> 
> This instruction modifies the frame base calculation, and so the test
> picks up whatever memory is pointed to in the callee frame.
> 
> Implementing the gdbarch hook gdbarch_stack_frame_destroyed_p fixes
> this problem.
> 
> As usual with this sort of patch, it has passed internal testing, but
> I don't have a good way to try it with dejagnu.  So, I don't know
> whether some existing test covers this.  I suspect there must be one,
> but it's also worth noting that this test passes for aarch64 in some
> configurations -- I don't know what causes one to fail and another to
> succeed.

Are the passing/failing runs using different compiler versions? If the 
variable no longer exists, then having a stale location like that seems 
wrong. Can you pinpoint what is different from a passing test and a 
failing one? GDB version, compiler version, different binary?

These hooks seem to take care of functions without debuginfo, so they 
tend to walk instruction by instruction to figure things out.

I don't have a problem with the patch itself, and I don't see 
regressions on aarch64-linux. But I'd like to understand if the compiler 
is possibly generating something that it shouldn't.

Re: [PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[Tom Tromey]

Luis> Are the passing/failing runs using different compiler versions?

Nope.

Luis> If the
Luis> variable no longer exists, then having a stale location like that
Luis> seems wrong. Can you pinpoint what is different from a passing test
Luis> and a failing one? GDB version, compiler version, different binary?

I didn't dig quite this deep but I think it's different memory contents
in the caller's stack frame.

Luis> These hooks seem to take care of functions without debuginfo, so they
Luis> tend to walk instruction by instruction to figure things out.

No, this is a case with full debuginfo.  What's happening is that the
DWARF for the callee describes the location of a variable as relative to
the stack frame:

>      <efa>   DW_AT_location    : 2 byte block: 91 7c 	(DW_OP_fbreg: -4)

However, at the end of the callee, there's an 'ldp' instruction -- which
resets the stack pointer.  So now, that variable's location description
points to an offset relative to the caller's frame.  However, this is
clearly incorrect, because the caller and the callee generally will not
agree on the relative location of anything in their frames.

Basically, this is a kind of epilogue detection.  It might be better if
the compiler told gdb about this (but it doesn't) or if variable
locations ended at the 'ldp' (but they don't).

Tom

Re: [PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[Luis Machado]

Hi,

On 3/14/22 13:24, Tom Tromey wrote:
> Luis> Are the passing/failing runs using different compiler versions?
> 
> Nope.
> 
> Luis> If the
> Luis> variable no longer exists, then having a stale location like that
> Luis> seems wrong. Can you pinpoint what is different from a passing test
> Luis> and a failing one? GDB version, compiler version, different binary?
> 
> I didn't dig quite this deep but I think it's different memory contents
> in the caller's stack frame.

The kind of which will change from run to run, not triggering the 
watchpoint in some cases?

> 
> Luis> These hooks seem to take care of functions without debuginfo, so they
> Luis> tend to walk instruction by instruction to figure things out.
> 
> No, this is a case with full debuginfo.  What's happening is that the
> DWARF for the callee describes the location of a variable as relative to
> the stack frame:
> 
>>       <efa>   DW_AT_location    : 2 byte block: 91 7c 	(DW_OP_fbreg: -4)
> 
> However, at the end of the callee, there's an 'ldp' instruction -- which
> resets the stack pointer.  So now, that variable's location description
> points to an offset relative to the caller's frame.  However, this is
> clearly incorrect, because the caller and the callee generally will not
> agree on the relative location of anything in their frames.
> 
> Basically, this is a kind of epilogue detection.  It might be better if
> the compiler told gdb about this (but it doesn't) or if variable
> locations ended at the 'ldp' (but they don't).

Yeah. Anyway, other backends rely on this hook as well. So the patch is 
fine to me. As I've said before, I didn't see any regressions.

Thanks,
Luis

Re: [PATCH] Implement gdbarch_stack_frame_destroyed_p for aarch64

[Tom Tromey]

>> I didn't dig quite this deep but I think it's different memory
>> contents
>> in the caller's stack frame.

Luis> The kind of which will change from run to run, not triggering the
Luis> watchpoint in some cases?

Yeah.  I didn't really look into that part too much.

Luis> Yeah. Anyway, other backends rely on this hook as well. So the patch
Luis> is fine to me. As I've said before, I didn't see any regressions.

Thank you, I'm going to check it in.

Tom