gdb: read pseudo register through frame

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.

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

[1] https://inbox.sourceware.org/gdb-patches/20180528174715.A954AD804AD@oc3748833570.ibm.com/

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

gdb/aarch64-tdep.c

@@ -3104,25 +3104,14 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
 
 /* Helper for aarch64_pseudo_read_value.  */
 
-static struct value *
-aarch64_pseudo_read_value_1 (struct gdbarch *gdbarch,
-			     readable_regcache *regcache, int regnum_offset,
-			     int regsize, struct value *result_value)
+static value *
+aarch64_pseudo_read_value_1 (frame_info_ptr next_frame,
+			     const int pseudo_reg_num, int raw_regnum_offset)
 {
-  unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
-
-  /* Enough space for a full vector register.  */
-  gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
-  static_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+  unsigned v_regnum = AARCH64_V0_REGNUM + raw_regnum_offset;
 
-  if (regcache->raw_read (v_regnum, reg_buf) != REG_VALID)
-    result_value->mark_bytes_unavailable (0,
-					  result_value->type ()->length ());
-  else
-    memcpy (result_value->contents_raw ().data (), reg_buf, regsize);
-
-  return result_value;
- }
+  return pseudo_from_raw_part (next_frame, pseudo_reg_num, v_regnum, 0);
+}
 
 /* Helper function for reading/writing ZA pseudo-registers.  Given REGNUM,
    a ZA pseudo-register number, return, in OFFSETS, the information on positioning
@@ -3205,54 +3194,47 @@ aarch64_za_offsets_from_regnum (struct gdbarch *gdbarch, int regnum,
 
 /* Given REGNUM, a SME pseudo-register number, return its value in RESULT.  */
 
-static struct value *
-aarch64_sme_pseudo_register_read (struct gdbarch *gdbarch,
-				  readable_regcache *regcache, int regnum,
-				  struct value *result)
+static value *
+aarch64_sme_pseudo_register_read (gdbarch *gdbarch, frame_info_ptr next_frame,
+				  const int pseudo_reg_num)
 {
   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
 
   gdb_assert (tdep->has_sme ());
   gdb_assert (tdep->sme_svq > 0);
-  gdb_assert (tdep->sme_pseudo_base <= regnum);
-  gdb_assert (regnum < tdep->sme_pseudo_base + tdep->sme_pseudo_count);
+  gdb_assert (tdep->sme_pseudo_base <= pseudo_reg_num);
+  gdb_assert (pseudo_reg_num < tdep->sme_pseudo_base + tdep->sme_pseudo_count);
 
   /* Fetch the offsets that we need in order to read from the correct blocks
      of ZA.  */
   struct za_offsets offsets;
-  aarch64_za_offsets_from_regnum (gdbarch, regnum, offsets);
+  aarch64_za_offsets_from_regnum (gdbarch, pseudo_reg_num, offsets);
 
   /* Fetch the contents of ZA.  */
-  size_t svl = sve_vl_from_vq (tdep->sme_svq);
-  gdb::byte_vector za (std::pow (svl, 2));
-  regcache->raw_read (tdep->sme_za_regnum, za.data ());
+  value *za_value = value_of_register (tdep->sme_za_regnum, next_frame);
+  value *result = value::allocate_register (next_frame, pseudo_reg_num);
 
   /* Copy the requested data.  */
   for (int chunks = 0; chunks < offsets.chunks; chunks++)
     {
-      const gdb_byte *source
-	= za.data () + offsets.starting_offset + chunks * offsets.stride_size;
-      gdb_byte *destination
-	= result->contents_raw ().data () + chunks * offsets.chunk_size;
-
-      memcpy (destination, source, offsets.chunk_size);
+      int src_offset = offsets.starting_offset + chunks * offsets.stride_size;
+      int dst_offset = chunks * offsets.chunk_size;
+      za_value->contents_copy (result, dst_offset, src_offset,
+			       offsets.chunk_size);
     }
+
   return result;
 }
 
 /* Implement the "pseudo_register_read_value" gdbarch method.  */
 
