Re: [PATCH] Always print call-clobbered registers in outer frames.

[Pedro Alves <palves@redhat.com>]

On 09/20/2013 01:27 PM, Mark Kettenis wrote:

> I strongly disagree with this change.  

Ack.

My thought process was that it was not that much different from
defining the value of PC in frames other than #0 to be the
return address, not the value the PC had _before_ the call.
If we went further and defined all register values in that
direction, things would all be consistent.  Something like:

  The values of registers in the outer frame, are defined as
  being the values the registers would have if the inner frame
  was to return immediately, respecting relevant ABI rules.

IOW, call-clobbered registers would show whatever value's
current in the inner frame, and callee-saved registers
would show the saved values.

I think _both_ definitions are reasonable, though this
one seemed a little more consistent to me.

> I think it is useful to show that rgeisters have not been saved.

Right, but that could also be done in some other way.  I
toyed with something like Doug suggested.  That is, make
"info reg" indicate whether the register was actually saved
or GDB assumed same_value.  Something like:

(gdb) i reg
rax            0x1 1		[not saved]
rbx            0x0 0
rcx            0x2 2		[not saved]
rdx            0x3 3		[not saved]
rsi            0x4 4		[not saved]
rdi            0x5 5		[not saved]
rbp            0x7fffffffda70   0x7fffffffda70
rsp            0x7fffffffda40   0x7fffffffda40

So GDB would still "flatten" the registers, and "p $rax"
would still print some number, and the user could tell
that the registers' values might no longer be the
values the frame had when it called out the inner frame.

>  If you want to look at their
> value anyway, you can always go down (or up) the stack and chase the
> value of the register yourself.

Of course.  Though not very convenient.  If that's the direction
we're heading, and if GDB gets the saved/not-saved wrong, that
may be a nuisance.  It might be useful to have shorthand for
that (though admittedly the use cases will probably trigger so
seldom that we may never hear requests for it). 

> 
> I also think it doesn't make sense for the varobj subsystem to
> reimplement bits of the unwinder subsystem.

You mean value machinery, I guess.  I wouldn't say reimplement.
The idea was to still leave the unwinder treating not-saved as is,
and only hide that at the user-facing presentation layer, so that
anything that might need to rely on the unwinder machinery getting
info about registers at inner frame call time, wouldn't get possibly
no-longer-correct register values unless it wanted to.  (IOW, we could
move the value_of_register function to stack.c and call it part of
the unwinder machinery as well.)

> If newer GCC versions are even less explicit about saving or not
> saving registers than older versions, we should probably improve the
> _dwarf2_frame_init_reg() implementations to indicate properly which
> registers are caller-saved and which registers are callee-saved.

So...  I tried that out for AMD64, on top of the <not-saved> patch.

(top-gdb) i reg
rax            0x5e60ff 6185215
rbx            0x0      0
rcx            0x323d435bf0     215776189424
rdx            0x7fffffffda50   140737488345680
rsi            0x0      0
rdi            0x7fffffffda50   140737488345680
rbp            0x7fffffffd9a0   0x7fffffffd9a0
rsp            0x7fffffffd860   0x7fffffffd860
r8             0x70     112
r9             0x100000 1048576
r10            0x8      8
r11            0x246    582
r12            0x45aed0 4566736
r13            0x7fffffffdb50   140737488345936
r14            0x0      0
r15            0x0      0
rip            0x5e6114 0x5e6114 <captured_main+21>
eflags         0x206    [ PF IF ]
cs             0x33     51
ss             0x2b     43
ds             0x0      0
es             0x0      0
fs             0x0      0
gs             0x0      0
(top-gdb) up
#1  0x00000000005e3876 in catch_errors (func=0x5e60ff <captured_main>, func_args=0x7fffffffda50, errstring=0x8a6194 "", mask=RETURN_MASK_ALL)
    at ../../src/gdb/exceptions.c:524
