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

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

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