Do not skip prologue for asm (.S) files

  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.
  

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 .*"
>
  

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,
>                                                      &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?

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,
>>                                                      &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?
>
> 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, &current_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,
                                                  &current_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, &current_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,
>                                                   &current_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