524           val = func (func_args);
(top-gdb) i reg
rax            <not saved>
rbx            0x0      0
rcx            <not saved>
rdx            <not saved>
rsi            <not saved>
rdi            <not saved>
rbp            0x7fffffffda10   0x7fffffffda10
rsp            0x7fffffffd9b0   0x7fffffffd9b0
r8             <not saved>
r9             <not saved>
r10            <not saved>
r11            <not saved>
r12            0x45aed0 4566736
r13            0x7fffffffdb50   140737488345936
r14            0x0      0
r15            0x0      0
rip            0x5e3876 0x5e3876 <catch_errors+95>
eflags         <not saved>
cs             <not saved>
ss             <not saved>
ds             <not saved>
es             <not saved>
fs             <not saved>
gs             <not saved>


The patch also teaches the heuristic unwinder about call-clobbered
registers, though I wonder whether we should be assuming much about the ABI
around assembly or no-debug-info functions.  Hmm.

Argh, this thread is getting quite confusing with the back and forth.
Sorry about that.

---
 gdb/amd64-tdep.c                               | 54 +++++++++++++++++++++++++-
 gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp | 31 +++++++++++++--
 gdb/testsuite/gdb.mi/mi-reg-undefined.exp      | 43 ++++++++++++++++----
 3 files changed, 115 insertions(+), 13 deletions(-)

diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
index 3ab74f0..3ab6058 100644
--- a/gdb/amd64-tdep.c
+++ b/gdb/amd64-tdep.c
@@ -40,6 +40,7 @@
 #include "exceptions.h"
 #include "amd64-tdep.h"
 #include "i387-tdep.h"
+#include "dwarf2-frame.h"
 
 #include "features/i386/amd64.c"
 #include "features/i386/amd64-avx.c"
@@ -1744,6 +1745,28 @@ amd64_alloc_frame_cache (void)
   return cache;
 }
 
+/* Return true if the ABI specifies REGNUM as a callee-saved
+   register.  */
+
+static int
+amd64_register_callee_saved (int regnum)
+{
+  if (regnum == AMD64_RBP_REGNUM
+      || regnum == AMD64_RBX_REGNUM
+      || (AMD64_R12_REGNUM <= regnum && regnum <= AMD64_R15_REGNUM))
+    return 1;
+
+  /* The control bits of the MXCSR register are callee-saved.  */
+  if (regnum == AMD64_MXCSR_REGNUM)
+    return 1;
+
+  /* The x87 control word is callee-saved.  */
+  if (regnum == AMD64_FCTRL_REGNUM)
+    return 1;
+
+  return 0;
+}
+
 /* GCC 4.4 and later, can put code in the prologue to realign the
    stack pointer.  Check whether PC points to such code, and update
    CACHE accordingly.  Return the first instruction after the code
@@ -2423,7 +2446,10 @@ amd64_frame_prev_register (struct frame_info *this_frame, void **this_cache,
     return frame_unwind_got_memory (this_frame, regnum,
 				    cache->saved_regs[regnum]);
 
-  return frame_unwind_got_register (this_frame, regnum, regnum);
+  if (amd64_register_callee_saved (regnum))
+    return frame_unwind_got_register (this_frame, regnum, regnum);
+  else
+    return frame_unwind_got_optimized (this_frame, regnum);
 }
 
 static const struct frame_unwind amd64_frame_unwind =
@@ -2846,6 +2872,30 @@ static const int amd64_record_regmap[] =
   AMD64_DS_REGNUM, AMD64_ES_REGNUM, AMD64_FS_REGNUM, AMD64_GS_REGNUM
 };
 
+/* Implement the "init_reg" dwarf2_frame_ops method.  */
+
+static void
+amd64_dwarf2_frame_init_reg (struct gdbarch *gdbarch, int regnum,
+			     struct dwarf2_frame_state_reg *reg,
+			     struct frame_info *this_frame)
+{
+  /* Mark the PC as the destination for the return address.  */
+  if (regnum == gdbarch_pc_regnum (gdbarch))
+    reg->how = DWARF2_FRAME_REG_RA;
+
+  /* Mark the stack pointer as the call frame address.  */
+  else if (regnum == gdbarch_sp_regnum (gdbarch))
+    reg->how = DWARF2_FRAME_REG_CFA;
+
+  /* The above was taken from the default init_reg in dwarf2-frame.c
+     while the below is AMD64 specific.  */
+
+  else if (amd64_register_callee_saved (regnum))
+    reg->how = DWARF2_FRAME_REG_SAME_VALUE;
+  else
+    reg->how = DWARF2_FRAME_REG_UNDEFINED;
+}
+
 void
 amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -2948,6 +2998,8 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   frame_unwind_append_unwinder (gdbarch, &amd64_frame_unwind);
   frame_base_set_default (gdbarch, &amd64_frame_base);
 
