From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75536 invoked by alias); 23 Jun 2015 20:35:10 -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 75525 invoked by uid 89); 23 Jun 2015 20:35:09 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.7 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 23 Jun 2015 20:35:08 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 9BF9BBC6B7; Tue, 23 Jun 2015 20:35:07 +0000 (UTC) Received: from host1.jankratochvil.net (ovpn-116-41.ams2.redhat.com [10.36.116.41]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5NKZ2OZ026204 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 23 Jun 2015 16:35:06 -0400 Date: Tue, 23 Jun 2015 20:35:00 -0000 From: Jan Kratochvil To: Doug Evans Cc: gdb-patches , Sergio Durigan Junior Subject: Re: [patch] Do not skip prologue for asm (.S) files Message-ID: <20150623203501.GA23565@host1.jankratochvil.net> References: <20150620154449.GA24593@host1.jankratochvil.net> <20150622211556.GA26323@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes X-SW-Source: 2015-06/txt/msg00507.txt.bz2 On Tue, 23 Jun 2015 01:46:08 +0200, Doug Evans wrote: > If locations_valid and COMPUNIT_LOCATIONS_VALID > were renamed to locations_valid_at_entry and > COMPUNIT_LOCATIONS_VALID_AT_ENTRY > this code would be massively more readable to me > and might even be self documenting (and thus not needing > any further comments, yay). > > How about that? "at entry" is only one of its possible use cases. In fact it guarantees "always", that is "for every PC". That means not only "at entry" but also "during prologue" and "during epilogue". Although you are right the current comment for "locations_valid" talks only about that "at entry" aspect. > Here's the full function. > [apologies if gmail messes this up :-(] That is very OT but if some mailer messes up your mails why do you use it. > static void > minsym_found (struct linespec_state *self, struct objfile *objfile, > struct minimal_symbol *msymbol, > struct symtabs_and_lines *result) > { > struct gdbarch *gdbarch = get_objfile_arch (objfile); > CORE_ADDR pc; > struct symtab_and_line sal; > > sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol), > (struct obj_section *) 0, 0); > sal.section = MSYMBOL_OBJ_SECTION (objfile, msymbol); > > /* The minimal symbol might point to a function descriptor; > resolve it to the actual code address instead. */ > pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, ¤t_target); > if (pc != sal.pc) > sal = find_pc_sect_line (pc, NULL, 0); > > if (self->funfirstline) > skip_prologue_sal (&sal); > > if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc)) > add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0); > } > > With the patch added, we're using the value of > MSYMBOL_VALUE_ADDRESS twice > and calling gdbarch_convert_from_func_ptr_addr twice. > [I'm not micro-optimizing here, my goal is code readability.] > > Plus the patch does: > > sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol); > sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, > ¤t_target); > > but it doesn't update sal.section nor sal.line. OK, I agree that seems wrong. > Do we need to do anything at all here? > IOW, what cases does the following alternative miss? > I'm wondering if we need to add some clarity to > find_pc_sect_line's API. > > if (self->funfirstline > && (sal.symtab == NULL > || !COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab)))) > skip_prologue_sal (&sal); As I said SAL was created by find_pc_sect_line: sal = find_pc_sect_line (MSYMBOL_VALUE_ADDRESS (objfile, msymbol), (struct obj_section *) 0, 0); and it is not guaranteed that: sal.pc == MSYMBOL_VALUE_ADDRESS even when we do not call skip_prologue_sal(). (ignoring gdbarch_convert_from_func_ptr_addr() here) > But now I'm wondering how is the reader supposed to interpret the > name "locations_valid". I can hear the reader asking > "Locations are valid in assembler???" > Renaming it to locations_valid_at_entry will help, a bit. Discussed above. Another solution is to rename it to "locations_not_valid", that would be fine as 'false' for language_asm. :-) > And comment saying that we're setting locations_valid_at_entry > to avoid prologue skipping in the assembler case will improve the > readability a lot (to this reader). > > E.g. > > if ((cu->has_loclist && gcc_4_minor >= 5) > /* The goal here is to avoid prologue skipping, which we want > for assembler. */ > || cu->language == language_asm) OK but we should first settle down on the "locations_valid" name. Jan