From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id vkKQMY4jBmNakywAWB0awg (envelope-from ) for ; Wed, 24 Aug 2022 09:11:42 -0400 Received: by simark.ca (Postfix, from userid 112) id BC5911E4A7; Wed, 24 Aug 2022 09:11:42 -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=WNvRA4AP; 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=-3.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 Received: from sourceware.org (server2.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 B75241E13B for ; Wed, 24 Aug 2022 09:11:41 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id EBB54385E00A for ; Wed, 24 Aug 2022 13:11:40 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EBB54385E00A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1661346701; bh=WeuVc9jI9Z4vv4WFoz4TJc7lScDbLHEtUJY8JmjSMMc=; 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=WNvRA4AP+qtpjPrW72xMUcuRI3FOeK8UXvaMI6/+/lqMuwwl/o6YtFYlurfgitcC2 Vvs7TPhKZ+jOsYoaDngR3oQBZ/HQiK9lPy3Tu+RR47tQTaCYdMXJVq0XSEW9OHr7ab MjKewc4NaUa6wfCP1N/GN4OJTDOyxUTOlfBL/K+A= Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 75DA23858C33 for ; Wed, 24 Aug 2022 13:11:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 75DA23858C33 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-543-zc_tj_zRNdCXhqAMQtI2sA-1; Wed, 24 Aug 2022 09:11:14 -0400 X-MC-Unique: zc_tj_zRNdCXhqAMQtI2sA-1 Received: by mail-wm1-f69.google.com with SMTP id m22-20020a7bca56000000b003a652939bd1so428350wml.9 for ; Wed, 24 Aug 2022 06:11:13 -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=WeuVc9jI9Z4vv4WFoz4TJc7lScDbLHEtUJY8JmjSMMc=; b=FH1DqfrJFLcWEIEz+sTiGxJUMiQi7dS4bXaueR+90tCUxIXQmpFY0wiapPfBHNyKwp PcaWN3wBg/sUUlvyKlWUnqFIV8+iP5OiFrBgBLhjLLan1zUkKYv0ly3QFGM/3lY7KmMT FWcgdqgZ70YqmF5IXPJA08LzP4GGXnb9dxOBn10BZ3bcXD02wN+gT3HMRhHcXDHu2UvY gtrzuf6cGSw5mVzQQeZRZKhlj/44/VG9qV9nBS9a3foMszcKy3SJdFCpJsqB8n3tVrY8 Jaw04hbQ7HpOzTozUiFs9Rvsi9t5uQ67bJNA4NAfWv+e6+t7M85megd+jahiBWXAUcQD Jqpg== X-Gm-Message-State: ACgBeo1e6l5JLKzChTdZFSFYgGvBVdS4yJo2fvytQk8u3LH6drMRhIWl +K60keWmj9b9/S9dWvPOx1cHdL6+lNdmM9/9bLGxPVMS1VZsp1pIXLXBTMdyG2NG67k8JDDV3++ cDOkm7UnvCn73lYcEkhth1w== X-Received: by 2002:adf:eb4a:0:b0:223:9164:b5f6 with SMTP id u10-20020adfeb4a000000b002239164b5f6mr16909916wrn.113.1661346672282; Wed, 24 Aug 2022 06:11:12 -0700 (PDT) X-Google-Smtp-Source: AA6agR5ittwfuurlYVl1txJjtWu+Bczn84+cHIoBBEAFnt3l/0LhpIzoaX+XTkVvTcdHiedvOgL+9Q== X-Received: by 2002:adf:eb4a:0:b0:223:9164:b5f6 with SMTP id u10-20020adfeb4a000000b002239164b5f6mr16909897wrn.113.1661346671846; Wed, 24 Aug 2022 06:11:11 -0700 (PDT) Received: from localhost ([31.111.84.229]) by smtp.gmail.com with ESMTPSA id o13-20020a05600c510d00b003a4efb794d7sm2401978wms.36.2022.08.24.06.11.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Aug 2022 06:11:11 -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: <20220823142204.31659-2-blarsen@redhat.com> References: <20220823142204.31659-1-blarsen@redhat.com> <20220823142204.31659-2-blarsen@redhat.com> Date: Wed, 24 Aug 2022 14:11:10 +0100 Message-ID: <87k06xeqhd.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" 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 +} + +# 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" ".*" + } +}