-static struct value *
-aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
-			   int regnum)
+static value *
+aarch64_pseudo_read_value (gdbarch *gdbarch, frame_info_ptr next_frame,
+			   const int pseudo_reg_num)
 {
   aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
-  struct value *result_value = value::allocate (register_type (gdbarch, regnum));
 
-  result_value->set_lval (lval_register);
-  VALUE_REGNUM (result_value) = regnum;
-
-  if (is_w_pseudo_register (gdbarch, regnum))
+  if (is_w_pseudo_register (gdbarch, pseudo_reg_num))
     {
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
       /* Default offset for little endian.  */
@@ -3262,53 +3244,49 @@ aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
 	offset = 4;
 
       /* Find the correct X register to extract the data from.  */
-      int x_regnum = AARCH64_X0_REGNUM + (regnum - tdep->w_pseudo_base);
-      gdb_byte data[4];
+      int x_regnum
+	= AARCH64_X0_REGNUM + (pseudo_reg_num - tdep->w_pseudo_base);
 
       /* Read the bottom 4 bytes of X.  */
-      if (regcache->raw_read_part (x_regnum, offset, 4, data) != REG_VALID)
-	result_value->mark_bytes_unavailable (0, 4);
-      else
-	memcpy (result_value->contents_raw ().data (), data, 4);
-
-      return result_value;
+      return pseudo_from_raw_part (next_frame, pseudo_reg_num, x_regnum,
+				   offset);
     }
-  else if (is_sme_pseudo_register (gdbarch, regnum))
-    return aarch64_sme_pseudo_register_read (gdbarch, regcache, regnum,
-					     result_value);
-
-  regnum -= gdbarch_num_regs (gdbarch);
-
-  if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_Q0_REGNUM,
-					Q_REGISTER_SIZE, result_value);
-
-  if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_D0_REGNUM,
-					D_REGISTER_SIZE, result_value);
-
-  if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_S0_REGNUM,
-					S_REGISTER_SIZE, result_value);
-
-  if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_H0_REGNUM,
-					H_REGISTER_SIZE, result_value);
-
-  if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_B0_REGNUM,
-					B_REGISTER_SIZE, result_value);
-
-  if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
-      && regnum < AARCH64_SVE_V0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
-					regnum - AARCH64_SVE_V0_REGNUM,
-					V_REGISTER_SIZE, result_value);
+  else if (is_sme_pseudo_register (gdbarch, pseudo_reg_num))
+    return aarch64_sme_pseudo_register_read (gdbarch, next_frame,
+					     pseudo_reg_num);
+
+  /* Offset in the "pseudo-register space".  */
+  int pseudo_offset = pseudo_reg_num - gdbarch_num_regs (gdbarch);
+
+  if (pseudo_offset >= AARCH64_Q0_REGNUM
+      && pseudo_offset < AARCH64_Q0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_Q0_REGNUM);
+
+  if (pseudo_offset >= AARCH64_D0_REGNUM
+      && pseudo_offset < AARCH64_D0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_D0_REGNUM);
+
+  if (pseudo_offset >= AARCH64_S0_REGNUM
+      && pseudo_offset < AARCH64_S0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_S0_REGNUM);
+
+  if (pseudo_offset >= AARCH64_H0_REGNUM
+      && pseudo_offset < AARCH64_H0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_H0_REGNUM);
+
+  if (pseudo_offset >= AARCH64_B0_REGNUM
+      && pseudo_offset < AARCH64_B0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_B0_REGNUM);
+
+  if (tdep->has_sve () && pseudo_offset >= AARCH64_SVE_V0_REGNUM
+      && pseudo_offset < AARCH64_SVE_V0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (next_frame, pseudo_reg_num,
+					pseudo_offset - AARCH64_SVE_V0_REGNUM);
 
   gdb_assert_not_reached ("regnum out of bound");
 }

