Message ID | 20150620154449.GA24593@host1.jankratochvil.net |
---|---|
State | New, archived |
Headers |
Received: (qmail 123312 invoked by alias); 20 Jun 2015 15:44:57 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 123302 invoked by uid 89); 20 Jun 2015 15:44:57 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, KAM_STOCKGEN, RP_MATCHES_RCVD, SPF_HELO_PASS, SPF_PASS autolearn=no 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; Sat, 20 Jun 2015 15:44:55 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id A12A219CBEF for <gdb-patches@sourceware.org>; Sat, 20 Jun 2015 15:44:54 +0000 (UTC) Received: from host1.jankratochvil.net (ovpn-116-41.ams2.redhat.com [10.36.116.41]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5KFio6b004824 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sat, 20 Jun 2015 11:44:52 -0400 Date: Sat, 20 Jun 2015 17:44:49 +0200 From: Jan Kratochvil <jan.kratochvil@redhat.com> To: gdb-patches@sourceware.org Cc: Sergio Durigan Junior <sergiodj@redhat.com> Subject: [patch] Do not skip prologue for asm (.S) files Message-ID: <20150620154449.GA24593@host1.jankratochvil.net> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nFreZHaLTZJo0R7j" Content-Disposition: inline User-Agent: Mutt/1.5.23 (2014-03-12) X-IsSubscribed: yes |
Commit Message
Jan Kratochvil
June 20, 2015, 3:44 p.m. UTC
Hi, https://bugzilla.redhat.com/show_bug.cgi?id=1084404 GDB tries to skip prologue for .S files according to .debug_line but it then places the breakpoint to a location where it is never hit. This is because #defines in .S files cause prologue skipping which is completely inappropriate, for s390x: glibc/sysdeps/unix/syscall-template.S 78:/* This is a "normal" system call stub: if there is an error, 79: it returns -1 and sets errno. */ 80: 81:T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS) 82: ret 00000000000f4210 T __select Line Number Statements: Extended opcode 2: set Address to 0xf41c8 Advance Line by 80 to 81 Copy Advance PC by 102 to 0xf422e Special opcode 6: advance Address by 0 to 0xf422e and Line by 1 to 82 Special opcode 34: advance Address by 2 to 0xf4230 and Line by 1 to 83 Advance PC by 38 to 0xf4256 Extended opcode 1: End of Sequence Compilation Unit @ offset 0x28b3e0: <0><28b3eb>: Abbrev Number: 1 (DW_TAG_compile_unit) <28b3ec> DW_AT_stmt_list : 0x7b439 <28b3f0> DW_AT_low_pc : 0xf41c8 <28b3f8> DW_AT_high_pc : 0xf4256 <28b400> DW_AT_name : ../sysdeps/unix/syscall-template.S <28b423> DW_AT_comp_dir : /usr/src/debug////////glibc-2.17-c758a686/misc <28b452> DW_AT_producer : GNU AS 2.23.52.0.1 <28b465> DW_AT_language : 32769 (MIPS assembler) without debuginfo - correct address: (gdb) b select Breakpoint 1 at 0xf4210 with debuginfo, either with or without the fix: (gdb) b select Breakpoint 1 at 0xf41c8: file ../sysdeps/unix/syscall-template.S, line 81. One part is to make 'locations_valid' true even for asm files. /* Symtab has been compiled with both optimizations and debug info so that GDB may stop skipping prologues as variables locations are valid already at function entry points. */ unsigned int locations_valid : 1; The other part is to extend the 'locations_valid' functionality more - I have chosen mostly randomly this place. No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu. Jan gdb/ChangeLog 2015-06-20 Jan Kratochvil <jan.kratochvil@redhat.com> * 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 <jan.kratochvil@redhat.com> * gdb.arch/amd64-prologue-skip.S: New file. * gdb.arch/amd64-prologue-skip.exp: New file.
Comments
On Sat, Jun 20, 2015 at 10:44 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > Hi, > > https://bugzilla.redhat.com/show_bug.cgi?id=1084404 > > GDB tries to skip prologue for .S files according to .debug_line but it then > places the breakpoint to a location where it is never hit. > > This is because #defines in .S files cause prologue skipping which is > completely inappropriate, for s390x: > > glibc/sysdeps/unix/syscall-template.S > 78:/* This is a "normal" system call stub: if there is an error, > 79: it returns -1 and sets errno. */ > 80: > 81:T_PSEUDO (SYSCALL_SYMBOL, SYSCALL_NAME, SYSCALL_NARGS) > 82: ret > > 00000000000f4210 T __select > Line Number Statements: > Extended opcode 2: set Address to 0xf41c8 > Advance Line by 80 to 81 > Copy > Advance PC by 102 to 0xf422e > Special opcode 6: advance Address by 0 to 0xf422e and Line by 1 to 82 > Special opcode 34: advance Address by 2 to 0xf4230 and Line by 1 to 83 > Advance PC by 38 to 0xf4256 > Extended opcode 1: End of Sequence > Compilation Unit @ offset 0x28b3e0: > <0><28b3eb>: Abbrev Number: 1 (DW_TAG_compile_unit) > <28b3ec> DW_AT_stmt_list : 0x7b439 > <28b3f0> DW_AT_low_pc : 0xf41c8 > <28b3f8> DW_AT_high_pc : 0xf4256 > <28b400> DW_AT_name : ../sysdeps/unix/syscall-template.S > <28b423> DW_AT_comp_dir : /usr/src/debug////////glibc-2.17-c758a686/misc > <28b452> DW_AT_producer : GNU AS 2.23.52.0.1 > <28b465> DW_AT_language : 32769 (MIPS assembler) > > without debuginfo - correct address: > (gdb) b select > Breakpoint 1 at 0xf4210 Hi. I'm having trouble understanding the discussion. f4210 is the correct address? It would help to have more data here to understand why. [e.g., disassembly of entire function?] > > with debuginfo, either with or without the fix: > (gdb) b select > Breakpoint 1 at 0xf41c8: file ../sysdeps/unix/syscall-template.S, line 81. The fix doesn't change the breakpoint address? > One part is to make 'locations_valid' true even for asm files. > /* Symtab has been compiled with both optimizations and debug info so that > GDB may stop skipping prologues as variables locations are valid already > at function entry points. */ > unsigned int locations_valid : 1; > > The other part is to extend the 'locations_valid' functionality more - I have > chosen mostly randomly this place. Can you elaborate on this? The original code is simple and intuitive: if (self->funfirstline) skip_prologue_sal (&sal); skip_prologue_sal is pretty complicated itself, but I can read those two lines without fretting too much about whether they're correct. Now it's got this extra code, and it's not clear to this reader why it's correct. if (self->funfirstline) { if (sal.symtab != NULL && COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))) { 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); } This may be the wrong way to look at it, but one question that comes to mind is: Why can't this extra code go into skip_prologue_sal? Thanks. > No regressions on {x86_64,x86_64-m32,i686}-fedora23pre-linux-gnu. > > > Jan > > gdb/ChangeLog > 2015-06-20 Jan Kratochvil <jan.kratochvil@redhat.com> > > * 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 <jan.kratochvil@redhat.com> > > * gdb.arch/amd64-prologue-skip.S: New file. > * gdb.arch/amd64-prologue-skip.exp: New file. > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c > index d79b2e3..76ff66d 100644 > --- a/gdb/dwarf2read.c > +++ b/gdb/dwarf2read.c > @@ -8096,7 +8096,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu, > Still one can confuse GDB by using non-standard GCC compilation > options - this waits on GCC PR other/32998 (-frecord-gcc-switches). > */ > - if (cu->has_loclist && gcc_4_minor >= 5) > + if ((cu->has_loclist && gcc_4_minor >= 5) || cu->language == language_asm) > cust->locations_valid = 1; How come this isn't written as: if (cu->has_loclist && (gcc_4_minor >= 5 || cu->language == language_asm)) ? > > if (gcc_4_minor >= 5) > diff --git a/gdb/linespec.c b/gdb/linespec.c > index d2089b5..a7ea41b 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -3454,7 +3454,17 @@ 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))) > + { > + 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)) > add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0); > diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip.S b/gdb/testsuite/gdb.arch/amd64-prologue-skip.S > new file mode 100644 > index 0000000..66b806a > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-prologue-skip.S > @@ -0,0 +1,28 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2015 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see <http://www.gnu.org/licenses/>. */ > + > + .text > +/*0*/ hlt > +pushrbp: .globl pushrbp > +#define PUSHRBP push %rbp; mov %rsp, %rbp; nop > +/*1*/ PUSHRBP > +/*6*/ hlt > + > +/*7*/ hlt > +#define MINSYM nop; .globl minsym; minsym: nop > +/*8*/ MINSYM > +/*a*/ hlt > diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp b/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp > new file mode 100644 > index 0000000..015cd69 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp > @@ -0,0 +1,35 @@ > +# Copyright 2010-2015 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > + > +standard_testfile .S > +set binfile ${binfile}.o > + > +if { ![istarget x86_64-*-* ] || ![is_lp64_target] } { > + verbose "Skipping ${testfile}." > + return > +} > + > +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {debug}] != "" } { > + untested ${testfile} > + return > +} > + > +clean_restart ${binfile} > + > +gdb_test "break *pushrbp" " at 0x1: file .*" > +gdb_test "break pushrbp" " at 0x1: file .*" > + > +gdb_test "break *minsym" " at 0x9: file .*" > +gdb_test "break minsym" " at 0x9: file .*" >
On Mon, 22 Jun 2015 00:52:54 +0200, Doug Evans wrote: > On Sat, Jun 20, 2015 at 10:44 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > > without debuginfo - correct address: > > (gdb) b select > > Breakpoint 1 at 0xf4210 > > Hi. I'm having trouble understanding the discussion. > > f4210 is the correct address? Yes. It is also where .dynsym+.symtab point to: 00000000000f4210 T __select 00000000000f4210 W select > It would help to have more data here to understand why. > [e.g., disassembly of entire function?] It is attached. The important part is that for -O2 -g code the very first instruction can jump arbitrarily, no matter what .debug_line says. So the disassembly does not matter much, I cannot read much s390 anyway. > > with debuginfo, either with or without the fix: > > (gdb) b select > > Breakpoint 1 at 0xf41c8: file ../sysdeps/unix/syscall-template.S, line 81. > > The fix doesn't change the breakpoint address? This is a typo from copy-pasting, it should have said: with debuginfo, without the fix: Sure "with the fix" it now puts the breakpoint at the correct address 0xf4210. > > One part is to make 'locations_valid' true even for asm files. > > /* Symtab has been compiled with both optimizations and debug info so that > > GDB may stop skipping prologues as variables locations are valid already > > at function entry points. */ > > unsigned int locations_valid : 1; > > > > The other part is to extend the 'locations_valid' functionality more - I have > > chosen mostly randomly this place. > > Can you elaborate on this? > The original code is simple and intuitive: > > if (self->funfirstline) > skip_prologue_sal (&sal); > > skip_prologue_sal is pretty complicated itself, but I can read those > two lines without fretting too much about whether they're correct. The complications in skip_prologue_sal IMO do not work anymore plus they are irrelevant as they are for various *_deprecated_* arch features. But I do not want to discuss it more or touch that so I just try to disable that code for new compilers with correct debug info. > Now it's got this extra code, and it's not clear to this reader why > it's correct. > > if (self->funfirstline) > { > if (sal.symtab != NULL > && COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))) > { > 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); > } > > This may be the wrong way to look at it, but one question that comes to mind is: > Why can't this extra code go into skip_prologue_sal? Because to read COMPUNIT_LOCATIONS_VALID one needs to find the symtab. So one needs to map PC->SAL by find_pc_sect_line(). But then SAL.PC is already "corrupted" by some .debug_line matching. Maybe one could map find the symtab a different way but when the symtab was already fetched in this code. > > --- a/gdb/dwarf2read.c > > +++ b/gdb/dwarf2read.c > > @@ -8096,7 +8096,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu, > > Still one can confuse GDB by using non-standard GCC compilation > > options - this waits on GCC PR other/32998 (-frecord-gcc-switches). > > */ > > - if (cu->has_loclist && gcc_4_minor >= 5) > > + if ((cu->has_loclist && gcc_4_minor >= 5) || cu->language == language_asm) > > cust->locations_valid = 1; > > How come this isn't written as: > > if (cu->has_loclist && (gcc_4_minor >= 5 || cu->language == language_asm)) > > ? This conditional would not work. language_asm always has has_loclist==0. has_loclist says "This CU references .debug_loc." - language_asm CUs never generate/reference .debug_loc. The check 'gcc_4_minor >= 5' is there also only for C/C++ (as described in the large comment there), not for language_asm. For language_asm 'gcc_4_minor' is always -1. Jan /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:81 f41c8: eb 25 f0 10 00 24 stmg %r2,%r5,16(%r15) f41ce: eb df f0 68 00 24 stmg %r13,%r15,104(%r15) f41d4: b9 04 00 ef lgr %r14,%r15 f41d8: a7 fb ff 60 aghi %r15,-160 f41dc: e3 e0 f0 00 00 24 stg %r14,0(%r15) f41e2: c0 e5 00 00 cd c7 brasl %r14,10dd70 <__libc_enable_asynccancel> f41e8: b9 04 00 02 lgr %r0,%r2 f41ec: eb 25 f0 b0 00 04 lmg %r2,%r5,176(%r15) f41f2: 0a 8e svc 142 f41f4: b9 04 00 d2 lgr %r13,%r2 f41f8: b9 04 00 20 lgr %r2,%r0 f41fc: c0 e5 00 00 cd fa brasl %r14,10ddf0 <__libc_disable_asynccancel> f4202: b9 04 00 2d lgr %r2,%r13 f4206: eb df f1 08 00 04 lmg %r13,%r15,264(%r15) f420c: a7 f4 00 0a j f4220 <__select_nocancel+0x2> 00000000000f4210 <__select>: __GI_select(): f4210: c0 10 00 05 62 a2 larl %r1,1a0754 <__libc_multiple_threads> f4216: bf 0f 10 00 icm %r0,15,0(%r1) f421a: a7 74 ff d7 jne f41c8 <setdomainname+0x38> 00000000000f421e <__select_nocancel>: f421e: 0a 8e svc 142 f4220: a7 49 f0 01 lghi %r4,-4095 f4224: b9 21 00 24 clgr %r2,%r4 f4228: c0 b4 00 00 00 04 jgnl f4230 <__select_nocancel+0x12> /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:82 f422e: 07 fe br %r14 /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:83 f4230: 13 02 lcr %r0,%r2 f4232: c0 10 00 05 38 07 larl %r1,19b240 <_GLOBAL_OFFSET_TABLE_+0x240> f4238: e3 10 10 00 00 04 lg %r1,0(%r1) f423e: b2 4f 00 20 ear %r2,%a0 f4242: eb 22 00 20 00 0d sllg %r2,%r2,32 f4248: b2 4f 00 21 ear %r2,%a1 f424c: 50 01 20 00 st %r0,0(%r1,%r2) f4250: a7 29 ff ff lghi %r2,-1 f4254: 07 fe br %r14 __select_nocancel(): f4256: 07 07 nopr %r7
On Mon, Jun 22, 2015 at 4:15 PM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: > On Mon, 22 Jun 2015 00:52:54 +0200, Doug Evans wrote: >> On Sat, Jun 20, 2015 at 10:44 AM, Jan Kratochvil <jan.kratochvil@redhat.com> wrote: >> > without debuginfo - correct address: >> > (gdb) b select >> > Breakpoint 1 at 0xf4210 >> >> Hi. I'm having trouble understanding the discussion. >> >> f4210 is the correct address? > > Yes. It is also where .dynsym+.symtab point to: > 00000000000f4210 T __select > 00000000000f4210 W select > > >> It would help to have more data here to understand why. >> [e.g., disassembly of entire function?] > > It is attached. The important part is that for -O2 -g code the very first > instruction can jump arbitrarily, no matter what .debug_line says. > So the disassembly does not matter much, I cannot read much s390 anyway. > > >> > with debuginfo, either with or without the fix: >> > (gdb) b select >> > Breakpoint 1 at 0xf41c8: file ../sysdeps/unix/syscall-template.S, line 81. >> >> The fix doesn't change the breakpoint address? > > This is a typo from copy-pasting, it should have said: > > with debuginfo, without the fix: > > Sure "with the fix" it now puts the breakpoint at the correct address 0xf4210. > > >> > One part is to make 'locations_valid' true even for asm files. >> > /* Symtab has been compiled with both optimizations and debug info so that >> > GDB may stop skipping prologues as variables locations are valid already >> > at function entry points. */ >> > unsigned int locations_valid : 1; >> > >> > The other part is to extend the 'locations_valid' functionality more - I have >> > chosen mostly randomly this place. >> >> Can you elaborate on this? >> The original code is simple and intuitive: >> >> if (self->funfirstline) >> skip_prologue_sal (&sal); >> >> skip_prologue_sal is pretty complicated itself, but I can read those >> two lines without fretting too much about whether they're correct. > > The complications in skip_prologue_sal IMO do not work anymore plus they are > irrelevant as they are for various *_deprecated_* arch features. But I do not > want to discuss it more or touch that so I just try to disable that code for > new compilers with correct debug info. gdb still has to work in a lot of situations on a lot of targets. Some things may be deprecated, but that doesn't make them irrelevant. Plus we still need to have readable code. 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? > >> Now it's got this extra code, and it's not clear to this reader why >> it's correct. >> >> if (self->funfirstline) >> { >> if (sal.symtab != NULL >> && COMPUNIT_LOCATIONS_VALID (SYMTAB_COMPUNIT (sal.symtab))) >> { >> 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); >> } >> >> This may be the wrong way to look at it, but one question that comes to mind is: >> Why can't this extra code go into skip_prologue_sal? > > Because to read COMPUNIT_LOCATIONS_VALID one needs to find the symtab. > So one needs to map PC->SAL by find_pc_sect_line(). But then SAL.PC is > already "corrupted" by some .debug_line matching. > > Maybe one could map find the symtab a different way but when the symtab was > already fetched in this code. Digging deeper, I'm still wondering what to do here. Here's the full function. [apologies if gmail messes this up :-(] 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. As a reader I'm left wondering if there's a problem here (even if there isn't a problem making the reader wonder is suboptimal). 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); [btw, it's kinda unfortunate that we've got an msymbol here, but the first thing find_pc_sect_line does is look up the msymbol given its pc. A topic for another day ...] > >> > --- a/gdb/dwarf2read.c >> > +++ b/gdb/dwarf2read.c >> > @@ -8096,7 +8096,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu, >> > Still one can confuse GDB by using non-standard GCC compilation >> > options - this waits on GCC PR other/32998 (-frecord-gcc-switches). >> > */ >> > - if (cu->has_loclist && gcc_4_minor >= 5) >> > + if ((cu->has_loclist && gcc_4_minor >= 5) || cu->language == language_asm) >> > cust->locations_valid = 1; >> >> How come this isn't written as: >> >> if (cu->has_loclist && (gcc_4_minor >= 5 || cu->language == language_asm)) >> >> ? > > This conditional would not work. language_asm always has has_loclist==0. > has_loclist says "This CU references .debug_loc." - language_asm CUs never > generate/reference .debug_loc. Righto. 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. 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) > The check 'gcc_4_minor >= 5' is there also only for C/C++ (as described in the > large comment there), not for language_asm. For language_asm 'gcc_4_minor' is > always -1. > > > Jan > > /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:81 > f41c8: eb 25 f0 10 00 24 stmg %r2,%r5,16(%r15) > f41ce: eb df f0 68 00 24 stmg %r13,%r15,104(%r15) > f41d4: b9 04 00 ef lgr %r14,%r15 > f41d8: a7 fb ff 60 aghi %r15,-160 > f41dc: e3 e0 f0 00 00 24 stg %r14,0(%r15) > f41e2: c0 e5 00 00 cd c7 brasl %r14,10dd70 <__libc_enable_asynccancel> > f41e8: b9 04 00 02 lgr %r0,%r2 > f41ec: eb 25 f0 b0 00 04 lmg %r2,%r5,176(%r15) > f41f2: 0a 8e svc 142 > f41f4: b9 04 00 d2 lgr %r13,%r2 > f41f8: b9 04 00 20 lgr %r2,%r0 > f41fc: c0 e5 00 00 cd fa brasl %r14,10ddf0 <__libc_disable_asynccancel> > f4202: b9 04 00 2d lgr %r2,%r13 > f4206: eb df f1 08 00 04 lmg %r13,%r15,264(%r15) > f420c: a7 f4 00 0a j f4220 <__select_nocancel+0x2> Brings back days of reading the 370 P of O. :-) > > 00000000000f4210 <__select>: > __GI_select(): > f4210: c0 10 00 05 62 a2 larl %r1,1a0754 <__libc_multiple_threads> > f4216: bf 0f 10 00 icm %r0,15,0(%r1) > f421a: a7 74 ff d7 jne f41c8 <setdomainname+0x38> > > 00000000000f421e <__select_nocancel>: > f421e: 0a 8e svc 142 > f4220: a7 49 f0 01 lghi %r4,-4095 > f4224: b9 21 00 24 clgr %r2,%r4 > f4228: c0 b4 00 00 00 04 jgnl f4230 <__select_nocancel+0x12> > /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:82 > f422e: 07 fe br %r14 > /usr/src/debug////////glibc-2.17-c758a686/misc/../sysdeps/unix/syscall-template.S:83 > f4230: 13 02 lcr %r0,%r2 > f4232: c0 10 00 05 38 07 larl %r1,19b240 <_GLOBAL_OFFSET_TABLE_+0x240> > f4238: e3 10 10 00 00 04 lg %r1,0(%r1) > f423e: b2 4f 00 20 ear %r2,%a0 > f4242: eb 22 00 20 00 0d sllg %r2,%r2,32 > f4248: b2 4f 00 21 ear %r2,%a1 > f424c: 50 01 20 00 st %r0,0(%r1,%r2) > f4250: a7 29 ff ff lghi %r2,-1 > f4254: 07 fe br %r14 > __select_nocancel(): > f4256: 07 07 nopr %r7 >
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
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c index d79b2e3..76ff66d 100644 --- a/gdb/dwarf2read.c +++ b/gdb/dwarf2read.c @@ -8096,7 +8096,7 @@ process_full_comp_unit (struct dwarf2_per_cu_data *per_cu, Still one can confuse GDB by using non-standard GCC compilation options - this waits on GCC PR other/32998 (-frecord-gcc-switches). */ - if (cu->has_loclist && gcc_4_minor >= 5) + if ((cu->has_loclist && gcc_4_minor >= 5) || cu->language == language_asm) cust->locations_valid = 1; if (gcc_4_minor >= 5) diff --git a/gdb/linespec.c b/gdb/linespec.c index d2089b5..a7ea41b 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -3454,7 +3454,17 @@ 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))) + { + 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)) add_sal_to_sals (self, result, &sal, MSYMBOL_NATURAL_NAME (msymbol), 0); diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip.S b/gdb/testsuite/gdb.arch/amd64-prologue-skip.S new file mode 100644 index 0000000..66b806a --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-prologue-skip.S @@ -0,0 +1,28 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2015 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + + .text +/*0*/ hlt +pushrbp: .globl pushrbp +#define PUSHRBP push %rbp; mov %rsp, %rbp; nop +/*1*/ PUSHRBP +/*6*/ hlt + +/*7*/ hlt +#define MINSYM nop; .globl minsym; minsym: nop +/*8*/ MINSYM +/*a*/ hlt diff --git a/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp b/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp new file mode 100644 index 0000000..015cd69 --- /dev/null +++ b/gdb/testsuite/gdb.arch/amd64-prologue-skip.exp @@ -0,0 +1,35 @@ +# Copyright 2010-2015 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +standard_testfile .S +set binfile ${binfile}.o + +if { ![istarget x86_64-*-* ] || ![is_lp64_target] } { + verbose "Skipping ${testfile}." + return +} + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" object {debug}] != "" } { + untested ${testfile} + return +} + +clean_restart ${binfile} + +gdb_test "break *pushrbp" " at 0x1: file .*" +gdb_test "break pushrbp" " at 0x1: file .*" + +gdb_test "break *minsym" " at 0x9: file .*" +gdb_test "break minsym" " at 0x9: file .*"