+  dwarf2_frame_set_init_reg (gdbarch, amd64_dwarf2_frame_init_reg);
+
   /* If we have a register mapping, enable the generic core file support.  */
   if (tdep->gregset_reg_offset)
     set_gdbarch_regset_from_core_section (gdbarch,
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
index 44bf8e9..c11b007 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
@@ -44,11 +44,24 @@ for {set f 0} {$f < 3} {incr f} {
 	set pattern_rax_rbx_rcx_info "$hex\\s+$decimal"
 	set pattern_r8_r9_print "$hex"
 	set pattern_r8_r9_info "$hex\\s+$decimal"
+	set pattern_r12_r15_print "$hex"
+	set pattern_r12_r15_info "$hex\\s+$decimal"
     } else {
+	# Even though rbx is callee-saved, these are all explicitly
+	# marked undefined in the debug info.
 	set pattern_rax_rbx_rcx_print "<not saved>"
 	set pattern_rax_rbx_rcx_info "<not saved>"
-	set pattern_r8_r9_print "$hex"
-	set pattern_r8_r9_info "$hex\\s+$decimal"
+
+	# ABI specifies these as caller-saved.  GDB should be
+	# implicitly aware they're not saved, even if the debug info
+	# doesn't explicitly say so (and also doesn't say they were
+	# actually saved).
+	set pattern_r8_r9_print "<not saved>"
+	set pattern_r8_r9_info "<not saved>"
+
+	# ABI specifies these as callee-saved.
+	set pattern_r12_r15_print "$hex"
+	set pattern_r12_r15_info "$hex\\s+$decimal"
     }
 
     # Select frame.
@@ -64,8 +77,18 @@ for {set f 0} {$f < 3} {incr f} {
     gdb_test "p/x \$r8" "$pattern_r8_r9_print" "print \$r8 in frame ${f}"
     gdb_test "p/x \$r9" "$pattern_r8_r9_print" "print \$r9 in frame ${f}"
 
+    gdb_test "p/x \$r12" "$pattern_r12_r15_print" "print \$r12 in frame ${f}"
+    gdb_test "p/x \$r15" "$pattern_r12_r15_print" "print \$r15 in frame ${f}"
 
     # Display register values.
-    gdb_test "info registers rax rbx rcx r8 r9" "rax\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nrbx\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nrcx\\s+${pattern_rax_rbx_rcx_info}\\s*\r\nr8\\s+${pattern_r8_r9_info}\\s*\r\nr9\\s+${pattern_r8_r9_info}\\s*" \
-	"Check values of rax, rbx, rcx, r8, r9 in frame ${f}"
+    send_gdb "info registers rax rbx rcx r8 r9 r12 r15\n"
+    gdb_expect_list "info registers in frame ${f}" "$gdb_prompt $" \
+	[list \
+	     "rax\\s+$pattern_rax_rbx_rcx_info\r\n" \
+	     "rbx\\s+$pattern_rax_rbx_rcx_info\r\n" \
+	     "rcx\\s+$pattern_rax_rbx_rcx_info\r\n" \
+	     "r8\\s+$pattern_r8_r9_info\r\n" \
+	     "r9\\s+$pattern_r8_r9_info\r\n" \
+	     "r12\\s+$pattern_r12_r15_info\r\n" \
+	     "r15\\s+$pattern_r12_r15_info\r\n" ]
 }
diff --git a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
index cb3a716..64fa581 100644
--- a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
+++ b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
@@ -52,20 +52,47 @@ mi_gdb_test "111-stack-list-frames" \
     "111\\^done,stack=\\\[frame=\{level=\"0\",addr=\"$hex\",func=\"stop_frame\",.*\},frame=\{level=\"1\",addr=\"$hex\",func=\"first_frame\",.*\},frame=\{level=\"2\",addr=\"$hex\",func=\"main\",.*\}\\\]" \
     "stack frame listing"
 
-set not_saved_pattern "<not saved>"
-
 for {set f 0} {$f < 3} {incr f} {
     if {${f} == 0} {
-	set pattern_0_1_2 ${hex}
+	set pattern ${hex}
     } else {
-	set pattern_0_1_2 ${not_saved_pattern}
+	set pattern "<not saved>"
     }
 
-    mi_gdb_test "22${f}-data-list-register-values --thread 1 --frame ${f} x  0 1 2 8 9" \
-	"22${f}\\^done,register-values=\\\[\{number=\"0\",value=\"${pattern_0_1_2}\"\},\{number=\"1\",value=\"${pattern_0_1_2}\"\},\{number=\"2\",value=\"${pattern_0_1_2}\"\},\{number=\"8\",value=\"$hex\"\},\{number=\"9\",value=\"$hex\"\}\\\]" \
+    # - rax,rbx,rcx are explicitly marked undefined in the debug info,
+    #   so GDB should display them all as not saved, even though rbx
+    #   is callee-saved.
+    #
+    # - r8-r9 are caller-saved.  GDB should be implicitly aware
+    #   they're not saved, even if the debug info doesn't explicitly
+    #   say so (and also doesn't say they were actually saved).
+    #
+    # - r12-r15 are callee-saved.  GDB should be able to find their
+    #   values.
+
+    mi_gdb_test "22${f}-data-list-register-values --thread 1 --frame ${f} x  0 1 2 8 9 12 15" \
+	[join [list \
+		   "22${f}\\^done,register-values=\\\["\
+		   "\{number=\"0\",value=\"${pattern}\"\}," \
+		   "\{number=\"1\",value=\"${pattern}\"\}," \
+		   "\{number=\"2\",value=\"${pattern}\"\}," \
+		   "\{number=\"8\",value=\"${pattern}\"\}," \
+		   "\{number=\"9\",value=\"${pattern}\"\}," \
+		   "\{number=\"12\",value=\"$hex\"\}," \
+		   "\{number=\"15\",value=\"$hex\"\}" \
+		   "\\\]" ] "" ] \
 	"register values, format x, frame ${f}"
 
-    mi_gdb_test "33${f}-data-list-register-values --thread 1 --frame ${f} r 0 1 2 8 9" \
-	"33${f}\\^done,register-values=\\\[\{number=\"0\",value=\"${pattern_0_1_2}\"\},\{number=\"1\",value=\"${pattern_0_1_2}\"\},\{number=\"2\",value=\"${pattern_0_1_2}\"\},\{number=\"8\",value=\"$hex\"\},\{number=\"9\",value=\"$hex\"\}\\\]" \
+    mi_gdb_test "33${f}-data-list-register-values --thread 1 --frame ${f} r  0 1 2 8 9 12 15" \
+	[join [list \
+		   "33${f}\\^done,register-values=\\\["\
+		   "\{number=\"0\",value=\"${pattern}\"\}," \
+		   "\{number=\"1\",value=\"${pattern}\"\}," \
+		   "\{number=\"2\",value=\"${pattern}\"\}," \
+		   "\{number=\"8\",value=\"${pattern}\"\}," \
+		   "\{number=\"9\",value=\"${pattern}\"\}," \
+		   "\{number=\"12\",value=\"$hex\"\}," \
+		   "\{number=\"15\",value=\"$hex\"\}" \
+		   "\\\]" ] "" ] \
 	"register values, format r, frame ${f}"
 }
-- 
1.7.11.7