[RFA] Fix DW_OP_GNU_regval_type with FP registers

[RFA] Fix DW_OP_GNU_regval_type with FP registers

[Joel Brobecker]

Hello,

Consider the following code, compiled at -O2 on ppc-linux:

    procedure Increment (Val : in out Float; Msg : String);

The implementation does not really matter in this case). In our example,
this function is being called from a function with Param_1 set to 99.0.
Trying to break inside that function, and running until reaching that
breakpoint yields:

    (gdb) b increment
    Breakpoint 1 at 0x100014b4: file callee.adb, line 6.
    (gdb) run
    Starting program: /[...]/foo

    Breakpoint 1, callee.increment (val=99.0, val@entry=0.0, msg=...)
        at callee.adb:6
    6             if Val > 200.0 then

The @entry value for parameter "val" is incorrect, it should be 99.0.

The associated call-site parameter DIE looks like this:

        .uleb128 0xc     # (DIE (0x115) DW_TAG_GNU_call_site_parameter)
        .byte   0x2      # DW_AT_location
        .byte   0x90     # DW_OP_regx
        .uleb128 0x21
        .byte   0x3      # DW_AT_GNU_call_site_value
        .byte   0xf5     # DW_OP_GNU_regval_type
        .uleb128 0x3f
        .uleb128 0x25

The DW_AT_GNU_call_site_value uses a DW_OP_GNU_regval_type
operation, referencing register 0x3f=63, which is $f31,
an 8-byte floating register. In that register, the value is
stored using the usual 8-byte float format:

    (gdb) info float
    f31            99.0 (raw 0x4058c00000000000)

The current code evaluating DW_OP_GNU_regval_type operations
currently is (dwarf2expr.c:execute_stack_op):

            result = (ctx->funcs->read_reg) (ctx->baton, reg);
            result_val = value_from_ulongest (address_type, result);
            result_val = value_from_contents (type,
                                              value_contents_all (result_val));

What the ctx->funcs->read_reg function does is read the contents
of the register as if it contained an address. The rest of the code
continues that assumption, thinking it's OK to then use that to
create an address/ulongest struct value, which we then re-type
to the type specified by DW_OP_GNU_regval_type.

We're getting 0.0 above because the read_reg implementations
end up treating the contents of the FP register as an integral,
reading only 4 out of the 8 bytes. Being a big-endian target,
we read the high-order ones, which gives us zero.

This patch fixes the problem by introducing a new callback to
read the contents of a register as a given type, and then adjust
the handling of DW_OP_GNU_regval_type to use that new callback.

gdb/ChangeLog:

        * dwarf2expr.h (struct dwarf_expr_context_funcs) <read_reg>:
        Extend the documentation a bit.
        <get_reg_value>: New field.
        * dwarf2loc.c (dwarf_expr_get_reg_value)
        (needs_frame_get_reg_value): New functions.
        (dwarf_expr_ctx_funcs, needs_frame_ctx_funcs): Add "get_reg_value"
        callback.
        * dwarf2-frame.c (get_reg_value): New function.
        (dwarf2_frame_ctx_funcs): Add "get_reg_value" callback.
        * dwarf2expr.c (execute_stack_op) <DW_OP_GNU_regval_type>:
        Use new callback to compute result_val.

gdb/testsuite/ChangeLog:

        * O2_float_param: New testcase.

tested on x86_64-linux and ppc-linux.
OK to commit?

Thank you,
-- 
Joel

---
 gdb/dwarf2-frame.c                              |   13 +++++++++
 gdb/dwarf2expr.c                                |    5 +---
 gdb/dwarf2expr.h                                |    9 ++++++-
 gdb/dwarf2loc.c                                 |   26 +++++++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param.exp        |   31 +++++++++++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/callee.adb |   26 +++++++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/callee.ads |   18 +++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/caller.adb |   26 +++++++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/caller.ads |   19 ++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/foo.adb    |   22 ++++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/io.adb     |   21 +++++++++++++++
 gdb/testsuite/gdb.ada/O2_float_param/io.ads     |   18 +++++++++++++
 12 files changed, 229 insertions(+), 5 deletions(-)
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param.exp
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/callee.adb
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/callee.ads
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/caller.adb
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/caller.ads
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/foo.adb
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/io.adb
 create mode 100644 gdb/testsuite/gdb.ada/O2_float_param/io.ads

diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index e05236f..b425d94 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -306,6 +306,18 @@ read_reg (void *baton, int reg)
   return unpack_long (register_type (gdbarch, regnum), buf);
 }
 
+/* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback.  */
+
+static struct value *
+get_reg_value (void *baton, struct type *type, int reg)
+{
+  struct frame_info *this_frame = (struct frame_info *) baton;
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, reg);
+
+  return value_from_register (type, regnum, this_frame);
+}
+
 static void
 read_mem (void *baton, gdb_byte *buf, CORE_ADDR addr, size_t len)
 {
@@ -347,6 +359,7 @@ register %s (#%d) at %s"),
 static const struct dwarf_expr_context_funcs dwarf2_frame_ctx_funcs =
 {
   read_reg,
+  get_reg_value,
   read_mem,
   ctx_no_get_frame_base,
   ctx_no_get_frame_cfa,
diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 752d782..25e9dc4 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -1439,10 +1439,7 @@ execute_stack_op (struct dwarf_expr_context *ctx,
 	    type_die.cu_off = uoffset;
 
 	    type = dwarf_get_base_type (ctx, type_die, 0);
-	    result = (ctx->funcs->read_reg) (ctx->baton, reg);
-	    result_val = value_from_ulongest (address_type, result);
-	    result_val = value_from_contents (type,
-					      value_contents_all (result_val));
+	    result_val = ctx->funcs->get_reg_value (ctx->baton, type, reg);
 	  }
 	  break;
 
diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index e85486a..b4c943e 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -31,9 +31,16 @@ struct dwarf_expr_context;
 
 struct dwarf_expr_context_funcs
 {
-  /* Return the value of register number REGNUM.  */
+  /* Return the value of register number REGNUM (a DWARF register number),
+     read as an address.  */
   CORE_ADDR (*read_reg) (void *baton, int regnum);
 
+  /* Return a value of type TYPE, stored in register number REGNUM
+     of the frame associated to the given BATON.
+
+     REGNUM is a DWARF register number.  */
+  struct value *(*get_reg_value) (void *baton, struct type *type, int regnum);
+
   /* Read LENGTH bytes at ADDR into BUF.  */
   void (*read_mem) (void *baton, gdb_byte *buf, CORE_ADDR addr, size_t length);
 
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 8242dca..8b6eb66 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -326,6 +326,18 @@ dwarf_expr_read_reg (void *baton, int dwarf_regnum)
   return result;
 }
 
+/* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback.  */
+
+static struct value *
+dwarf_expr_get_reg_value (void *baton, struct type *type, int dwarf_regnum)
+{
+  struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
+  struct gdbarch *gdbarch = get_frame_arch (debaton->frame);
+  int regnum = gdbarch_dwarf2_reg_to_regnum (gdbarch, dwarf_regnum);
+
+  return value_from_register (type, regnum, debaton->frame);
+}
+
 /* Read memory at ADDR (length LEN) into BUF.  */
 
 static void
@@ -2182,6 +2194,7 @@ static const struct lval_funcs pieced_value_funcs = {
 static const struct dwarf_expr_context_funcs dwarf_expr_ctx_funcs =
 {
   dwarf_expr_read_reg,
+  dwarf_expr_get_reg_value,
   dwarf_expr_read_mem,
   dwarf_expr_frame_base,
   dwarf_expr_frame_cfa,
@@ -2435,6 +2448,18 @@ needs_frame_read_reg (void *baton, int regnum)
   return 1;
 }
 
+/* struct dwarf_expr_context_funcs' "get_reg_value" callback:
+   Reads from registers do require a frame.  */
+
+static struct value *
+needs_frame_get_reg_value (void *baton, struct type *type, int regnum)
+{
+  struct needs_frame_baton *nf_baton = baton;
+
+  nf_baton->needs_frame = 1;
+  return value_zero (type, not_lval);
+}
+
 /* Reads from memory do not require a frame.  */
 static void
 needs_frame_read_mem (void *baton, gdb_byte *buf, CORE_ADDR addr, size_t len)
@@ -2516,6 +2541,7 @@ needs_get_addr_index (void *baton, unsigned int index)
 static const struct dwarf_expr_context_funcs needs_frame_ctx_funcs =
 {
   needs_frame_read_reg,
+  needs_frame_get_reg_value,
   needs_frame_read_mem,
   needs_frame_frame_base,
   needs_frame_frame_cfa,
diff --git a/gdb/testsuite/gdb.ada/O2_float_param.exp b/gdb/testsuite/gdb.ada/O2_float_param.exp
new file mode 100644
index 0000000..8b891e8
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param.exp
@@ -0,0 +1,31 @@
+# Copyright 2013 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib "ada.exp"
+
+if { [skip_ada_tests] } { return -1 }
+
+standard_ada_testfile foo
+
+if {[gdb_compile_ada "${srcfile}" "${binfile}" executable {debug optimize=-O2}] != ""} {
+    return -1
+}
+
+clean_restart ${testfile}
+
+runto "increment"
+
+gdb_test "frame" \
+         "#0\\s+callee\\.increment \\(val(=val@entry)?=99\\.0, msg=\\.\\.\\.\\).*"
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/callee.adb b/gdb/testsuite/gdb.ada/O2_float_param/callee.adb
new file mode 100644
index 0000000..0ce4024
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/callee.adb
@@ -0,0 +1,26 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with IO; use IO;
+
+package body Callee is
+   procedure Increment (Val : in out Float; Msg: String) is
+   begin
+      if Val > 200.0 then
+         Put_Line (Msg);
+      end if;
+      Val := Val + 1.0;
+   end Increment;
+end Callee;
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/callee.ads b/gdb/testsuite/gdb.ada/O2_float_param/callee.ads
new file mode 100644
index 0000000..92d97c3
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/callee.ads
@@ -0,0 +1,18 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package Callee is
+   procedure Increment (Val : in out Float; Msg : String);
+end Callee;
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/caller.adb b/gdb/testsuite/gdb.ada/O2_float_param/caller.adb
new file mode 100644
index 0000000..36666c2
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/caller.adb
@@ -0,0 +1,26 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with IO; use IO;
+with Callee; use Callee;
+
+package body Caller is
+   procedure Verbose_Increment (Val : in out Float; Msg : String) is
+   begin
+      Put_Line ("DEBUG: " & Msg);
+      Increment (Val, "Verbose_Increment");
+   end Verbose_Increment;
+end Caller;
+
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/caller.ads b/gdb/testsuite/gdb.ada/O2_float_param/caller.ads
new file mode 100644
index 0000000..e44f62b
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/caller.ads
@@ -0,0 +1,19 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package Caller is
+   procedure Verbose_Increment (Val : in out Float; Msg : String);
+end Caller;
+
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/foo.adb b/gdb/testsuite/gdb.ada/O2_float_param/foo.adb
new file mode 100644
index 0000000..d6289ff
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/foo.adb
@@ -0,0 +1,22 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+with Caller; use Caller;
+
+procedure Foo is
+   Num : Float := 99.0;
+begin
+   Verbose_Increment (Num, "Foo");
+end Foo;
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/io.adb b/gdb/testsuite/gdb.ada/O2_float_param/io.adb
new file mode 100644
index 0000000..7ef1689
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/io.adb
@@ -0,0 +1,21 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package body IO is
+   procedure Put_Line (S : String) is
+   begin
+      null;
+   end Put_Line;
+end IO;
diff --git a/gdb/testsuite/gdb.ada/O2_float_param/io.ads b/gdb/testsuite/gdb.ada/O2_float_param/io.ads
new file mode 100644
index 0000000..a09f4fe
--- /dev/null
+++ b/gdb/testsuite/gdb.ada/O2_float_param/io.ads
@@ -0,0 +1,18 @@
+--  Copyright 2013 Free Software Foundation, Inc.
+--
+--  This program is free software; you can redistribute it and/or modify
+--  it under the terms of the GNU General Public License as published by
+--  the Free Software Foundation; either version 3 of the License, or
+--  (at your option) any later version.
+--
+--  This program is distributed in the hope that it will be useful,
+--  but WITHOUT ANY WARRANTY; without even the implied warranty of
+--  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+--  GNU General Public License for more details.
+--
+--  You should have received a copy of the GNU General Public License
+--  along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package IO is
+   procedure Put_Line (S : String);
+end IO;
-- 
1.7.0.4

Re: [RFA] Fix DW_OP_GNU_regval_type with FP registers

[Pedro Alves]

On 11/14/2013 01:02 PM, Joel Brobecker wrote:
> Hello,
> 
> Consider the following code, compiled at -O2 on ppc-linux:
> 
>     procedure Increment (Val : in out Float; Msg : String);
> 
> The implementation does not really matter in this case). In our example,
> this function is being called from a function with Param_1 set to 99.0.
> Trying to break inside that function, and running until reaching that
> breakpoint yields:
> 
>     (gdb) b increment
>     Breakpoint 1 at 0x100014b4: file callee.adb, line 6.
>     (gdb) run
>     Starting program: /[...]/foo
> 
>     Breakpoint 1, callee.increment (val=99.0, val@entry=0.0, msg=...)
>         at callee.adb:6
>     6             if Val > 200.0 then
> 
> The @entry value for parameter "val" is incorrect, it should be 99.0.
> 
> The associated call-site parameter DIE looks like this:
> 
>         .uleb128 0xc     # (DIE (0x115) DW_TAG_GNU_call_site_parameter)
>         .byte   0x2      # DW_AT_location
>         .byte   0x90     # DW_OP_regx
>         .uleb128 0x21
>         .byte   0x3      # DW_AT_GNU_call_site_value
>         .byte   0xf5     # DW_OP_GNU_regval_type
>         .uleb128 0x3f
>         .uleb128 0x25
> 
> The DW_AT_GNU_call_site_value uses a DW_OP_GNU_regval_type
> operation, referencing register 0x3f=63, which is $f31,
> an 8-byte floating register. In that register, the value is
> stored using the usual 8-byte float format:
> 
>     (gdb) info float
>     f31            99.0 (raw 0x4058c00000000000)
> 
> The current code evaluating DW_OP_GNU_regval_type operations
> currently is (dwarf2expr.c:execute_stack_op):
> 
>             result = (ctx->funcs->read_reg) (ctx->baton, reg);
>             result_val = value_from_ulongest (address_type, result);
>             result_val = value_from_contents (type,
>                                               value_contents_all (result_val));
> 
> What the ctx->funcs->read_reg function does is read the contents
> of the register as if it contained an address. The rest of the code
> continues that assumption, thinking it's OK to then use that to
> create an address/ulongest struct value, which we then re-type
> to the type specified by DW_OP_GNU_regval_type.
> 
> We're getting 0.0 above because the read_reg implementations
> end up treating the contents of the FP register as an integral,
> reading only 4 out of the 8 bytes. 
>
> Being a big-endian target,
> we read the high-order ones, which gives us zero.
> 
> This patch fixes the problem by introducing a new callback to
> read the contents of a register as a given type, and then adjust
> the handling of DW_OP_GNU_regval_type to use that new callback.

I definitely agree with this.  The patch looked fine to me.

(Eliminating read_reg might be tricky, but I think it'd be nice
if it were be renamed to something that implies "address", like
read_addr_reg or some such.)

-- 
Pedro Alves

Re: [RFA] Fix DW_OP_GNU_regval_type with FP registers

[Tom Tromey]

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

Joel>         * dwarf2expr.h (struct dwarf_expr_context_funcs) <read_reg>:
Joel>         Extend the documentation a bit.
Joel>         <get_reg_value>: New field.
Joel>         * dwarf2loc.c (dwarf_expr_get_reg_value)
Joel>         (needs_frame_get_reg_value): New functions.
Joel>         (dwarf_expr_ctx_funcs, needs_frame_ctx_funcs): Add "get_reg_value"
Joel>         callback.
Joel>         * dwarf2-frame.c (get_reg_value): New function.
Joel>         (dwarf2_frame_ctx_funcs): Add "get_reg_value" callback.
Joel>         * dwarf2expr.c (execute_stack_op) <DW_OP_GNU_regval_type>:
Joel>         Use new callback to compute result_val.

Joel> gdb/testsuite/ChangeLog:

Joel>         * O2_float_param: New testcase.

Joel> tested on x86_64-linux and ppc-linux.
Joel> OK to commit?

It looks good to me.

Tom

Re: [RFA] Fix DW_OP_GNU_regval_type with FP registers

[Joel Brobecker]

> >>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:
> 
> Joel>         * dwarf2expr.h (struct dwarf_expr_context_funcs) <read_reg>:
> Joel>         Extend the documentation a bit.
> Joel>         <get_reg_value>: New field.
> Joel>         * dwarf2loc.c (dwarf_expr_get_reg_value)
> Joel>         (needs_frame_get_reg_value): New functions.
> Joel>         (dwarf_expr_ctx_funcs, needs_frame_ctx_funcs): Add "get_reg_value"
> Joel>         callback.
> Joel>         * dwarf2-frame.c (get_reg_value): New function.
> Joel>         (dwarf2_frame_ctx_funcs): Add "get_reg_value" callback.
> Joel>         * dwarf2expr.c (execute_stack_op) <DW_OP_GNU_regval_type>:
> Joel>         Use new callback to compute result_val.
> 
> Joel> gdb/testsuite/ChangeLog:
> 
> Joel>         * O2_float_param: New testcase.
> 
> Joel> tested on x86_64-linux and ppc-linux.
> Joel> OK to commit?

Thanks both, Pedro and Tom, for reviewing the patch. I immediately
starting thinking about Pedro's suggestion...

> (Eliminating read_reg might be tricky, but I think it'd be nice
> if it were be renamed to something that implies "address", like
> read_addr_reg or some such.)

... and forgot to announce here that I pushed the patch this morning.

Regarding Pedro's suggestion, what do we think of "read_addr_from_reg"?
Still not perfect, as it hides a bit the fact that it only does
a partial register read if the address size is smaller than the
register size. But much better than "read_reg", I agree.

-- 
Joel

Re: [RFA] Fix DW_OP_GNU_regval_type with FP registers

[Pedro Alves]

On 11/15/2013 12:06 PM, Joel Brobecker wrote:
> 
> Regarding Pedro's suggestion, what do we think of "read_addr_from_reg"?
> Still not perfect, as it hides a bit the fact that it only does
> a partial register read if the address size is smaller than the
> register size. But much better than "read_reg", I agree.

That sounds fine to me.  It maps well to address_from_register.

-- 
Pedro Alves

[RFA] Rename "read_reg" into "read_addr_from_reg" in struct dwarf_expr_context_funcs

[Joel Brobecker]

Hello,

This implements the suggestion discussed at:
http://www.sourceware.org/ml/gdb-patches/2013-11/msg00412.html

~~~

This is to help make it slightly clearer how this method is expected
to extract data from the given register.

gdb/ChangeLog:

        * dwarf2expr.h (struct dwarf_expr_context_funcs)
        <read_addr_from_reg>: Renames "read_reg".
        * dwarf2-frame.c (read_addr_from_reg): Renames "read_reg".
        Adjust comment.
        (dwarf2_frame_ctx_funcs, execute_stack_op, dwarf2_frame_cache):
        Use read_addr_from_reg in place of read_reg.
        * dwarf2expr.c (execute_stack_op): Use read_addr_from_reg
        in place of read_reg.
        * dwarf2loc.c (dwarf_expr_read_addr_from_reg): Renames
        dwarf_expr_read_reg.
        (dwarf_expr_ctx_funcs): Replace dwarf_expr_read_reg
        with dwarf_expr_read_addr_from_reg.
        (needs_frame_read_addr_from_reg): Renames needs_frame_read_reg.
        (needs_frame_ctx_funcs): Replace needs_frame_read_reg with
        needs_frame_read_addr_from_reg.

Tested on x86_64-linux.  OK to commit?

Thank you,
-- 
Joel

---
 gdb/dwarf2-frame.c | 13 +++++++------
 gdb/dwarf2expr.c   | 10 ++++++----
 gdb/dwarf2expr.h   |  2 +-
 gdb/dwarf2loc.c    |  8 ++++----
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/gdb/dwarf2expr.h b/gdb/dwarf2expr.h
index b4c943e..c06c556 100644
--- a/gdb/dwarf2expr.h
+++ b/gdb/dwarf2expr.h
@@ -33,7 +33,7 @@ struct dwarf_expr_context_funcs
 {
   /* Return the value of register number REGNUM (a DWARF register number),
      read as an address.  */
-  CORE_ADDR (*read_reg) (void *baton, int regnum);
+  CORE_ADDR (*read_addr_from_reg) (void *baton, int regnum);
 
   /* Return a value of type TYPE, stored in register number REGNUM
      of the frame associated to the given BATON.
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b425d94..b53c015 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -286,7 +286,7 @@ dwarf2_frame_state_free (void *p)
 /* Helper functions for execute_stack_op.  */
 
 static CORE_ADDR
-read_reg (void *baton, int reg)
+read_addr_from_reg (void *baton, int reg)
 {
   struct frame_info *this_frame = (struct frame_info *) baton;
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
@@ -301,8 +301,8 @@ read_reg (void *baton, int reg)
   /* Convert the register to an integer.  This returns a LONGEST
      rather than a CORE_ADDR, but unpack_pointer does the same thing
      under the covers, and this makes more sense for non-pointer
-     registers.  Maybe read_reg and the associated interfaces should
-     deal with "struct value" instead of CORE_ADDR.  */
+     registers.  Maybe read_addr_from_reg and the associated interfaces
+     should deal with "struct value" instead of CORE_ADDR.  */
   return unpack_long (register_type (gdbarch, regnum), buf);
 }
 
@@ -358,7 +358,7 @@ register %s (#%d) at %s"),
 
 static const struct dwarf_expr_context_funcs dwarf2_frame_ctx_funcs =
 {
-  read_reg,
+  read_addr_from_reg,
   get_reg_value,
   read_mem,
   ctx_no_get_frame_base,
@@ -397,7 +397,8 @@ execute_stack_op (const gdb_byte *exp, ULONGEST len, int addr_size,
   if (ctx->location == DWARF_VALUE_MEMORY)
     result = dwarf_expr_fetch_address (ctx, 0);
   else if (ctx->location == DWARF_VALUE_REGISTER)
-    result = read_reg (this_frame, value_as_long (dwarf_expr_fetch (ctx, 0)));
+    result = read_addr_from_reg (this_frame,
+				 value_as_long (dwarf_expr_fetch (ctx, 0)));
   else
     {
       /* This is actually invalid DWARF, but if we ever do run across
@@ -1110,7 +1111,7 @@ dwarf2_frame_cache (struct frame_info *this_frame, void **this_cache)
       switch (fs->regs.cfa_how)
 	{
 	case CFA_REG_OFFSET:
-	  cache->cfa = read_reg (this_frame, fs->regs.cfa_reg);
+	  cache->cfa = read_addr_from_reg (this_frame, fs->regs.cfa_reg);
 	  if (fs->armcc_cfa_offsets_reversed)
 	    cache->cfa -= fs->regs.cfa_offset;
 	  else
diff --git a/gdb/dwarf2expr.c b/gdb/dwarf2expr.c
index 25e9dc4..69c08c2 100644
--- a/gdb/dwarf2expr.c
+++ b/gdb/dwarf2expr.c
@@ -921,7 +921,8 @@ execute_stack_op (struct dwarf_expr_context *ctx,
 	case DW_OP_breg31:
 	  {
 	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &offset);
-	    result = (ctx->funcs->read_reg) (ctx->baton, op - DW_OP_breg0);
+	    result = (ctx->funcs->read_addr_from_reg) (ctx->baton,
+						       op - DW_OP_breg0);
 	    result += offset;
 	    result_val = value_from_ulongest (address_type, result);
 	  }
@@ -930,7 +931,7 @@ execute_stack_op (struct dwarf_expr_context *ctx,
 	  {
 	    op_ptr = safe_read_uleb128 (op_ptr, op_end, &reg);
 	    op_ptr = safe_read_sleb128 (op_ptr, op_end, &offset);
-	    result = (ctx->funcs->read_reg) (ctx->baton, reg);
+	    result = (ctx->funcs->read_addr_from_reg) (ctx->baton, reg);
 	    result += offset;
 	    result_val = value_from_ulongest (address_type, result);
 	  }
@@ -955,8 +956,9 @@ execute_stack_op (struct dwarf_expr_context *ctx,
 	    if (ctx->location == DWARF_VALUE_MEMORY)
 	      result = dwarf_expr_fetch_address (ctx, 0);
 	    else if (ctx->location == DWARF_VALUE_REGISTER)
-	      result = (ctx->funcs->read_reg) (ctx->baton,
-				     value_as_long (dwarf_expr_fetch (ctx, 0)));
+	      result = (ctx->funcs->read_addr_from_reg)
+			  (ctx->baton,
+			   value_as_long (dwarf_expr_fetch (ctx, 0)));
 	    else
 	      error (_("Not implemented: computing frame "
 		       "base using explicit value operator"));
diff --git a/gdb/dwarf2loc.c b/gdb/dwarf2loc.c
index 2879ead..1664f6a 100644
--- a/gdb/dwarf2loc.c
+++ b/gdb/dwarf2loc.c
@@ -313,7 +313,7 @@ struct dwarf_expr_baton
 /* Using the frame specified in BATON, return the value of register
    REGNUM, treated as a pointer.  */
 static CORE_ADDR
-dwarf_expr_read_reg (void *baton, int dwarf_regnum)
+dwarf_expr_read_addr_from_reg (void *baton, int dwarf_regnum)
 {
   struct dwarf_expr_baton *debaton = (struct dwarf_expr_baton *) baton;
   struct gdbarch *gdbarch = get_frame_arch (debaton->frame);
@@ -2194,7 +2194,7 @@ static const struct lval_funcs pieced_value_funcs = {
 
 static const struct dwarf_expr_context_funcs dwarf_expr_ctx_funcs =
 {
-  dwarf_expr_read_reg,
+  dwarf_expr_read_addr_from_reg,
   dwarf_expr_get_reg_value,
   dwarf_expr_read_mem,
   dwarf_expr_frame_base,
@@ -2441,7 +2441,7 @@ struct needs_frame_baton
 
 /* Reads from registers do require a frame.  */
 static CORE_ADDR
-needs_frame_read_reg (void *baton, int regnum)
+needs_frame_read_addr_from_reg (void *baton, int regnum)
 {
   struct needs_frame_baton *nf_baton = baton;
 
@@ -2541,7 +2541,7 @@ needs_get_addr_index (void *baton, unsigned int index)
 
 static const struct dwarf_expr_context_funcs needs_frame_ctx_funcs =
 {
-  needs_frame_read_reg,
+  needs_frame_read_addr_from_reg,
   needs_frame_get_reg_value,
   needs_frame_read_mem,
   needs_frame_frame_base,
-- 
1.8.1.2

Re: [RFA] Rename "read_reg" into "read_addr_from_reg" in struct dwarf_expr_context_funcs

[Pedro Alves]

On 11/16/2013 03:50 AM, Joel Brobecker wrote:
> Hello,
> 
> This implements the suggestion discussed at:
> http://www.sourceware.org/ml/gdb-patches/2013-11/msg00412.html
> 
> ~~~
> 
> This is to help make it slightly clearer how this method is expected
> to extract data from the given register.
> 
> gdb/ChangeLog:
> 
>         * dwarf2expr.h (struct dwarf_expr_context_funcs)
>         <read_addr_from_reg>: Renames "read_reg".
>         * dwarf2-frame.c (read_addr_from_reg): Renames "read_reg".
>         Adjust comment.
>         (dwarf2_frame_ctx_funcs, execute_stack_op, dwarf2_frame_cache):
>         Use read_addr_from_reg in place of read_reg.
>         * dwarf2expr.c (execute_stack_op): Use read_addr_from_reg
>         in place of read_reg.
>         * dwarf2loc.c (dwarf_expr_read_addr_from_reg): Renames
>         dwarf_expr_read_reg.
>         (dwarf_expr_ctx_funcs): Replace dwarf_expr_read_reg
>         with dwarf_expr_read_addr_from_reg.
>         (needs_frame_read_addr_from_reg): Renames needs_frame_read_reg.
>         (needs_frame_ctx_funcs): Replace needs_frame_read_reg with
>         needs_frame_read_addr_from_reg.
> 
> Tested on x86_64-linux.  OK to commit?

Looks good to me.  Thanks!

-- 
Pedro Alves

pushed: [RFA] Rename "read_reg" into "read_addr_from_reg" in struct dwarf_expr_context_funcs

[Joel Brobecker]

> > gdb/ChangeLog:
> > 
> >         * dwarf2expr.h (struct dwarf_expr_context_funcs)
> >         <read_addr_from_reg>: Renames "read_reg".
> >         * dwarf2-frame.c (read_addr_from_reg): Renames "read_reg".
> >         Adjust comment.
> >         (dwarf2_frame_ctx_funcs, execute_stack_op, dwarf2_frame_cache):
> >         Use read_addr_from_reg in place of read_reg.
> >         * dwarf2expr.c (execute_stack_op): Use read_addr_from_reg
> >         in place of read_reg.
> >         * dwarf2loc.c (dwarf_expr_read_addr_from_reg): Renames
> >         dwarf_expr_read_reg.
> >         (dwarf_expr_ctx_funcs): Replace dwarf_expr_read_reg
> >         with dwarf_expr_read_addr_from_reg.
> >         (needs_frame_read_addr_from_reg): Renames needs_frame_read_reg.
> >         (needs_frame_ctx_funcs): Replace needs_frame_read_reg with
> >         needs_frame_read_addr_from_reg.
> > 
> > Tested on x86_64-linux.  OK to commit?
> 
> Looks good to me.  Thanks!

Thanks! Patch now pushed.

-- 
Joel

[OB] Simplify dwarf2-frame.c:read_addr_from_reg. (was: [RFA] Rename "read_reg" into "read_addr_from_reg" in struct dwarf_expr_context_funcs)

[Pedro Alves]

On 11/17/2013 03:02 AM, Joel Brobecker wrote:
> Thanks! Patch now pushed.

I've pushed this obvious patch too.

-------
Simplify dwarf2-frame.c:read_addr_from_reg.

Since 'struct dwarf_expr_context_funcs::read_addr_from_reg' is now
only used for addresses, we can make it use unpack_pointer.  And since
we now have 'struct dwarf_expr_context_funcs'::get_reg_value, there's
no need for speculation about using values here.

Tested on x86_64 Fedora 17.

gdb/
2013-11-18  Pedro Alves  <palves@redhat.com>

	* dwarf2-frame.c (read_addr_from_reg): Remove stale comment and
	use unpack_pointer.
---

 gdb/ChangeLog      |    5 +++++
 gdb/dwarf2-frame.c |    7 +------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 53de4fb..7fc09e0 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,8 @@
+2013-11-18  Pedro Alves  <palves@redhat.com>
+
+	* dwarf2-frame.c (read_addr_from_reg): Remove stale comment and
+	use unpack_pointer.
+
 2013-11-18  Joel Brobecker  <brobecker@adacore.com>
 
 	* mi/mi-main.c (mi_cmd_list_features): Add "language-options"
diff --git a/gdb/dwarf2-frame.c b/gdb/dwarf2-frame.c
index b53c015..cd4f47c 100644
--- a/gdb/dwarf2-frame.c
+++ b/gdb/dwarf2-frame.c
@@ -298,12 +298,7 @@ read_addr_from_reg (void *baton, int reg)
   buf = alloca (register_size (gdbarch, regnum));
   get_frame_register (this_frame, regnum, buf);
 
-  /* Convert the register to an integer.  This returns a LONGEST
-     rather than a CORE_ADDR, but unpack_pointer does the same thing
-     under the covers, and this makes more sense for non-pointer
-     registers.  Maybe read_addr_from_reg and the associated interfaces
-     should deal with "struct value" instead of CORE_ADDR.  */
-  return unpack_long (register_type (gdbarch, regnum), buf);
+  return unpack_pointer (register_type (gdbarch, regnum), buf);
 }
 
 /* Implement struct dwarf_expr_context_funcs' "get_reg_value" callback.  */