From patchwork Thu Jun 25 20:30:14 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Doug Evans X-Patchwork-Id: 7352 Received: (qmail 81154 invoked by alias); 25 Jun 2015 20:30:17 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 81142 invoked by uid 89); 25 Jun 2015 20:30:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, KAM_STOCKGEN, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-pd0-f201.google.com Received: from mail-pd0-f201.google.com (HELO mail-pd0-f201.google.com) (209.85.192.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Thu, 25 Jun 2015 20:30:15 +0000 Received: by pdjp10 with SMTP id p10so6874988pdj.0 for ; Thu, 25 Jun 2015 13:30:14 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:message-id:date:subject:from:to:cc :content-type; bh=Qcdno+FHn6SdOVoZyCt6rEWxb/CCUONXeuGvzgvET48=; b=PzWDwUdyDsO/QTHn4zDoY3GjExSSEpOHVdm2gjRUCou7gLoPgVeMoLuItfjhnHEe8H JctlP4UhN8lwVP3qeirBv+Ajzb2KSKSJ9bMMaROAMe/9ZlBE8X1eUB1QUhFslvbobsld gpE/p+GLLj95gqyNtI/0Rc2/iQuj4TwCNMBIO9ejThVumGGTfZx7koGZaYXPLFPXihhT ty3YKvu5BtGJXkVp42MfMN+Us02aP3JuWTdCDR9qtMO1Jz3DXEYK/PPSXypWxPzUSJ5J ERsSy5udrA+zHcTrR/0Sy16cmBLmvQJHimFN6aLQNrrG3YRzBZsba9FG9xLi4p0rCLPF Jlrw== X-Gm-Message-State: ALoCoQmTpCButvG+nrvj+xBOhF3DjjlJ3MnmUUDEnaYIvwVcvbDF6CjKushZKS8KmQxJsPqJqseI MIME-Version: 1.0 X-Received: by 10.70.42.208 with SMTP id q16mr61354767pdl.4.1435264214076; Thu, 25 Jun 2015 13:30:14 -0700 (PDT) Message-ID: <047d7bfeb318a3caf305195d7da8@google.com> Date: Thu, 25 Jun 2015 20:30:14 +0000 Subject: Re: [patch] Do not skip prologue for asm (.S) files From: Doug Evans To: Jan Kratochvil Cc: gdb-patches , Sergio Durigan Junior X-IsSubscribed: yes Jan Kratochvil writes: > On Tue, 23 Jun 2015 22:35:01 +0200, Jan Kratochvil wrote: > > On Tue, 23 Jun 2015 01:46:08 +0200, Doug Evans wrote: > > > 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. > > I do not agree, it seems correct to me. I was wondering if things were correct, not stating they weren't. > I have only added a comment to the code. Is it enough this way? The comment helps, thanks. I'm still uncomfortable with setting locations_valid for assembler. The flag may get used for more things in the future, and this feels like asking for trouble. Fortunately, I think there's a better way: instead of setting locations_valid check the language at the place where we care. add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0); [and leave dwarf2read.c as is] Ok with this change. Thanks! > I am sorry I cannot write it much cleanly as the data structures and functions > involved are not much clean. > > > Jan > gdb/ChangeLog > 2015-06-20 Jan Kratochvil > > * dwarf2read.c (process_full_comp_unit): Set LOCATIONS_VALID also for > language_asm. > * linespec.c (minsym_found): Reset sal.PC forCOMPUNIT_LOCATIONS_VALID. > > gdb/testsuite/ChangeLog > 2015-06-20 Jan Kratochvil > > * gdb.arch/amd64-prologue-skip.S: New file. > * gdb.arch/amd64-prologue-skip.exp: New file. > diff --git a/gdb/linespec.c b/gdb/linespec.c index d2089b5..71bab61 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -3454,7 +3454,23 @@ minsym_found (struct linespec_state *self, struct objfile *objfile, sal = find_pc_sect_line (pc, NULL, 0); if (self->funfirstline) - skip_prologue_sal (&sal); + { + if (sal.symtab != NULL + && (COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab)) + || SYMTAB_LANGUAGE (sal.symtab) == language_asm)) + { + /* If gdbarch_convert_from_func_ptr_addr does not apply then + sal.SECTION, sal.LINE&co. will stay correct from above. + If gdbarch_convert_from_func_ptr_addr applies then + sal.SECTION is cleared from above and sal.LINE&co. will + stay correct from the last find_pc_sect_line above. */ + sal.pc = MSYMBOL_VALUE_ADDRESS (objfile, msymbol); + sal.pc = gdbarch_convert_from_func_ptr_addr (gdbarch, sal.pc, + ¤t_target); + } + else + skip_prologue_sal (&sal); + } if (maybe_add_address (self->addr_set, objfile->pspace, sal.pc))