From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id OmDXJNmhY2Qg2AsAWB0awg (envelope-from ) for ; Tue, 16 May 2023 11:31:37 -0400 Received: by simark.ca (Postfix, from userid 112) id 8965D1E11E; Tue, 16 May 2023 11:31:37 -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=HMbFbaPE; 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=-9.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,NICE_REPLY_A, RCVD_IN_DNSWL_HI,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 14CAF1E0D6 for ; Tue, 16 May 2023 11:31:37 -0400 (EDT) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A567A3857005 for ; Tue, 16 May 2023 15:31:35 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A567A3857005 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1684251095; bh=CHCjKvOadi6B5ewm1f5H3noWH9gJ4yxTOSpd4AwW3WM=; h=Date:Subject:To:Cc:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=HMbFbaPEwL569OTtbHdGRtnKpLJDTQ9cKdVkzgumWV93vvo7rBDL4yd0Nvrif3zNP 6d9BY6btAJ+kLQpssqopX1wG+c8yH0N392xFp1Rf9s2NlAN8Q07TWehvI1GrZJCe6z +C9uZUxjLpWrRmTWtPuD62+ouecPYgxQsgjdUtuI= Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id CBA923858C83 for ; Tue, 16 May 2023 15:31:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CBA923858C83 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 F128E21D9B; Tue, 16 May 2023 15:31:14 +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 CEE22138F5; Tue, 16 May 2023 15:31:14 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id t4ddMcKhY2SpCQAAMHmgww (envelope-from ); Tue, 16 May 2023 15:31:14 +0000 Message-ID: Date: Tue, 16 May 2023 17:31:14 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [PATCH v3] gdb: Fix false match issue in skip_prologue_using_linetable Content-Language: en-US To: Luis Machado , Kevin Buettner , Tom de Vries via Gdb-patches Cc: WANG Rui References: <20230418120939.29102-1-tdevries@suse.de> <1a4148e1-1376-3bc6-e398-4c611922c58f@suse.de> <20230421110346.7ca9b163@f37-zws-nv> <276f25d7-ab32-268f-0875-70cab6180e8d@suse.de> <6663707c-4297-c2f2-a0bd-f3e84fc62aad@suse.de> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Tom de Vries via Gdb-patches Reply-To: Tom de Vries Errors-To: gdb-patches-bounces+public-inbox=simark.ca@sourceware.org Sender: "Gdb-patches" On 5/16/23 16:19, Luis Machado wrote: > On 4/24/23 15:15, Tom de Vries wrote: >> On 4/24/23 14:53, Luis Machado wrote: >>> On 4/22/23 09:01, Tom de Vries via Gdb-patches wrote: >>>> On 4/21/23 20:03, Kevin Buettner wrote: >>>>> Hi Tom, >>>>> >>>>> On Tue, 18 Apr 2023 14:15:06 +0200 >>>>> Tom de Vries via Gdb-patches wrote: >>>>> >>>>>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote: >>>>>>> Co-Authored-By: WANG Rui (fix, tiny change [1]) >>>>>>> Co-Authored-By: Tom de Vries (test-case) >>>>>>> >>>>>>> [1] >>>>>>> https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html >>>>>> >>>>>> I'm not used to deal with these matters, so I'd appreciate some >>>>>> review/approval on this.  Is my copyright status assessment >>>>>> correct, and >>>>>> did I write it up correctly? >>>>> >>>>> I refreshed my memory via the link you provided above.  Based on what >>>>> is written there, I conclude that Wang Rui's change is not legally >>>>> signficant for copyright purposes. >>>>> >>>>> Also, I've looked over the Rui's patch as well as your test case, and >>>>> it looks good to me.  So... >>>>> >>>>> Approved-by: Kevin Buettner >>>>> >>>> >>>> Hi Kevin, >>>> >>>> Thanks for review. >>>> >>>> Committed and also backported to gdb-13-branch, because it was a 12 >>>> -> 13 regression. >>>> >>>> Thanks, >>>> - Tom >>>> >>> >>> For some reason aarch64 is grumpy with this test, and it FAIL's the >>> last comparison. >>> >>> Maybe aarch64 is broken in this regard? >> >> Hi Luis, >> >> thanks for reporting this. >> >> I could reproduce it on openSUSE Leap 15.4. >> >> I think there are two independent problems: >> - the aarch64 prologue analyzer walks past the end of the function >> - the test-case assumes that the prologue analyzer will return the first >>    insn in foo, rather that some insn in foo. >> >> The WIP patch below addresses both issues, and allows the test-case to >> pass for me. >> >> [ FWIW, alternatively using some "maint set skip-prologue" value from >> this RFC ( >> https://sourceware.org/pipermail/gdb-patches/2022-August/191343.html ) >> could also suffice to ignore the first problem. ] >> >> Thanks, >> - Tom >> >> ... >> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c >> index ec0e51bdaf7..d974595e48f 100644 >> --- a/gdb/aarch64-tdep.c >> +++ b/gdb/aarch64-tdep.c >> @@ -917,12 +917,13 @@ aarch64_analyze_prologue_test (void) >>   static CORE_ADDR >>   aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc) >>   { >> -  CORE_ADDR func_addr, limit_pc; >> +  CORE_ADDR func_addr, func_end_addr, limit_pc; >> >>     /* See if we can determine the end of the prologue via the symbol >>        table.  If so, then return either PC, or the PC after the >>        prologue, whichever is greater.  */ >> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL)) >> +  bool func_addr_found = find_pc_partial_function (pc, NULL, >> &func_addr, &func_end_addr); >> +  if (func_addr_found) >>       { >>         CORE_ADDR post_prologue_pc >>          = skip_prologue_using_sal (gdbarch, func_addr); >> @@ -941,7 +942,8 @@ aarch64_skip_prologue (struct gdbarch *gdbarch, >> CORE_ADDR pc) >>     limit_pc = skip_prologue_using_sal (gdbarch, pc); >>     if (limit_pc == 0) >>       limit_pc = pc + 128;       /* Magic.  */ >> - >> +  limit_pc = std::min (limit_pc, func_end_addr - 4); >> + >>     /* Try disassembling prologue.  */ >>     return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL); >>   } >> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp >> b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp >> index 488f85f9674..c506cfd55cc 100644 >> --- a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp >> +++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp >> @@ -95,15 +95,15 @@ if { $break_addr == "" } { >> >>   # Get the "foo_label" address. >> >> -set foo_label_addr "" >> -gdb_test_multiple "print /x &foo_label" "" { >> +set bar_label_addr "" >> +gdb_test_multiple "print /x &bar_label" "" { >>       -re -wrap "= ($hex)" { >> -       set foo_label_addr $expect_out(1,string) >> +       set bar_label_addr $expect_out(1,string) >>          pass $gdb_test_name >>       } >>   } >> >> -if { $foo_label_addr == "" } { >> +if { $bar_label_addr == "" } { >>       return >>   } >> >> @@ -115,4 +115,4 @@ gdb_test "print &foo_end == &bar_label" " = 1" >>   # Check that the breakpoint is set at the expected address. >> Regression test >>   # for PR30369. >> >> -gdb_assert { $break_addr == $foo_label_addr } >> +gdb_assert { $break_addr < $bar_label_addr } >> ... > > Sorry, I thought I had replied to this thread. Indeed the above patch > addresses this problem with the aarch64 prologue skipper, > and it also fixes things for arm. For arm I suspect we might need the > same fix to the prologue skipper that the patch addresses for aarch64. > > I can pick it up, refresh and submit if you're happy with it as well. Hi Luis, thanks for confirming. If you want to pick this up, great, thanks. - Tom