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

[Doug Evans <dje at google dot com>]

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
>