From patchwork Fri Jan 27 21:13:33 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 63817 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id DD0513858C1F for ; Fri, 27 Jan 2023 21:14:01 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DD0513858C1F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1674854041; bh=30jSqBajK3iyjfEt7RWQP05PNU4WNoFWAL2UdcfqM2Q=; h=Date:Subject:To:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=xXLQkRJB/rkiJ/2FZa6L05lFKfJ08/laUj5b24nF4dhXfuJTra7uLj/8EdAacpQhK b3LdW7u40cGszw8L7F8g5E+Bg2Qyy4czGYnjk4/e5UcOvQhwbOAVWaHEosy+QIILli Btcu4cZK+UQV8dQH5F0euYMeflNuEPfFyzBWhuhg= X-Original-To: gdb-patches@sourceware.org Delivered-To: gdb-patches@sourceware.org Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 6BF6D385840E for ; Fri, 27 Jan 2023 21:13:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6BF6D385840E Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id A563B21FD5; Fri, 27 Jan 2023 21:13:33 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 8C5C1138E3; Fri, 27 Jan 2023 21:13:33 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id D2sgIX0+1GMyCwAAMHmgww (envelope-from ); Fri, 27 Jan 2023 21:13:33 +0000 Message-ID: <5d5eeb5d-bc39-c01f-53e8-d961ed0b98ea@suse.de> Date: Fri, 27 Jan 2023 22:13:33 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.1 Subject: [PATCH] [gdb/tdep] Assume epilogue unwind info is valid unless gcc < 4.5.0 Content-Language: en-US To: Tom Tromey , Tom de Vries via Gdb-patches References: <20230121074807.22032-1-tdevries@suse.de> <87a62blqxn.fsf@tromey.com> In-Reply-To: <87a62blqxn.fsf@tromey.com> X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_NUMSUBJECT, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org 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: , X-Patchwork-Original-From: Tom de Vries via Gdb-patches From: Tom de Vries Reply-To: Tom de Vries Errors-To: gdb-patches-bounces+patchwork=sourceware.org@sourceware.org Sender: "Gdb-patches" On 1/21/23 18:48, Tom Tromey wrote: >>>>>> "Tom" == Tom de Vries via Gdb-patches writes: > > Tom> Fix these two issues by reversing the burden of proof: > Tom> - currently we assume epilogue unwind info is invalid unless we can proof that > Tom> gcc >= 4.5.0. > Tom> - instead, assume epilogue unwind info is valid unless we can proof that > Tom> gcc < 4.5.0. > > FWIW this approach makes sense to me. > OK, then changing RFC -> PATCH. > It's pretty lame that there's no way to detect this failure from the > frame section -- it can't be producer-sniffed and the augmentation > strings can't really be changed. > > gcc 4.5 was released in 2010, and so it's not like we're inconveniencing > a lot of users. If needed I guess we could add a user setting to switch > this behavior back on. > > Note there is a similar issue for the prologue, see: > > https://sourceware.org/bugzilla/show_bug.cgi?id=25696 > https://sourceware.org/bugzilla/show_bug.cgi?id=17265 > https://sourceware.org/bugzilla/show_bug.cgi?id=21470 > > Also worth seeing the hilarious: > > https://github.com/rust-lang/rust/issues/41252#issuecomment-293676579 > > I think that in this area we should assume the debug info is correct, > and keep a list of known-bad producers rather than assuming the debug > info is wrong and having a list of known-good ones. > > Tom> + if (/* In absence of producer information, optimistically assume that we're > Tom> + not dealing with gcc < 4.5.0. */ > > This placement of the comment is pretty weird, it seems fine to just > stick it before the 'if'. > Done. > Tom> + if (cu->producer == nullptr) > Tom> + /* In absence of producer information, optimistically assume that we're > Tom> + not dealing with gcc < 4.5.0. */ > Tom> + cust->set_epilogue_unwind_valid (true); > Tom> + if (!producer_is_gcc (cu->producer, nullptr, nullptr)) > > Normally if there is a comment and a line of code as the consequence of > an 'if', we put them both in a block. > Done. > Anyway I was also thinking that the second one should say 'else if'. True, thanks for catching that, also done. I've also added a test-case, for the amd64-tdep.c change. I could make another one for the i386-tdep.c change, and/or one for the dwarf/read.c change, but I'm not sure that's worth the trouble. Thanks, - Tom From cac1a96a28c761543cc33fce09c068713c6d6c96 Mon Sep 17 00:00:00 2001 From: Tom de Vries Date: Fri, 20 Jan 2023 18:19:38 +0100 Subject: [PATCH] [gdb/tdep] Assume epilogue unwind info is valid unless gcc < 4.5.0 The gcc 4.4.x (and earlier) compilers had the problem that the unwind info in the epilogue was inaccurate. In order to work around this in gdb, epilogue unwinders were added with a higher priority than the dwarf unwinders in the amd64 and i386 targets: - amd64_epilogue_frame_unwind, and - i386_epilogue_frame_unwind see: - submission emails: https://sourceware.org/pipermail/gdb-patches/2009-July/066779.html https://sourceware.org/pipermail/gdb-patches/2009-August/067684.html - gdb commits 872761f485e and 06da04c6105 Subsequently, the epilogue unwind info problem got fixed in gcc 4.5.0, see: - submission email https://gcc.gnu.org/pipermail/gcc-patches/2009-May/261377.html - gcc commit cd9c1ca866b - release notes gcc 4.5.0 ( https://gcc.gnu.org/gcc-4.5/changes.html ): GCC now generates unwind info also for epilogues. However, the epilogue unwinders prevented gdb from taking advantage of the fixed epilogue unwind info, so the scope of the epilogue unwinders was limited, bailing out for gcc >= 4.5.0, see: - submisssion email https://sourceware.org/pipermail/gdb-patches/2011-June/083429.html - gdb commit e0d00bc749e "Disable epilogue unwinders on recent GCCs" This scope limitation mechanism works well for gcc -g: the producer is available in the debug info, and we can determine whether we're dealing with reliable epilogue unwind info or not. For gcc -g0 though, epilogue unwind information is available in .eh_frame, but the producer is not availabe to determine whether that information is reliable or not, and consequently the info is ignored: - in the case of using gcc <= 4.4.x, that is the ok decision and we're working around the gcc problem, but - in the case of gcc >= 4.5.0, that means we fail to take advantage of fixed epilogue unwind info. Furthermore, let's review the history of what epilogue unwind information is trusted by gdb: - initial position: trust all information - after the epilogue unwinders were added: trust none - after the scope limitation: only trust gcc >= 4.5.0. So, while we started out with trusting info from all compilers, we end up trusting only gcc >= 4.5.0, which seems a bit of an overreach for a workaround for a problem in the gcc compiler. Fix these two issues by reversing the burden of proof: - currently we assume epilogue unwind info is invalid unless we can proof that gcc >= 4.5.0. - instead, assume epilogue unwind info is valid unless we can proof that gcc < 4.5.0. An added benefit of this is that it makes the amd64 and i386 targets more similar to other targets, which makes comparing behaviour easier. Note that some other targets also have an epilogue unwinder, but none of those have a higher priority than the dwarf unwinders. Tested on x86_64-linux with gcc 7.5.0, with target boards unix/-m64 and unix/-m32. The patch contains 3 changes, in amd64-tdep.c, i386-tdep.c and dwarf/read.c. A regression test-case was added for the amd64-tdep.c part. PR tdep/30028 Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30028 --- gdb/amd64-tdep.c | 4 +- gdb/dwarf2/read.c | 12 ++++- gdb/i386-tdep.c | 4 +- .../gdb.base/unwind-on-each-insn-amd64-2.exp | 52 ++++++++++++++++++ .../gdb.base/unwind-on-each-insn-amd64-2.s | 54 +++++++++++++++++++ 5 files changed, 123 insertions(+), 3 deletions(-) create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c index 70d0d0f3318..1132af2e88c 100644 --- a/gdb/amd64-tdep.c +++ b/gdb/amd64-tdep.c @@ -2906,7 +2906,9 @@ amd64_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) struct compunit_symtab *cust; cust = find_pc_compunit_symtab (pc); - if (cust != NULL && cust->epilogue_unwind_valid ()) + /* In absence of producer information, optimistically assume that we're + not dealing with gcc < 4.5.0, and have valid epilogue unwind info. */ + if (cust == NULL || cust->epilogue_unwind_valid ()) return 0; if (target_read_memory (pc, &insn, 1)) diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index cd937f24ee7..bc6ca06573a 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -8484,7 +8484,17 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language) if (cu->has_loclist && gcc_4_minor >= 5) cust->set_locations_valid (true); - if (gcc_4_minor >= 5) + if (cu->producer == nullptr) + { + /* In absence of producer information, optimistically assume that + we're not dealing with gcc < 4.5.0. */ + cust->set_epilogue_unwind_valid (true); + } + else if (!producer_is_gcc (cu->producer, nullptr, nullptr)) + /* Not gcc. */ + cust->set_epilogue_unwind_valid (true); + else if (gcc_4_minor >= 5) + /* gcc >= 4.5.0. */ cust->set_epilogue_unwind_valid (true); cust->set_call_site_htab (cu->call_site_htab); diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c index 580664d2ce5..708370be114 100644 --- a/gdb/i386-tdep.c +++ b/gdb/i386-tdep.c @@ -2222,7 +2222,9 @@ i386_stack_frame_destroyed_p (struct gdbarch *gdbarch, CORE_ADDR pc) struct compunit_symtab *cust; cust = find_pc_compunit_symtab (pc); - if (cust != NULL && cust->epilogue_unwind_valid ()) + /* In absence of producer information, optimistically assume that we're + not dealing with gcc < 4.5.0, and have valid epilogue unwind info. */ + if (cust == NULL || cust->epilogue_unwind_valid ()) return 0; if (target_read_memory (pc, &insn, 1)) diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp new file mode 100644 index 00000000000..c8a94ccef4f --- /dev/null +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.exp @@ -0,0 +1,52 @@ +# Copyright 2023 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 . */ + +# Check that epilogue unwind info is used, even if no debug info is available. + +require is_x86_64_m64_target + +set srcfile_flags {debug} +set srcfile2_flags {nodebug} + +if [info exists COMPILE] { + # Make sure that we use .eh_frame info, by generating it + # using -fasynchronous-unwind-tables. + if { [gdb_can_simple_compile fasynchronous-unwind-tables \ + { void foo () { } } object -fasynchronous-unwind-tables] } { + lappend srcfile2_flags additional_flags=-fasynchronous-unwind-tables + } else { + unsupported "required: .eh_frame" + return + } + standard_testfile unwind-on-each-insn.c unwind-on-each-insn-foo.c + # When updating the .s file, use these flags to generate the file: + #lappend srcfile2_flags additional_flags=-save-temps + #lappend srcfile2_flags additional_flags=-dA + # and do the following: + # - copy it in place, run the test-case and verify that all tests pass. + # - confuse the amd64 epilogue unwinder by inserting the following + # in foo: + # nop + # + pushq $.L1 + # + ret + # + .L1: + # + nop + # popq %rbp + # - verify that the test-case passes. +} else { + standard_testfile unwind-on-each-insn.c .s +} + +source $srcdir/$subdir/unwind-on-each-insn.exp.tcl diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s new file mode 100644 index 00000000000..c141f71817c --- /dev/null +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn-amd64-2.s @@ -0,0 +1,54 @@ + .file "unwind-on-each-insn-foo.c" + .text + .globl foo + .type foo, @function +foo: +.LFB0: + .cfi_startproc +# BLOCK 2 seq:0 +# PRED: ENTRY (FALLTHRU) + pushq %rbp + .cfi_def_cfa_offset 16 + .cfi_offset 6, -16 + movq %rsp, %rbp + .cfi_def_cfa_register 6 + movq %rdi, -8(%rbp) + nop + pushq $.L1 + ret +.L1: + nop + popq %rbp + .cfi_def_cfa 7, 8 +# SUCC: EXIT [100.0%] + ret + .cfi_endproc +.LFE0: + .size foo, .-foo + .globl bar + .type bar, @function +bar: +.LFB1: + .cfi_startproc +# BLOCK 2 seq:0 +# PRED: ENTRY (FALLTHRU) + pushq %rbp + .cfi_def_cfa_offset 16 + .cfi_offset 6, -16 + movq %rsp, %rbp + .cfi_def_cfa_register 6 + subq $8, %rsp + movq %rdi, -8(%rbp) + movq -8(%rbp), %rax + movq %rax, %rdi + call foo + nop + leave + .cfi_def_cfa 7, 8 +# SUCC: EXIT [100.0%] + ret + .cfi_endproc +.LFE1: + .size bar, .-bar + .ident "GCC: (SUSE Linux) 7.5.0" + .section .note.GNU-stack,"",@progbits base-commit: d9195131530370f56e78a2b084928c11da536d9e -- 2.35.3