From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 43480 invoked by alias); 21 Jun 2019 14:26:42 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 43157 invoked by uid 89); 21 Jun 2019 14:26:42 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.4 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_STOCKGEN,RCVD_IN_DNSWL_NONE,T_FILL_THIS_FORM_SHORT autolearn=ham version=3.3.1 spammy=2150, IIUC, iiuc, limitations X-HELO: mail-wr1-f65.google.com Received: from mail-wr1-f65.google.com (HELO mail-wr1-f65.google.com) (209.85.221.65) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 21 Jun 2019 14:26:40 +0000 Received: by mail-wr1-f65.google.com with SMTP id p11so6779610wre.7 for ; Fri, 21 Jun 2019 07:26:40 -0700 (PDT) Return-Path: Received: from ?IPv6:2001:8a0:f913:f700:4c97:6d52:2cea:997b? ([2001:8a0:f913:f700:4c97:6d52:2cea:997b]) by smtp.gmail.com with ESMTPSA id v18sm3454250wrd.51.2019.06.21.07.26.37 (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Fri, 21 Jun 2019 07:26:37 -0700 (PDT) Subject: Re: [PATCH 1/4] Prefer symtab symbol over minsym for function names in non-contiguous blocks To: Kevin Buettner , gdb-patches@sourceware.org References: <20190608195434.26512-1-kevinb@redhat.com> <20190608195434.26512-2-kevinb@redhat.com> From: Pedro Alves Message-ID: <8f624a5d-58ba-8c6b-064d-9a82a63a6769@redhat.com> Date: Fri, 21 Jun 2019 14:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <20190608195434.26512-2-kevinb@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2019-06/txt/msg00423.txt.bz2 On 6/8/19 8:54 PM, Kevin Buettner wrote: > The discussion on gdb-patches which led to this patch may be found > here: > > https://www.sourceware.org/ml/gdb-patches/2019-05/msg00018.html > > Here's a brief synopsis/analysis: > > Eli Zaretskii, while debugging a Windows emacs executable, found > that functions comprised of more than one (non-contiguous) > address range were not being displayed correctly in a backtrace. This > is the example that Eli provided: > > (gdb) bt > #0 0x76a63227 in KERNELBASE!DebugBreak () > from C:\Windows\syswow64\KernelBase.dll > #1 0x012e7b89 in emacs_abort () at w32fns.c:10768 > #2 0x012e1f3b in print_vectorlike.cold () at print.c:1824 > #3 0x011d2dec in print_object (obj=, printcharfun=XIL(0), > escapeflag=true) at print.c:2150 > > The function print_vectorlike consists of two address ranges, one of > which contains "cold" code which is expected to not execute very often. > There is a minimal symbol, print_vectorlike.cold.65, which is the address > of the "cold" range. > > GDB is prefering this minsym over the the name provided by the > DWARF info due to some really old code in GDB which handles > "certain pathological cases". See the first big block comment > in find_frame_funname for more information. Yuck! > > I considered removing the code for this corner case entirely, but it > seems as though it might still be useful, so I left it intact. > Yeah, I'd be inclined to try removing it too. The comment smells of a.out or stabs limitations. But I'm not 100% sure, and I'm sympathetic with forward incremental progress. > That code is already disabled for inline functions. I added a > condition which disables it for non-contiguous functions as well. > > The change to find_frame_funname in stack.c fixes this problem for > stack traces. A similar change to print_address_symbolic in > printcmd.c fixes this problem for the "x" command and other commands > which use print_address_symbolic(). > > gdb/ChangeLog: > > * stack.c (find_frame_funname): Disable use of minsym for function > name in functions comprised of non-contiguous blocks. > * printcmd.c (print_address_symbolic): Likewise. > --- > gdb/printcmd.c | 4 +++- > gdb/stack.c | 15 ++++++++++++--- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/gdb/printcmd.c b/gdb/printcmd.c > index 9e84594fe6..e00a9c671a 100644 > --- a/gdb/printcmd.c > +++ b/gdb/printcmd.c > @@ -627,7 +627,9 @@ build_address_symbolic (struct gdbarch *gdbarch, > > if (msymbol.minsym != NULL) > { > - if (BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location || symbol == NULL) > + if (symbol == NULL > + || (BLOCK_CONTIGUOUS_P (SYMBOL_BLOCK_VALUE (symbol)) > + && BMSYMBOL_VALUE_ADDRESS (msymbol) > name_location)) I think this warrants a comment somewhere. Maybe a bit higher up, where it reads: /* First try to find the address in the symbol table, then in the minsyms. Take the closest one. */ Or better yet, abstract out the relevant bits in build_address_symbolic and find_frame_funname to a separate function, and then use that function in both places? IIUC, in both cases, we're looking to see if there's a minimal symbol that would be better than the debug symbol we start with. > @@ -1067,9 +1067,18 @@ find_frame_funname (struct frame_info *frame, enum language *funlang, > > struct bound_minimal_symbol msymbol; > > - /* Don't attempt to do this for inlined functions, which do not > - have a corresponding minimal symbol. */ > - if (!block_inlined_p (SYMBOL_BLOCK_VALUE (func))) > + /* Don't attempt to do this for two cases: > + > + 1) Inlined functions, which do not have a corresponding minimal > + symbol. > + > + 2) Functions which are comprised of non-contiguous blocks. > + Such functions often contain a minimal symbol for a > + "cold" range, i.e. code which is not expected to execute > + very often. It is incorrect to use the minimal symbol > + associated with this range. */ I don't find this "incorrect" here very useful -- why is it incorrect is my immediate question when I read this. Maybe that old comment: So look in the minimal symbol tables as well, and if it comes up with a larger address for the function use that instead. I don't think this can ever cause any problems; there shouldn't be any minimal symbols in the middle of a function; should be updated. Thanks, Pedro Alves