Re: [PATCH 14/24] gdb: read pseudo register through frame

John Baldwin
2023-11-11T20:11:46Z

On 11/7/23 9:00 PM, Simon Marchi wrote:
> From: Simon Marchi <simon.marchi@efficios.com>
>
> Change gdbarch_pseudo_register_read_value to take a frame instead of a
> regcache. The frame (and formerly the regcache) is used to read raw
> registers needed to make up the pseudo register value. The problem with
> using the regcache is that it always provides raw register values for
> the current frame (frame 0).
>
> Let's say the user wants to read the ebx register on amd64. ebx is a pseudo
> register, obtained by reading the bottom half (bottom 4 bytes) of the
> rbx register, which is a raw register. If the currently selected frame
> is frame 0, it works fine:
>
> (gdb) frame 0
> #0 break_here_asm () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S:36
> 36 in /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S
> (gdb) p/x $ebx
> $1 = 0x24252627
> (gdb) p/x $rbx
> $2 = 0x2021222324252627
>
> But if the user is looking at another frame, and the raw register behind
> the pseudo register has been saved at some point in the call stack, then
> we get a wrong answer:
>
> (gdb) frame 1
> #1 0x000055555555517d in caller () at /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S:56
> 56 in /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.arch/amd64-pseudo-unwind-asm.S
> (gdb) p/x $ebx
> $3 = 0x24252627
> (gdb) p/x $rbx
> $4 = 0x1011121314151617
>
> Here, the value of ebx was computed using the value of rbx in frame 0
> (through the regcache), it should have been computed using the value of
> rbx in frame 1.
>
> In other to make this work properly, make the following changes:
>
> - Make dwarf2_frame_prev_register return nullptr if it doesn't know how
> to unwind a register and that register is a pseudo register.
> Previously, it returned `frame_unwind_got_register`, meaning, in our
> example, "the value of ebx in frame 1 is the same as the value of ebx
> in frame 0", which is obviously false. Return nullptr as a way to
> say "I don't know".
>
> - In frame_unwind_register_value, when prev_register (for instance
> dwarf2_frame_prev_register) returns nullptr, and we are trying to
> read a pseudo register, try to get the register value through
> gdbarch_pseudo_register_read_value or gdbarch_pseudo_register_read.
> If using gdbarch_pseudo_register_read, the behavior is known to be
> broken. Implementations should be migrated to use
> gdbarch_pseudo_register_read_value to fix that.
>
> - Change gdbarch_pseudo_register_read_value to take a frame_info
> instead of a regcache, update implementations (aarch64, amd64, i386).
> In i386-tdep.c, I made a copy of i386_mmx_regnum_to_fp_regnum that
> uses a frame instead of a regcache. The version using the regcache
> is still used by i386_pseudo_register_write. It will get removed in
> a subsequent patch.
>
> - Add some helpers in value.{c,h} to implement the common cases of
> pseudo registers: taking part of a raw register and concatenating
> multiple raw registers.

These are quite a nice change and reduce a lot of the copy/paste that would
otherwise be present.

> - Update readable_regcache::{cooked_read,cooked_read_value} to pass the
> current frame to gdbarch_pseudo_register_read_value. Passing the
> current frame will give the same behavior as before: for frame 0, raw
> registers will be read from the current thread's regcache.
>
> Notes:
>
> - I do not plan on changing gdbarch_pseudo_register_read to receive a
> frame instead of a regcache. That method is considered deprecated.
> Instead, we should be working on migrating implementations to use
> gdbarch_pseudo_register_read_value instead.
>
> - In frame_unwind_register_value, we still ask the unwinder to try to
> unwind pseudo register values. It's apparently possible for the
> debug info to provide information about [1] pseudo registers, so we
> want to try that first, before falling back to computing them
> ourselves.

Only updating read_value to use a frame seems sensible to me as a plan.

I had to stare at the update to handle reading the BND pseudo registers
for i386 for a while, but it does look correct to me.

Reviewed-by: John Baldwin <jhb@FreeBSD.org>

--
John Baldwin