[RFC] Document replacement for frame_register_read (deprecated).

[RFC] Document replacement for frame_register_read (deprecated).

[Joel Brobecker]

Hello,

This is inspired by a discussion from a long time ago:
http://www.sourceware.org/ml/gdb-patches/2011-03/msg01095.html

If you agree with the patch, we will also have to update the ARI
script to mention get_frame_register_value as the alternative.

And we might also want to rename the function as well. I don't think
we can rely on the ARI alone to avoid new uses of this function.
And once new uses are in, it's much harder to make sure we undo them.

Thoughts?

gdb/ChangeLog:

        * frame.h (frame_register_read): Remove FIXME comment.
        * frame.c (frame_register_read): Add suggestion explaining
        which function to use in place of this one.

Thanks,
-- 
Joel


---
 gdb/frame.c |    3 ++-
 gdb/frame.h |   12 ------------
 2 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index a2f23a4..ea0cb90 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -1093,7 +1093,8 @@ put_frame_register (struct frame_info *frame, int regnum,
     }
 }
 
-/* frame_register_read ()
+/* This function is deprecated.  Use get_frame_register_value instead,
+   which provides more accurate information.
 
    Find and return the value of REGNUM for the specified stack frame.
    The number of bytes copied is REGISTER_SIZE (REGNUM).
diff --git a/gdb/frame.h b/gdb/frame.h
index fa80663..c0559d9 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -673,18 +673,6 @@ extern struct frame_info *block_innermost_frame (const struct block *);
 
 extern int deprecated_pc_in_call_dummy (struct gdbarch *gdbarch, CORE_ADDR pc);
 
-/* FIXME: cagney/2003-02-02: Should be deprecated or replaced with a
-   function called get_frame_register_p().  This slightly weird (and
-   older) variant of get_frame_register() returns zero (indicating the
-   register value is unavailable/invalid) if either: the register
-   isn't cached; or the register has been optimized out; or the
-   register contents are unavailable (because they haven't been
-   collected in a traceframe).  Problem is, neither check is exactly
-   correct.  A register can't be optimized out (it may not have been
-   saved as part of a function call); The fact that a register isn't
-   in the register cache doesn't mean that the register isn't
-   available (it could have been fetched from memory).  */
-
 extern int frame_register_read (struct frame_info *frame, int regnum,
 				gdb_byte *buf);
 
-- 
1.7.9.5

Re: [RFC] Document replacement for frame_register_read (deprecated).

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

Joel> This is inspired by a discussion from a long time ago:
Joel> http://www.sourceware.org/ml/gdb-patches/2011-03/msg01095.html

Joel> If you agree with the patch, we will also have to update the ARI
Joel> script to mention get_frame_register_value as the alternative.

Joel> And we might also want to rename the function as well. I don't think
Joel> we can rely on the ARI alone to avoid new uses of this function.
Joel> And once new uses are in, it's much harder to make sure we undo them.

Joel> Thoughts?

The patch seems reasonable to me.

Renaming seems like a decent idea, since there is a use in jit.c, which
was added long after this was deprecated.

I count 22 mentions (including the declaration) of frame_register_read
in the tree right now.  Getting rid of all of them doesn't seem so
difficult, aside from the testing.  This would be clearly better...

At least one of the current uses seems suspicious since it ignores the
result.  I assume I'm missing some context though.

Tom

Re: [RFC] Document replacement for frame_register_read (deprecated).

[Pedro Alves]

On 10/23/2012 06:19 PM, Tom Tromey wrote:
>>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel> This is inspired by a discussion from a long time ago:
> Joel> http://www.sourceware.org/ml/gdb-patches/2011-03/msg01095.html
> 
> Joel> If you agree with the patch, we will also have to update the ARI
> Joel> script to mention get_frame_register_value as the alternative.
> 
> Joel> And we might also want to rename the function as well. I don't think
> Joel> we can rely on the ARI alone to avoid new uses of this function.
> Joel> And once new uses are in, it's much harder to make sure we undo them.
> 
> Joel> Thoughts?
> 
> The patch seems reasonable to me.

Me too.

> 
> Renaming seems like a decent idea, since there is a use in jit.c, which
> was added long after this was deprecated.

Yeah.

> I count 22 mentions (including the declaration) of frame_register_read
> in the tree right now.  Getting rid of all of them doesn't seem so
> difficult, aside from the testing.  This would be clearly better...
> 
> At least one of the current uses seems suspicious since it ignores the
> result.  I assume I'm missing some context though.

I took a look too.

