Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

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" ".*"
> +    }
> +}