gdb/amd64-tdep.c

@@ -348,18 +348,12 @@ amd64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
     return i386_pseudo_register_name (gdbarch, regnum);
 }
 
-static struct value *
-amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
-				  readable_regcache *regcache,
+static value *
+amd64_pseudo_register_read_value (gdbarch *gdbarch, frame_info_ptr next_frame,
 				  int regnum)
 {
   i386_gdbarch_tdep *tdep = gdbarch_tdep<i386_gdbarch_tdep> (gdbarch);
 
-  value *result_value = value::allocate (register_type (gdbarch, regnum));
-  result_value->set_lval (lval_register);
-  VALUE_REGNUM (result_value) = regnum;
-  gdb_byte *buf = result_value->contents_raw ().data ();
-
   if (i386_byte_regnum_p (gdbarch, regnum))
     {
       int gpnum = regnum - tdep->al_regnum;
@@ -368,44 +362,21 @@ amd64_pseudo_register_read_value (struct gdbarch *gdbarch,
       if (gpnum >= AMD64_NUM_LOWER_BYTE_REGS)
 	{
 	  gpnum -= AMD64_NUM_LOWER_BYTE_REGS;
-	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
 
 	  /* Special handling for AH, BH, CH, DH.  */
-	  register_status status = regcache->raw_read (gpnum, raw_buf);
-	  if (status == REG_VALID)
-	    memcpy (buf, raw_buf + 1, 1);
-	  else
-	    result_value->mark_bytes_unavailable (0,
-						  result_value->type ()->length ());
+	  return pseudo_from_raw_part (next_frame, regnum, gpnum, 1);
 	}
       else
-	{
-	  gdb_byte raw_buf[register_size (gdbarch, gpnum)];
-	  register_status status = regcache->raw_read (gpnum, raw_buf);
-	  if (status == REG_VALID)
-	    memcpy (buf, raw_buf, 1);
-	  else
-	    result_value->mark_bytes_unavailable (0,
-						  result_value->type ()->length ());
-	}
+	return pseudo_from_raw_part (next_frame, regnum, gpnum, 0);
     }
   else if (i386_dword_regnum_p (gdbarch, regnum))
     {
       int gpnum = regnum - tdep->eax_regnum;
-      gdb_byte raw_buf[register_size (gdbarch, gpnum)];
-      /* Extract (always little endian).  */
-      register_status status = regcache->raw_read (gpnum, raw_buf);
-      if (status == REG_VALID)
-	memcpy (buf, raw_buf, 4);
-      else
-	result_value->mark_bytes_unavailable (0,
-					      result_value->type ()->length ());
+
+      return pseudo_from_raw_part (next_frame, regnum, gpnum, 0);
     }
   else
-    i386_pseudo_register_read_into_value (gdbarch, regcache, regnum,
-					  result_value);
-
-  return result_value;
+    return i386_pseudo_register_read_value (gdbarch, next_frame, regnum);
 }
 
 static void

gdb/dwarf2/frame.c

@@ -1220,7 +1220,10 @@ dwarf2_frame_prev_register (frame_info_ptr this_frame, void **this_cache,
 	 "undefined").  Code above issues a complaint about this.
 	 Here just fudge the books, assume GCC, and that the value is
 	 more inner on the stack.  */
-      return frame_unwind_got_register (this_frame, regnum, regnum);
+      if (regnum < gdbarch_num_regs (gdbarch))
+	return frame_unwind_got_register (this_frame, regnum, regnum);
+      else
+	return nullptr;
 
     case DWARF2_FRAME_REG_SAME_VALUE:
       return frame_unwind_got_register (this_frame, regnum, regnum);

gdb/frame.c

@@ -1257,9 +1257,39 @@ frame_unwind_register_value (frame_info_ptr next_frame, int regnum)
     frame_unwind_find_by_frame (next_frame, &next_frame->prologue_cache);
 
   /* Ask this frame to unwind its register.  */
-  value *value = next_frame->unwind->prev_register (next_frame,
-						    &next_frame->prologue_cache,
-						    regnum);
+  value *value
+    = next_frame->unwind->prev_register (next_frame,
+					 &next_frame->prologue_cache, regnum);
+  if (value == nullptr)
+    {
+      if (gdbarch_pseudo_register_read_value_p (gdbarch))
+	{
+	  /* This is a pseudo register, we don't know how how what raw registers
+	     this pseudo register is made of.  Ask the gdbarch to read the
+	     value, it will itself ask the next frame to unwind the values of
+	     the raw registers it needs to compose the value of the pseudo
+	     register.  */
+	  value = gdbarch_pseudo_register_read_value
+	    (gdbarch, next_frame, regnum);
+	}
+      else if (gdbarch_pseudo_register_read_p (gdbarch))
+	{
+	  value = value::allocate_register (next_frame, regnum);
+
+	  /* Passing the current regcache is known to be broken, the pseudo
+	     register value will be constructed using the current raw registers,
+	     rather than reading them using NEXT_FRAME.  Architectures should be
+	     migrated to gdbarch_pseudo_register_read_value.  */
+	  register_status status = gdbarch_pseudo_register_read
+	    (gdbarch, get_thread_regcache (inferior_thread ()), regnum,
+	     value->contents_writeable ().data ());
+	  if (status == REG_UNAVAILABLE)
+	    value->mark_bytes_unavailable (0, value->type ()->length ());
+	}
+      else
+	error (_("Can't unwind value of register %d (%s)"), regnum,
+	       user_reg_map_regnum_to_name (gdbarch, regnum));
+    }
 
   if (frame_debug)
     {

gdb/gdbarch-gen.h

@@ -196,8 +196,8 @@ extern void set_gdbarch_pseudo_register_read (struct gdbarch *gdbarch, gdbarch_p
 
 extern bool gdbarch_pseudo_register_read_value_p (struct gdbarch *gdbarch);
 
-typedef struct value * (gdbarch_pseudo_register_read_value_ftype) (struct gdbarch *gdbarch, readable_regcache *regcache, int cookednum);
-extern struct value * gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, readable_regcache *regcache, int cookednum);
+typedef struct value * (gdbarch_pseudo_register_read_value_ftype) (struct gdbarch *gdbarch, frame_info_ptr next_frame, int cookednum);
+extern struct value * gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, frame_info_ptr next_frame, int cookednum);
 extern void set_gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, gdbarch_pseudo_register_read_value_ftype *pseudo_register_read_value);
 
 extern bool gdbarch_pseudo_register_write_p (struct gdbarch *gdbarch);

gdb/gdbarch.c

@@ -1886,13 +1886,13 @@ gdbarch_pseudo_register_read_value_p (struct gdbarch *gdbarch)
 }
 
 struct value *
-gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, readable_regcache *regcache, int cookednum)
+gdbarch_pseudo_register_read_value (struct gdbarch *gdbarch, frame_info_ptr next_frame, int cookednum)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->pseudo_register_read_value != NULL);
   if (gdbarch_debug >= 2)
     gdb_printf (gdb_stdlog, "gdbarch_pseudo_register_read_value called\n");
-  return gdbarch->pseudo_register_read_value (gdbarch, regcache, cookednum);
+  return gdbarch->pseudo_register_read_value (gdbarch, next_frame, cookednum);
 }
 
 void

gdb/gdbarch_components.py

@@ -414,7 +414,7 @@
 """,
     type="struct value *",
     name="pseudo_register_read_value",
-    params=[("readable_regcache *", "regcache"), ("int", "cookednum")],
+    params=[("frame_info_ptr", "next_frame"), ("int", "cookednum")],
     predicate=True,
 )