From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73769 invoked by alias); 28 Jun 2018 17:42:08 -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 73756 invoked by uid 89); 28 Jun 2018 17:42:08 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=opposite X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 28 Jun 2018 17:42:06 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 839E956141; Thu, 28 Jun 2018 13:42:05 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id XMdDy3dvFJhC; Thu, 28 Jun 2018 13:42:05 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 4E7D35613F; Thu, 28 Jun 2018 13:42:05 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 8434B8793F; Thu, 28 Jun 2018 10:42:03 -0700 (PDT) Date: Thu, 28 Jun 2018 17:42:00 -0000 From: Joel Brobecker To: Pedro Alves Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 2/2] "break LINENO/*ADDRESS", inline functions and "info break" output Message-ID: <20180628174203.GC2511@adacore.com> References: <77528608-be2c-5a21-e250-36e7d56ba950@redhat.com> <20180628145035.24713-2-palves@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180628145035.24713-2-palves@redhat.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-SW-Source: 2018-06/txt/msg00696.txt.bz2 Hi Pedro, > While experimenting with the previous patch, I noticed this inconsistency > in GDB's output: [...] > When we run to the breakpoint, we present the stop at the same line > number, and correctly show "func1" as the function name (2). > > But in "info break" output (3), notice that we say "in main", not "in > func1". > (It seems odd to me that block_linkage_function says "the CONTAINING > function will be returned", and then block_containing_function says it > returns "the closest enclosing function". Something seems reversed > here. Still, I've kept the same nomenclature and copied the comments, > so that at least there's consistency. Maybe we should fix that up > somehow.) That seems opposite to me as well... > Then I wondered, why make print_breakpoint_location look up the symbol > every time it is called, instead of just always storing the symbol > when the location is created, since the location already stores the > symbol in some cases. Agreed. > So to find which cases might be missing setting > the symbol in the sal which is used to create the breakpoint location, > I added an assertion to print_breakpoint_location, and ran the > testsuite. That caught a few places, unsurprisingly: > > - setting a breakpoint by line number > - setting a breapoint by address > - ifunc resolving > > Those are all fixed by this commit. Nice approach! > I decided not to add the > assertion to block_linkage_function and leave the existing "if (sym)" > check in place, because it's plausible that we have symtabs with line > info but no symbols. I.e., that would not be a GDB bug, but > a peculiarity of debug info input. Agreed as well. > gdb/ChangeLog: > yyyy-mm-dd Pedro Alves > > * blockframe.c (find_pc_sect_containing_function): New function. > * breakpoint.c (print_breakpoint_location): Don't call > find_pc_sect_function. > * linespec.c (create_sals_line_offset): Record the location's > symbol in the sal. > * linespec.c (convert_address_location_to_sals): Fill in sal's > symbol with find_pc_sect_containing_function. > * symtab.c (find_function_start_sal): Rename to ... > (find_function_start_sal_1): ... this. > (find_function_start_sal): Reimplement as wrapper around > find_function_start_sal_1, and use > find_pc_sect_containing_function to fill in the sal's symbol. > (find_function_start_sal(symbol*, bool)): Adjust. > * symtab.h (find_pc_function, find_pc_sect_function): Adjust > comments. > (find_pc_sect_containing_function): Declare. I know it might be considered a small and trivial part, but I really appreciate the attention to the function's comment description. > > gdb/testsuite/ChangeLog: > yyyy-mm-dd Pedro Alves > > * gdb.opt/inline-break.exp (line number, address): Add "info > break" tests. I went through the patch and it looks good. Thanks again! -- Joel