This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Do not skip prologue for asm (.S) files


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,
                                                     &current_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,
> +                                                      &current_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 .*"
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]