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

Pedro Alves
2013-09-24T12:06:52Z

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