gdb: Implement the init_reg dwarf2_frame_ops for amd64
Andrew Burgess
aburgess@redhat.com
Thu Dec 2 15:56:55 GMT 2021
* Djordje Todorovic <Djordje.Todorovic@syrmia.com> [2021-11-18 12:42:32 +0000]:
> Hi Andrew,
>
> Please find the patch, rebased on top of your improvement.
>
> Thanks,
> Djordje
>
> ---
> gdb/amd64-tdep.c | 37 +++++++++++++
> gdb/frame.c | 9 +++-
> .../gdb.arch/amd64-invalid-stack-middle.exp | 8 +--
> gdb/testsuite/gdb.arch/amd64-not-saved-regs.c | 28 ++++++++++
> .../gdb.arch/amd64-not-saved-regs.exp | 52 +++++++++++++++++++
> gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp | 4 +-
> .../gdb.dwarf2/dw2-reg-undefined.exp | 8 +--
> gdb/testsuite/gdb.mi/mi-reg-undefined.exp | 4 +-
> 8 files changed, 136 insertions(+), 14 deletions(-)
> create mode 100644 gdb/testsuite/gdb.arch/amd64-not-saved-regs.c
> create mode 100644 gdb/testsuite/gdb.arch/amd64-not-saved-regs.exp
> ?
> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
> index 7c67359678b..a6b544145fb 100644
> --- a/gdb/amd64-tdep.c
> +++ b/gdb/amd64-tdep.c
> @@ -25,6 +25,8 @@
> #include "arch-utils.h"
> #include "block.h"
> #include "dummy-frame.h"
> +#include "dwarf2.h"
> +#include "dwarf2/frame.h"
> #include "frame.h"
> #include "frame-base.h"
> #include "frame-unwind.h"
> @@ -3103,6 +3105,38 @@ amd64_in_indirect_branch_thunk (struct gdbarch *gdbarch, CORE_ADDR pc)
> AMD64_RIP_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)
> +{
> + switch (regnum)
> + {
> + case AMD64_RIP_REGNUM:
> + reg->how = DWARF2_FRAME_REG_FN;
> + return;
> +
> + case AMD64_RSP_REGNUM:
> + reg->how = DWARF2_FRAME_REG_CFA;
> + return;
> +
> + /* Caller-saved registers. */
> + case AMD64_RAX_REGNUM:
> + case AMD64_RDI_REGNUM:
> + case AMD64_RSI_REGNUM:
> + case AMD64_RDX_REGNUM:
> + case AMD64_RCX_REGNUM:
> + case AMD64_R8_REGNUM:
> + case AMD64_R9_REGNUM:
> + case AMD64_R10_REGNUM:
> + case AMD64_R11_REGNUM:
> + reg->how = DWARF2_FRAME_REG_UNDEFINED;
> + return;
> + }
> +}
I believe that this is the System-V ABI, which is not the only ABI
used on x86-64. From what I've read I believe that Windows uses a
slightly different ABI, where $rsi and $rdi are not callee saved.
I think that we might consider moving this function from the general
amd64-tdep.c to something like amd64-linux-tdep.c, and register this
in the function amd64_linux_init_abi.
Of course, this will mean that other System-V like targets would miss
out for now, but maybe the function itself could be renamed to
something like amd64_system_v_dwarf2_frame_init_reg, and left in
amd64-tdep.c, then from amd64-linux-tdep.c we can register that
function. And in future, other system-v like targets can also
register the function.
> +
> void
> amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
> const target_desc *default_tdesc)
> @@ -3217,6 +3251,9 @@ amd64_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
> set_gdbarch_stab_reg_to_regnum (gdbarch, amd64_dwarf_reg_to_regnum);
> set_gdbarch_dwarf2_reg_to_regnum (gdbarch, amd64_dwarf_reg_to_regnum);
>
> + /* Frame handling. */
> + dwarf2_frame_set_init_reg (gdbarch, amd64_dwarf2_frame_init_reg);
> +
> /* We don't override SDB_REG_RO_REGNUM, since COFF doesn't seem to
> be in use on any of the supported AMD64 targets. */
>
> diff --git a/gdb/frame.c b/gdb/frame.c
> index 2a899fc494f..da12ed36e02 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -2315,9 +2315,14 @@ get_prev_frame_always (struct frame_info *this_frame)
> }
> catch (const gdb_exception_error &ex)
> {
> - if (ex.error == MEMORY_ERROR)
> + if (ex.error == MEMORY_ERROR || ex.error == OPTIMIZED_OUT_ERROR)
> {
> - this_frame->stop_reason = UNWIND_MEMORY_ERROR;
> + if (ex.error == MEMORY_ERROR)
> + this_frame->stop_reason = UNWIND_MEMORY_ERROR;
> + else
> + /* This is for the OPTIMIZED_OUT_ERROR case. */
> + this_frame->stop_reason = UNWIND_UNAVAILABLE;
> +
> if (ex.message != NULL)
> {
> char *stop_string;
> diff --git a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
> index f9e590d83bb..6ae1c9c1322 100644
> --- a/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
> +++ b/gdb/testsuite/gdb.arch/amd64-invalid-stack-middle.exp
> @@ -42,11 +42,11 @@ if ![runto breakpt] {
> return -1
> }
>
> -gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
> - "first backtrace, with error message"
> +gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: value has been optimized out" \
> + "first backtrace, with error message"
I think the new "value has been optimized out" message is better than
what we had previously ("Cannot access memory at address 0x....")
which I guess was caused by trying to access through an unavailable
register.
That said, I remember a long time ago there was a whole big discussion
about whether registers that were not saved should be described as
"optimized out" or not. In the end we added the "<not saved>" message
to cover this case.
So I wonder if we should instead be ending the backtrace with
something like: "Backtrace stopped: value is not available"? I took
that exact text from require_available (value.c) but anything that
talks about available rather than optimized out might be better.
Though I don't know how hard it would be to achieve that result. I
guess we'd need to start in dwarf2_frame_prev_register where we map
DWARF2_FRAME_REG_UNDEFINED onto optimized_out - maybe that needs to
map to an unavailable value instead, but that feels like it might have
huge knock on consequences ... we don't want optimized out variables
to suddenly start reporting themselves as unavailable instead...
I don't know if others have any opinions on this, but it might be
worth having a short investigation to see how hard such a change would
be, I guess if it turns out to be a real pain then we could punt it
for later.
>
> -gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: Cannot access memory at address 0x\[0-9a-f\]+" \
> - "second backtrace, with error message"
> +gdb_test "bt" "^bt\r\n#0 +breakpt *\\(\\) \[^\r\n\]*\r\n#1 +0x\[0-9a-f\]+ in func5\[^\r\n\]*\r\n#2 +0x\[0-9a-f\]+ in func4\[^\r\n\]*\r\n#3 +0x\[0-9a-f\]+ in func3\[^\r\n\]*\r\nBacktrace stopped: value has been optimized out" \
> + "second backtrace, with error message"
>
> clean_restart ${binfile}
>
> diff --git a/gdb/testsuite/gdb.arch/amd64-not-saved-regs.c b/gdb/testsuite/gdb.arch/amd64-not-saved-regs.c
> new file mode 100644
> index 00000000000..5698e549f00
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-not-saved-regs.c
> @@ -0,0 +1,28 @@
> +/* This test program is part of GDB, the GNU debugger.
> +
> + Copyright 2019-2021 Free Software Foundation, Inc.
Should that copyright range just be 2021?
> +
> + 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/>. */
> +
> +int
> +foo (int a)
> +{
> + return a; /* break here. */
> +}
> +
> +int
> +main ()
> +{
> + return foo (5);
> +}
> diff --git a/gdb/testsuite/gdb.arch/amd64-not-saved-regs.exp b/gdb/testsuite/gdb.arch/amd64-not-saved-regs.exp
> new file mode 100644
> index 00000000000..07b870b5d30
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/amd64-not-saved-regs.exp
> @@ -0,0 +1,52 @@
> +# Copyright 2010-2021 Free Software Foundation, Inc.
I suspect the copyright range here is wrong too.
For new files you should only have a from date in the copyright range
if the content of the file is almost completely copied unchanged from
some other, older, file. If the content in the new file is mostly
new, then you should just place this years date as the range:
# Copyright 2021 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/>.
> +
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org
> +
> +# This file is part of the gdb testsuite.
> +
> +if { ![istarget x86_64-*-* ]} {
> + verbose "Skipping ${gdb_test_file_name}."
> + return
> +}
> +
> +# Build program with debug symbols.
> +standard_testfile
> +set compile_flags {debug}
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} ${compile_flags}] } {
> + return -1
> +}
> +
> +if ![runto_main] {
> + untested "could not run to main"
> + return -1
> +}
> +
> +gdb_breakpoint [ gdb_get_line_number "break here" ]
> +gdb_continue_to_breakpoint "break here" ".*break here.*"
> +
> +# Switch to frame 1.
> +gdb_test "frame 1" "#1.*in main ().*"
> +
> +gdb_test "info registers rax" "rax .* <not saved>"
> +gdb_test "info registers rcx" "rcx .* <not saved>"
> +gdb_test "info registers rdx" "rdx .* <not saved>"
> +gdb_test "info registers rsi" "rsi .* <not saved>"
> +gdb_test "info registers rdi" "rdi .* <not saved>"
> +gdb_test "info registers r8" "r8 .* <not saved>"
> +gdb_test "info registers r9" "r9 .* <not saved>"
> +gdb_test "info registers r10" "r10 .* <not saved>"
> +gdb_test "info registers r11" "r11 .* <not saved>"
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp b/gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp
> index 238830e711e..70f22cb9efe 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-op-out-param.exp
> @@ -50,11 +50,11 @@ gdb_test "bt" "#0 ($hex in )?breakpt \\(\\)\r\n#1 ($hex in )?int_param_single_
>
> # (2) struct_param_single_reg_loc
> gdb_continue_to_breakpoint "Stop in breakpt for struct_param_single_reg_loc"
> -gdb_test "bt" "#0 ($hex in )?breakpt \\(\\)\r\n#1 ($hex in )?struct_param_single_reg_loc \\(operand0={a = 0xdeadbe00deadbe01, b = <optimized out>}, operand1={a = <optimized out>, b = 0xdeadbe04deadbe05}, operand2=<optimized out>\\)\r\n#2 ($hex in )?main \\(\\)" "backtrace for test struct_param_single_reg_loc"
> +gdb_test "bt" "#0 ($hex in )?breakpt \\(\\)\r\n#1 ($hex in )?struct_param_single_reg_loc \\(operand0={a = 0xdeadbe00deadbe01, b = <optimized out>}, operand1=<optimized out>, operand2=<optimized out>\\)\r\n#2 ($hex in )?main \\(\\)" "backtrace for test struct_param_single_reg_loc"
>
> # (3) struct_param_two_reg_pieces
> gdb_continue_to_breakpoint "Stop in breakpt for struct_param_two_reg_pieces"
> -gdb_test "bt" "#0 ($hex in )?breakpt \\(\\)\r\n#1 ($hex in )?struct_param_two_reg_pieces \\(operand0={a = 0xdeadbe04deadbe05, b = <optimized out>}, operand1={a = <optimized out>, b = 0xdeadbe00deadbe01}, operand2=<optimized out>\\)\r\n#2 ($hex in )?main \\(\\)" "backtrace for test struct_param_two_reg_pieces"
> +gdb_test "bt" "#0 ($hex in )?breakpt \\(\\)\r\n#1 ($hex in )?struct_param_two_reg_pieces \\(operand0=<optimized out>, operand1={a = <optimized out>, b = 0xdeadbe00deadbe01}, operand2=<optimized out>\\)\r\n#2 ($hex in )?main \\(\\)" "backtrace for test struct_param_two_reg_pieces"
I'm worried that this test has lost some of its meaning with this
change. What I think we need to do is modify the .S file that
accompanies this test to mark all of the registers that the ABI marks
as undefined, to DW_CFA_same_value. If you're not comfortable doing
that I'm happy to help once the other issues have been addressed.
>
> # (4) int_param_two_reg_pieces
> gdb_continue_to_breakpoint "Stop in breakpt for int_param_two_reg_pieces"
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> index b1c28b2f41c..2b296c8d445 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-reg-undefined.exp
> @@ -47,8 +47,8 @@ for {set f 0} {$f < 3} {incr f} {
> } else {
> 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"
> + set pattern_r8_r9_print "<not saved>"
> + set pattern_r8_r9_info "<not saved>"
> }
Again, I think we've lost something here. The original choice of
r8/r9 was arbitrary, and, at the time didn't make any difference as
all the registers were treated as "same value".
I'd suggest that you add testing of r12/r13 here as they should show
the same behaviour that r8/r9 did before the patch.
>
> # Select frame.
> @@ -79,8 +79,8 @@ for {set f 0} {$f < 3} {incr f} {
> # "not saved" and not "optimized out".
> gdb_test "set debug frame 1"
> gdb_test {print $rax} [multi_line \
> - { \[frame\] frame_unwind_register_value: frame=0, regnum=0\(rax\)} \
> - { \[frame\] frame_unwind_register_value: -> <not saved>} \
> + { \[frame\] frame_unwind_register_value: frame=1, regnum=0\(rax\)} \
> + { \[frame\] frame_unwind_register_value: -> <not saved>} \
As above, this change is fine for rax, but you should add a test for
rbx too, that register should behave just as rax used too before your
patch.
> {.*}]
> gdb_test "set debug frame 0"
>
> diff --git a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
> index f38e20003a3..3f193b1379b 100644
> --- a/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
> +++ b/gdb/testsuite/gdb.mi/mi-reg-undefined.exp
> @@ -62,10 +62,10 @@ for {set f 0} {$f < 3} {incr f} {
> }
>
> 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\"\}\\\]" \
> + "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=\"${pattern_0_1_2}\"\},\{number=\"9\",value=\"${pattern_0_1_2}\"\}\\\]" \
> "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\"\}\\\]" \
> + "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=\"${pattern_0_1_2}\"\},\{number=\"9\",value=\"${pattern_0_1_2}\"\}\\\]" \
> "register values, format r, frame ${f}"
Like the above test, I think you need to extend what's being tested
here to cover registers that are similar to the old behaviour, I'd
suggest r12/r13 again.
Also, you should probably rename pattern_0_1_2 as this was referring
to register 0, 1, and 2, but now is being used more widely.
Thanks,
Andrew
More information about the Gdb-patches
mailing list