From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id oZlhLNUqBmO+lSwAWB0awg (envelope-from ) for ; Wed, 24 Aug 2022 09:42:45 -0400 Received: by simark.ca (Postfix, from userid 112) id AC68E1E4A7; Wed, 24 Aug 2022 09:42:45 -0400 (EDT) Authentication-Results: simark.ca; dkim=pass (1024-bit key; secure) header.d=sourceware.org header.i=@sourceware.org header.a=rsa-sha256 header.s=default header.b=qWNDHeJ1; dkim-atps=neutral X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,RDNS_DYNAMIC,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id A19AA1E13B for ; Wed, 24 Aug 2022 09:42:44 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id AC0E3385E00A for ; Wed, 24 Aug 2022 13:42:41 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org AC0E3385E00A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1661348561; bh=euvtTUzmfwLm5d8wf8ejqT6KW5nHc/21tihn6k1zheQ=; h=To:Subject:In-Reply-To:References:Date:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=qWNDHeJ1CE8z1djrE6LLJAUNml4z97vFQArLbdfawqLbVekZNtxNrBTLq7SVwm7xA zIZAgDZaeTWOFPXMYUhXeLeUwlZoik0duxSOn5vA4CHKtUXie/XdvYNmAprI0kBLrQ 4t7LFDCA+IcmfA1uw9PCyB/TO8lLaWI/l7daUHWI= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 0E7933858C33 for ; Wed, 24 Aug 2022 13:42:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0E7933858C33 Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-356-mhZSfWtGOreEoLU5Ivj0jA-1; Wed, 24 Aug 2022 09:42:18 -0400 X-MC-Unique: mhZSfWtGOreEoLU5Ivj0jA-1 Received: by mail-wr1-f70.google.com with SMTP id d11-20020adfc08b000000b002207555c1f6so2767584wrf.7 for ; Wed, 24 Aug 2022 06:42:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:date:references:in-reply-to:subject:cc:to :from:x-gm-message-state:from:to:cc; bh=euvtTUzmfwLm5d8wf8ejqT6KW5nHc/21tihn6k1zheQ=; b=WxXxutJ4pEJI4LrC1j7ichM8XHc1L6sdIVuaNAD1D8Sh6L3HgK3J9pN5LbEQem0oNG 7wvs0B6GYl2Y0tVuHW6Gt5Icma43nfReu6sDua6gjc92eSDi4y3bWEVYSO2PAy6d0I3H eyqZ/uPtPsvpXEe5mgHlbYACoTCSmVHpbhn7BlmMdb2TqTCxdsijV5WcX2purcY28WnA o4Xrl5HYULbfTgZ1KqLqmXtI3WqAYQ3IyeCTxCIn4kGalGlZ/40Lv+X8fQHbBEBdPlCe 34uqZSmUB+I1/vuEVBGOSK4bDcubzn+xLvRz1T65vD0u7/JkXacPDk46uKCoCEltFkS/ 8Wxg== X-Gm-Message-State: ACgBeo0Hg8pgmmQtKP2eN21Ou1LZsHZaW7CuICbBXbfN5qFNJOXvRZEK CwBGVEln6uILeTsZaArRbG3yEQfFZMsOUNul/FR56MniX4E8Z1DDOECMKNFpqyLt6/O+Gkaxi9b 4tL1X8JezA1QITYa7+NygaA== X-Received: by 2002:a05:600c:4ece:b0:3a6:28:bc59 with SMTP id g14-20020a05600c4ece00b003a60028bc59mr5288371wmq.154.1661348537314; Wed, 24 Aug 2022 06:42:17 -0700 (PDT) X-Google-Smtp-Source: AA6agR6pLEBfP6S93o1btFTir4v8vJKImtwcLbqBjWhrhW1eUwqWP6Jcd9pWXv0EbT9dEPwJFEnVCg== X-Received: by 2002:a05:600c:4ece:b0:3a6:28:bc59 with SMTP id g14-20020a05600c4ece00b003a60028bc59mr5288349wmq.154.1661348536944; Wed, 24 Aug 2022 06:42:16 -0700 (PDT) Received: from localhost ([31.111.84.229]) by smtp.gmail.com with ESMTPSA id d1-20020adfe881000000b0022063e5228bsm17398369wrm.93.2022.08.24.06.42.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Aug 2022 06:42:16 -0700 (PDT) To: Bruno Larsen , gdb-patches@sourceware.org Subject: Re: [PATCH v2 1/2] Change calculation of frame_id by amd64 epilogue unwinder In-Reply-To: <87k06xeqhd.fsf@redhat.com> References: <20220823142204.31659-1-blarsen@redhat.com> <20220823142204.31659-2-blarsen@redhat.com> <87k06xeqhd.fsf@redhat.com> Date: Wed, 24 Aug 2022 14:42:15 +0100 Message-ID: <87edx5ep1k.fsf@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Andrew Burgess via Gdb-patches Reply-To: Andrew Burgess Cc: pedro@palves.net Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" Andrew Burgess writes: > Bruno Larsen via Gdb-patches 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 . */ > + > +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 . */ > + > +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 . */ > + > +# 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" ".*" > + } > +}