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

[John Baldwin <jhb@FreeBSD.org>]

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