[PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder
Andrew Burgess
aburgess@redhat.com
Wed Aug 24 13:42:15 GMT 2022
Andrew Burgess <aburgess@redhat.com> writes:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> When GDB is stopped at a ret instruction and no debug information is
>> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
>> be able to generate a decent backtrace. However, when calculating the
>> frame id, the epilogue unwinder generates information as if the return
>> instruction was the whole frame.
>>
>> This was an issue especially when attempting to reverse debug, as GDB
>> would place a step_resume_breakpoint from the epilogue of a function if
>> we were to attempt to skip that function, and this breakpoint should
>> ideally have the current function's frame_id to avoid other problems
>> such as PR record/16678.
>>
>> This commit changes the frame_id calculation for the amd64 epilogue,
>> so that it is always the same as the dwarf2 unwinder's frame_id.
>> ---
>> gdb/amd64-tdep.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index d89e06d27cb..17c82ac919c 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -2943,7 +2943,7 @@ amd64_epilogue_frame_cache (struct frame_info *this_frame, void **this_cache)
>> byte_order) + cache->sp_offset;
>>
>> /* Cache pc will be the frame func. */
>> - cache->pc = get_frame_pc (this_frame);
>> + cache->pc = get_frame_func (this_frame);
>>
>> /* The saved %esp will be at cache->base plus 16. */
>> cache->saved_sp = cache->base + 16;
>
> This change is fine, though I wonder if you would mind updating the
> comments in this function. These comments have clearly been copied over
> from the i386 code, and still refer to %esp instead of %rsp, etc.
>
> Additionally, this comment:
>
> /* The saved %esp will be at cache->base plus 16. */
>
> I believe is completely wrong. The previous %rsp value is not _at_
> anything, instead:
>
> /* The previous value of %rsp is cache->base plus 16. */
>
> This change of wording aligns with similar wording in
> amd64_frame_cache_1 where the saved_sp is calculated.
>
>> @@ -2986,7 +2986,7 @@ amd64_epilogue_frame_this_id (struct frame_info *this_frame,
>> if (!cache->base_p)
>> (*this_id) = frame_id_build_unavailable_stack (cache->pc);
>> else
>> - (*this_id) = frame_id_build (cache->base + 8, cache->pc);
>> + (*this_id) = frame_id_build (cache->saved_sp, cache->pc);
>> }
>
> I honestly don't understand the logic behind how cache->base and
> cache->sp_offset are used for the x86-64 prologue based unwinders.
>
> The sp_offset starts out as -8, which makes sense, the call instruction
> will have pushed an 8-byte address to the stack.
>
> Personally, at this point, I would think that:
>
> cache->base = current_sp - cache->sp_offset;
>
> would be the correct way to calculate cache->base. This would set the
> cache->base to be the frame base address. However, we actually
> calculate the cache->base as:
>
> cache->base = current_sp + cache->sp_offset;
>
> notice the '+' instead of the '-' I proposed. As a consequence, the
> frame base address ends up being:
>
> cache->base + 16
>
> which is exactly what we use in amd64_frame_this_id and
> amd64_sigtramp_frame_this_id.
>
> Maybe I'm missing some super subtle logic here for why we do things the
> way that we do. But honestly, my guess is that, at some point, a logic
> bug was introduced (using '+' instead of '-'), and the rest of the code
> has just grown to work around that problem, e.g. the '+ 16' when
> calculating the frame base address.
>
> Now, in your change, you propose using saved_sp instead of '+ 8'.
>
> I'm thinking that we would be better off just using 'cache->base + 16'
> here instead. My reasoning would be:
>
> 1. A '+ 16' here is inline with the other frame_id function on x86-64,
> which is probably a good idea, and
>
> 2. Though we can see that the frame base address _is_ the same as the
> previous stack pointer value, they don't have to be. For me at least,
> these two things are different concepts, and just because the actual
> value is the same, doesn't mean we should use the two variables
> interchangably. So, I'd prefer to see cache->base used like we do be
> the other frame_id functions. Who knows, one day maybe we could even
> change things so the '+ 16' is not needed, and cache->base will
> contain the "correct" value ... but not today I think.
>
> Finally, on this topic, I took a look at i386_epilogue_frame_this_id to
> see if the same bug was present there, and I notice that code uses '+ 8'
> just like the amd64 code does, but in that case addresses are only
> 4-bytes, and indeed, the initial sp_offset is -4, so '+ 8' here (i386)
> is really '+ 2 * sizeof(void*)', which means there's no i386 bug, but I
> think this another indication that using '+16' in the amd64 code is the
> way to go.
>
> Finally, I wondered if it is worth adding a test that covers
> specifically this functionality, without relying on the gdb.reverse test
> you mention - as that test need futher fixes before it can exercise this
> code. I've included a test at the end of this email which I believe
> covers this functionality, if you agree this would be worthwhile, then
> feel free to include this test, or to use any parts of this test that
> are helpful in writing your own test.
>
> Thanks,
> Andrew
>
> --
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> new file mode 100644
> index 00000000000..e5e5ebfd013
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
> @@ -0,0 +1,22 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2022 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/>. */
> +
> +void
> +foo ()
> +{
> + /* Nothing. */
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.c b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> new file mode 100644
> index 00000000000..3e4cc2675f2
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.c
> @@ -0,0 +1,25 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2022 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/>. */
> +
> +extern void foo ();
> +
> +int
> +main ()
> +{
> + foo ();
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> new file mode 100644
> index 00000000000..46bea800c1b
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -0,0 +1,154 @@
> +# Copyright 2022 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/>. */
> +
> +# Single step through a simple (empty) function that was compiled
> +# without DWARF debug information.
> +#
> +# At each instruction check that the frame-id, and frame base address,
> +# are calculated correctly.
> +#
> +# Additionally, check we can correctly unwind to the previous frame,
> +# and that the previous stack-pointer value, and frame base address
> +# value, can be calculated correctly.
> +
> +standard_testfile .c -foo.c
> +
> +if {[prepare_for_testing_full "failed to prepare" \
> + [list ${testfile} debug \
> + $srcfile {debug} $srcfile2 {nodebug}]]} {
> + return -1
> +}
> +
> +if ![runto_main] then {
> + return 0
> +}
> +
> +# Return a two element list, the first element is the stack-pointer
> +# value (from the $sp register), and the second element is the frame
> +# base address (from the 'info frame' output).
> +proc get_sp_and_fba { testname } {
> + with_test_prefix "get \$sp and frame base $testname" {
> + set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> +
> + set fba ""
> + gdb_test_multiple "info frame" "" {
> + -re "^info frame\r\n" {
> + exp_continue
> + }
> +
> + -re "^Stack level ${::decimal}, frame at ($::hex):\r\n.*$::gdb_prompt $" {
> + set fba $expect_out(1,string)
> + }
> + }
> +
> + return [list $sp $fba]
> + }
> +}
> +
> +# Return the frame-id of the current frame, collected using the 'maint
> +# print frame-id' command.
> +proc get_fid { } {
> + set fid ""
> + gdb_test_multiple "maint print frame-id" "" {
> + -re "^maint print frame-id\r\n" {
> + exp_continue
> + }
> +
> + -re "^frame-id for frame #${::decimal}: (\[^\r\n\]+).*" {
> + set fid $expect_out(1,string)
> + }
> + }
> + return $fid
> +}
I remembered that this test relies on my 'maint print frame-id' patch.
I think I'll go and merge that now.
Thanks,
Andrew
> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in main"] main_sp main_fba
> +set main_fid [get_fid]
> +
> +# Now enter the foo function.
> +gdb_breakpoint "*foo"
> +gdb_continue_to_breakpoint "enter foo"
> +
> +# Figure out the range of addresses covered by this function.
> +set last_addr_in_foo ""
> +gdb_test_multiple "disassemble foo" "" {
> + -re "^disassemble foo\r\n" {
> + exp_continue
> + }
> +
> + -re "^Dump of assembler code for function foo:\r\n" {
> + exp_continue
> + }
> +
> + -re "^...($hex) \[^\r\n\]+\r\n" {
> + set last_addr_in_foo $expect_out(1,string)
> + exp_continue
> + }
> +
> + -wrap -re "^End of assembler dump\\." {
> + gdb_assert { ![string equal $last_addr_in_foo ""] } \
> + "found some addresses in foo"
> + pass $gdb_test_name
> + }
> +}
> +
> +# Record the current stack-pointer, and the frame base address.
> +lassign [get_sp_and_fba "in foo"] foo_sp foo_fba
> +set foo_fid [get_fid]
> +
> +for { set i_count 1 } { true } { incr i_count } {
> + with_test_prefix "instruction ${i_count}" {
> +
> + # The current stack-pointer value can legitimately change
> + # throughout the lifetime of a function, so we don't check the
> + # current stack-pointer value. But the frame base address
> + # should not change, so we do check for that.
> + lassign [get_sp_and_fba "for foo"] sp_value fba_value
> + gdb_assert { $fba_value == $foo_fba }
> +
> + # The frame-id should never change within a function, so check
> + # that now.
> + set fid [get_fid]
> + gdb_assert { [string equal $fid $foo_fid] } \
> + "check frame-id matches"
> +
> + # Check that the previous frame is 'main'.
> + gdb_test "bt 2" "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> + # Move up the stack (to main).
> + gdb_test "up" \
> + "\r\n#1\\s+\[^\r\n\]+ in main \\(\\) .*"
> +
> + # Check we can unwind the stack-pointer and the frame base
> + # address correctly.
> + lassign [get_sp_and_fba "for main"] sp_value fba_value
> + gdb_assert { $sp_value == $main_sp }
> + gdb_assert { $fba_value == $main_fba }
> +
> + # Check we have a consistent value for main's frame-id.
> + set fid [get_fid]
> + gdb_assert { [string equal $fid $main_fid] }
> +
> + # Move back to the inner most frame.
> + gdb_test "frame 0" ".*"
> +
> + set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
> + if { $pc == $last_addr_in_foo } {
> + break
> + }
> +
> + gdb_test "stepi" ".*"
> + }
> +}
More information about the Gdb-patches
mailing list