> frame.c:775:do_frame_register_read (void *src, int regnum, gdb_byte *buf)
> frame.c:777:  if (!frame_register_read (src, regnum, buf))

Probably innocuous, but need to check if there's a path that calls
this for frames other than #0, or if possible to get here for cores and
tracing (IIRC, this is used for infcalls, which can't work on those cases).
If so, since failure always maps to REG_UNAVAILABLE, we're losing info here.

> frame.c:1218:     frame_register_read (frame, regnum, buf);

A read-modify-write that doesn't take in account whether the read
was successful.  Can't be good.

> i386-tdep.c:1748:         && frame_register_read (this_frame, cache->saved_sp_reg, buf))

I think it's okay.

> infcmd.c:2052:      if (! frame_register_read (frame, i, value_contents_raw (val)))

I guess we should be saying "*optimized out*" if the register is not saved, and
leave "*value not available*" for the case where we can't get at it because it's
missing from the truncated core or from the tracebuffer.

> jit.c:1056:  value->defined = frame_register_read (priv->this_frame, gdb_reg,

No idea.

> mips-tdep.c:4606:  if (!frame_register_read (frame, regno, raw_buffer))
> mips-tdep.c:4643:      if (!frame_register_read (frame, regno, rare_buffer))
> mips-tdep.c:4879:      if (!frame_register_read (frame, regnum, raw_buffer))
> mt-tdep.c:688:    frame_register_read (frame, regnum, buff);
> mt-tdep.c:713:    frame_register_read (frame, MT_COPRO_REGNUM, buf);
> mt-tdep.c:734:    frame_register_read (frame, MT_MAC_REGNUM, buf);
> mt-tdep.c:740:        frame_register_read (frame, MT_EXMAC_REGNUM, buf);
> sh64-tdep.c:2054:  if (!frame_register_read (frame, regnum, raw_buffer))
> sh64-tdep.c:2170:  if (!frame_register_read (frame, regnum, raw_buffer))

All these should be fixed to handle unavailableness gracefully.  It just
hasn't been pressing no target with with arch can do tracepoints,
AFAIK (though there are other ways the registers could be missing).

-- 
Pedro Alves

Re: [RFC] Document replacement for frame_register_read (deprecated).

[Joel Brobecker]

> The patch seems reasonable to me.

Thanks. I just checked it in.

> Renaming seems like a decent idea, since there is a use in jit.c, which
> was added long after this was deprecated.
> 
> I count 22 mentions (including the declaration) of frame_register_read
> in the tree right now.  Getting rid of all of them doesn't seem so
> difficult, aside from the testing.  This would be clearly better...

Yeah - I was hoping to have the time to do that, but I don't think
it's completely straightforward.

For now, I'll do a quick and replace to clearly mark as deprecated.
If I have a little more time, I'll try to fix some of the current
uses...

-- 
Joel

Re: [RFC] Document replacement for frame_register_read (deprecated).

[Joel Brobecker]

[-- Attachment #1: Type: text/plain, Size: 495 bytes --]

> For now, I'll do a quick and replace to clearly mark as deprecated.
> If I have a little more time, I'll try to fix some of the current
> uses...

Attached is the patch that I just checked in. I tested it on
x86_64-linux by building GDB with --enable-targets=all and I ran
it through the testsuite, JIC. No regression.

I'll keep this item on my list, to see if I can look at at least
at the ones that are in the core part of GDB, and maybe at those
that are for targets I can test.

-- 
Joel

[-- Attachment #2: 0001-rename-frame_register_read-into-deprecated_frame_reg.patch --]
[-- Type: text/x-diff, Size: 8318 bytes --]

From 11dfea93f9c607b1800394ce1d84f878ea82c37c Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Mon, 12 Nov 2012 13:48:55 -0800
Subject: [PATCH] rename frame_register_read into
 deprecated_frame_register_read.

gdb/ChangeLog:

        * frame.h (deprecated_frame_register_read): Renames
        frame_register_read.
        * frame.c (deprecated_frame_register_read): Renames
        frame_register_read.  Update all callers.
	* i386-tdep.c: Update all callers of frame_register_read.
	* infcmd.c: Likewise.
	* jit.c: Likewise.
	* mips-tdep.c: Likewise.
	* mt-tdep.c: Likewise.
	* sh64-tdep.c: Likewise.
---
 gdb/frame.c     |    6 +++---
 gdb/frame.h     |    2 +-
 gdb/i386-tdep.c |    3 ++-
 gdb/infcmd.c    |    2 +-
 gdb/jit.c       |    4 ++--
 gdb/mips-tdep.c |    6 +++---
 gdb/mt-tdep.c   |    8 ++++----
 gdb/sh64-tdep.c |    4 ++--
 8 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/gdb/frame.c b/gdb/frame.c
index 52498ba..bf034a8 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -776,7 +776,7 @@ get_frame_func (struct frame_info *this_frame)
 static enum register_status
 do_frame_register_read (void *src, int regnum, gdb_byte *buf)
 {
-  if (!frame_register_read (src, regnum, buf))
+  if (!deprecated_frame_register_read (src, regnum, buf))
     return REG_UNAVAILABLE;
   else
     return REG_VALID;
@@ -1097,7 +1097,7 @@ put_frame_register (struct frame_info *frame, int regnum,
    Returns 0 if the register value could not be found.  */
 
 int
-frame_register_read (struct frame_info *frame, int regnum,
+deprecated_frame_register_read (struct frame_info *frame, int regnum,
 		     gdb_byte *myaddr)
 {
   int optimized;
@@ -1218,7 +1218,7 @@ put_frame_register_bytes (struct frame_info *frame, int regnum,
 	{
 	  gdb_byte buf[MAX_REGISTER_SIZE];
 
-	  frame_register_read (frame, regnum, buf);
+	  deprecated_frame_register_read (frame, regnum, buf);
 	  memcpy (buf + offset, myaddr, curr_len);
 	  put_frame_register (frame, regnum, buf);
 	}
diff --git a/gdb/frame.h b/gdb/frame.h
index c0559d9..1032904 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -673,7 +673,7 @@ extern struct frame_info *block_innermost_frame (const struct block *);
 
 extern int deprecated_pc_in_call_dummy (struct gdbarch *gdbarch, CORE_ADDR pc);
 
-extern int frame_register_read (struct frame_info *frame, int regnum,
+extern int deprecated_frame_register_read (struct frame_info *frame, int regnum,
 				gdb_byte *buf);
 
 /* From stack.c.  */
diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
index b6879b9..f0056be 100644
--- a/gdb/i386-tdep.c
+++ b/gdb/i386-tdep.c
@@ -1783,7 +1783,8 @@ i386_frame_cache_1 (struct frame_info *this_frame,
       /* Saved stack pointer has been saved (but the SAVED_SP_REG
 	 register may be unavailable).  */
       if (cache->saved_sp == 0
-	  && frame_register_read (this_frame, cache->saved_sp_reg, buf))
+	  && deprecated_frame_register_read (this_frame,
+					     cache->saved_sp_reg, buf))
 	cache->saved_sp = extract_unsigned_integer (buf, 4, byte_order);
     }
   /* Now that we have the base address for the stack frame we can
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index c3e602b..7a08e31 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -2155,7 +2155,7 @@ default_print_registers_info (struct gdbarch *gdbarch,
       val = allocate_value (regtype);
 
       /* Get the data in raw format.  */
-      if (! frame_register_read (frame, i, value_contents_raw (val)))
+      if (! deprecated_frame_register_read (frame, i, value_contents_raw (val)))
 	mark_value_bytes_unavailable (val, 0, TYPE_LENGTH (value_type (val)));
 
       default_print_one_register_info (file,
diff --git a/gdb/jit.c b/gdb/jit.c
index 2dcafbc..1afbee6 100644
--- a/gdb/jit.c
+++ b/gdb/jit.c
@@ -1061,8 +1061,8 @@ jit_unwind_reg_get_impl (struct gdb_unwind_callbacks *cb, int regnum)
   gdb_reg = gdbarch_dwarf2_reg_to_regnum (frame_arch, regnum);
   size = register_size (frame_arch, gdb_reg);
   value = xmalloc (sizeof (struct gdb_reg_value) + size - 1);
-  value->defined = frame_register_read (priv->this_frame, gdb_reg,
-                                        value->value);
+  value->defined = deprecated_frame_register_read (priv->this_frame, gdb_reg,
+						   value->value);
   value->size = size;
   value->free = reg_value_free_impl;
   return value;
diff --git a/gdb/mips-tdep.c b/gdb/mips-tdep.c
index 61f2cd2..8140f23 100644
--- a/gdb/mips-tdep.c
+++ b/gdb/mips-tdep.c
@@ -6032,7 +6032,7 @@ mips_read_fp_register_single (struct frame_info *frame, int regno,
   int raw_size = register_size (gdbarch, regno);
   gdb_byte *raw_buffer = alloca (raw_size);
 
-  if (!frame_register_read (frame, regno, raw_buffer))
+  if (!deprecated_frame_register_read (frame, regno, raw_buffer))
     error (_("can't read register %d (%s)"),
 	   regno, gdbarch_register_name (gdbarch, regno));
   if (raw_size == 8)
@@ -6069,7 +6069,7 @@ mips_read_fp_register_double (struct frame_info *frame, int regno,
     {
       /* We have a 64-bit value for this register, and we should use
          all 64 bits.  */
-      if (!frame_register_read (frame, regno, rare_buffer))
+      if (!deprecated_frame_register_read (frame, regno, rare_buffer))
 	error (_("can't read register %d (%s)"),
 	       regno, gdbarch_register_name (gdbarch, regno));
     }
@@ -6302,7 +6302,7 @@ print_gp_register_row (struct ui_file *file, struct frame_info *frame,
 	break;			/* End row: large register.  */
 
       /* OK: get the data in raw format.  */
-      if (!frame_register_read (frame, regnum, raw_buffer))
+      if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
 	error (_("can't read register %d (%s)"),
 	       regnum, gdbarch_register_name (gdbarch, regnum));
       /* pad small registers */
diff --git a/gdb/mt-tdep.c b/gdb/mt-tdep.c
index 0ae51b3..f45ebcf 100644
--- a/gdb/mt-tdep.c
+++ b/gdb/mt-tdep.c
@@ -685,7 +685,7 @@ mt_registers_info (struct gdbarch *gdbarch,
 	  buff = alloca (regsize);
 	  bytes = alloca (regsize * sizeof (*bytes));
 
-	  frame_register_read (frame, regnum, buff);
+	  deprecated_frame_register_read (frame, regnum, buff);
 
 	  fputs_filtered (gdbarch_register_name
 			  (gdbarch, regnum), file);
@@ -710,7 +710,7 @@ mt_registers_info (struct gdbarch *gdbarch,
 	  struct value_print_options opts;
 
 	  buf = alloca (register_size (gdbarch, MT_COPRO_REGNUM));
-	  frame_register_read (frame, MT_COPRO_REGNUM, buf);
+	  deprecated_frame_register_read (frame, MT_COPRO_REGNUM, buf);
 	  /* And print.  */
 	  regnum = MT_COPRO_PSEUDOREG_REGNUM;
 	  fputs_filtered (gdbarch_register_name (gdbarch, regnum),
@@ -731,13 +731,13 @@ mt_registers_info (struct gdbarch *gdbarch,
 	  gdb_byte buf[3 * sizeof (LONGEST)];
 
 	  /* Get the two "real" mac registers.  */
-	  frame_register_read (frame, MT_MAC_REGNUM, buf);
+	  deprecated_frame_register_read (frame, MT_MAC_REGNUM, buf);
 	  oldmac = extract_unsigned_integer
 	    (buf, register_size (gdbarch, MT_MAC_REGNUM), byte_order);
 	  if (gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_mrisc2
 	      || gdbarch_bfd_arch_info (gdbarch)->mach == bfd_mach_ms2)
 	    {
-	      frame_register_read (frame, MT_EXMAC_REGNUM, buf);
+	      deprecated_frame_register_read (frame, MT_EXMAC_REGNUM, buf);
 	      ext_mac = extract_unsigned_integer
 		(buf, register_size (gdbarch, MT_EXMAC_REGNUM), byte_order);
 	    }
diff --git a/gdb/sh64-tdep.c b/gdb/sh64-tdep.c
index e11e746..5d649df 100644
--- a/gdb/sh64-tdep.c
+++ b/gdb/sh64-tdep.c
@@ -1931,7 +1931,7 @@ sh64_do_fp_register (struct gdbarch *gdbarch, struct ui_file *file,
     alloca (register_size (gdbarch, gdbarch_fp0_regnum (gdbarch)));
 
   /* Get the data in raw format.  */
-  if (!frame_register_read (frame, regnum, raw_buffer))
+  if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
     error (_("can't read register %d (%s)"),
 	   regnum, gdbarch_register_name (gdbarch, regnum));
 
@@ -2047,7 +2047,7 @@ sh64_do_register (struct gdbarch *gdbarch, struct ui_file *file,
 				      (gdbarch, regnum)), file);
 
   /* Get the data in raw format.  */
-  if (!frame_register_read (frame, regnum, raw_buffer))
+  if (!deprecated_frame_register_read (frame, regnum, raw_buffer))
     fprintf_filtered (file, "*value not available*\n");
 
   get_formatted_print_options (&opts, 'x');
-- 
1.7.9.5