gdb: Implement the init_reg dwarf2_frame_ops for amd64
Djordje
djordje.todorovic@syrmia.com
Fri Dec 3 16:27:21 GMT 2021
Hi Andrew,
Thanks for your suggestions (some comments inline)!
Best,
Djordje
On 2.12.21. 16:56, Andrew Burgess wrote:
> * 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.
I'd vote for this, thanks (also, John Baldwin agreed on this -- it is in
a separate mail).
>> +
>> 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.
I am with you there. Unfortunately, I won't be able to work on that now.
>
>> -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?
Yep, thanks!
>
>> +
>> + 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.
I am not very familiar with incorporating that info from asm level, so
any help will be appreciated.
>> # (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.
Oh yes. I've updated it automatically.
>> # 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.
Thanks!
>> {.*}]
>> 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.
Sure, thanks!
> Thanks,
> Andrew
>
More information about the Gdb-patches
mailing list