Re: [PATCH] gdb: Don't skip prologue for explicit line breakpoints in assembler

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* Pedro Alves <palves@redhat.com> [2019-06-24 20:16:23 +0100]:

> On 6/22/19 12:05 PM, Andrew Burgess wrote:
> > * Pedro Alves <palves@redhat.com> [2019-06-21 14:43:26 +0100]:
> > 
> >> On 6/21/19 12:23 AM, Andrew Burgess wrote:
> >>
> >>> I spent some more time trying to find a path that would call both
> >>> 'decode_digits_list_mode' and then 'skip_prologue_sal', but I still
> >>> can't find one.
> >>
> >> But won't that change affect any code path that ends up in
> >> skip_prologue_sal with explicit_line set?
> > 
> >     [ Disclaimer: In the below I'll take about 'in current testing we
> >          never do X'.  I understand that this doesn't mean GDB will
> >          _never_ do X as our testing doesn't guarantee to hit every
> >          possible code path, it's more an invitation for people to
> >          suggest how me might create a test that does do X. ]
> > 
> > Indeed.  My claim is that in the current testing we never get to
> > skip_prologue_sal with explicit_line set.  My patch means we do now
> > enter skip_prologue_sal with explicit_line set, and I find that the
> > existing check that uses explicit_line means GDB doesn't behave as I'd
> > like.
> > 
> > Given that in HEAD explicit_line only seems to be set when decoding a
> > line spec in 'list_mode', my current belief is that explicit_line is
> > never set in a condition where the flag will then be checked.  In
> > other words, I think the explicit_line is currently useless.
> 
> Since the flag is checked in breakpoint.c, it likely had a use
> for breakpoint mode too, though as you say, it's probably not used today.
> 
> Greping for explicit_line finds this case:
> 
> 
>   set_breakpoint_location_function (loc,
> 				    sal->explicit_pc || sal->explicit_line);
> 
> in 
> 
>   add_location_to_breakpoint
> 
> I wonder whether that explicit_line use makes sense here.
> 
> set_breakpoint_location_function says:
> 
>  /* Initialize loc->function_name.  EXPLICIT_LOC says no indirect function
>     resolutions should be made as the user specified the location explicitly
>     enough.  */
> 
>  static void
>  set_breakpoint_location_function (struct bp_location *loc, int explicit_loc)
>  {
> 
> 
> I'm not immediately seeing how to set a breakpoint at an ifunc symbol
> by line number such that you end up in set_breakpoint_location_function
> with loc->msymbol != NULL.

We certainly never get into set_breakpoint_location_function with
explicit_line set in current HEAD.

The explicit_loc parameter was added in commit 0e30163f127122 (which
was really about adding or improving ifunc support) and the tests were
in commit e462023046d892.

Out of interest I ran the testsuite checking to see if:

  loc->msymbol != NULL && explicit_loc

was ever true (in current HEAD) and it never is.  I suspect this is
because loc->msymbol is initialised from sal->msymbol, which is only
set in one place linespec.c:minsym_found, which I don't think allows
for the address and/or line number to be explicitly stated.

After this investigation I propose that the explicit_loc parameter be
removed completely from set_breakpoint_location_function - I've
included this in an updated series that I'll post shortly

> 
> We should probably delete that sal->explicit_line reference.

Agreed.

> 
> > 
> >>
> >> E.g.:
> >>
> >> /* Helper function for break_command_1 and disassemble_command.  */
> >>
> >> void
> >> resolve_sal_pc (struct symtab_and_line *sal)
> >> {
> >>   CORE_ADDR pc;
> >>
> >>   if (sal->pc == 0 && sal->symtab != NULL)
> >>     {
> >>       if (!find_line_pc (sal->symtab, sal->line, &pc))
> >> 	error (_("No line %d in file \"%s\"."),
> >> 	       sal->line, symtab_to_filename_for_display (sal->symtab));
> >>       sal->pc = pc;
> >>
> >>       /* If this SAL corresponds to a breakpoint inserted using a line
> >>          number, then skip the function prologue if necessary.  */
> >>       if (sal->explicit_line)
> >> 	skip_prologue_sal (sal);
> >>     }
> >>
> >> Is that path unreachable today?
> > 
> > In current testing we enter this code block once, by accident.
> > 
> > The test 'gdb.dwarf2/dw2-objfile-overlap.exp' enters the block because
> > we load a symbol file at address 0.  The check '(sal->pc == 0 &&
> > sal->symtab != NULL)' is trying to find SALs where the 'pc' has not
> > been set, in our case the 'pc' has been set; to zero.  When we then
> > call 'find_line_pc' and then 'sal->pc = pc', we reset the 'pc' to zero
> > again.
> > 
> > In this one case the explicit_line flag is false, so skip_prologue_sal
> > is never called.
> > 
> > As an aside how would you feel about a patch that made the 'pc' field
> > of symtab_and_line private, and updated all users to use getter/setter
> > methods?  
> 
> Sounds fine to me in principle.

I'll do this as a separate patch.

> 
> > I already did this in order to add a 'is_pc_initialised?'
> > type method to symtab_and_line.  When I add this and change the above
> > code to say this:
> > 
> >   void
> >   resolve_sal_pc (struct symtab_and_line *sal)
> >   {
> >     CORE_ADDR pc;
> > 
> >     if (sal->symtab != NULL && !sal->pc_p ())
> >       {
> >         // ... etc ...
> > 
> > then we no longer enter this block at all during the current testing.
> 
> So does this mean that linespec nowadays always fills in the PC
> then?

This is my claim.

> 
> If that's the case, when do we need that is_pc_initialized method?

I imagine it would only be used within an assertion.

> 
> I wonder what else is not reachable around here, laying around
> waiting to be garbage collected...

Indeed...

> 
> >>> Looking back at how the explicit_line flag was originally used when
> >>> it was added in commit ed0616c6b78a0966, things have changed quite a
> >>> bit in the 10+ years since.  There were some tests added along with
> >>> the explicit_line flag (gdb.cp/mb-*.exp) and these all pass both in
> >>> master and in my patched branch.
> >>>
> >>> My current thinking is that the explicit_line flag was no longer doing
> >>> anything useful in master, but if someone disagrees I'd love to
> >>> understand more about this.
> >>
> >> I seem to recall that GDB didn't use to update a breakpoint's line
> >> number to advance to the next line number that includes some actual
> >> compiled code.  Like if you set a breakpoint at line 10 below:
> >>
> >>  10    // just a comment
> >>  11    i++;
> >>
> >> you end up with a breakpoint at line 11.  Maybe it's old code
> >> related to that.
> > 
> > I wonder if what you meant to say here is the breakpoint is placed at
> > the address of line 11, but is recorded as being at line 10.  This
> > actually would line up with what the explicit line flag was doing if
> > the explicit line flag was being set.
> 
> Yes, exactly.
> 
> > 
> > The problem seems to be that when the explicit_line flag was first
> > added there was just function for decoding linespec line numbers
> > 'decode_all_digits'.  At some point in time this split into
> > decode_digits_ordinary and decode_digits_list_mode, when this happened
> > the explicit_line flag was only ever being set in one path.
> > 
> > I suspect that if the behaviour you discussed above ever existed, then
> > it was before the split in how digits were decoded.
> > 
> > I'm running out of time to investigate this today, but when I get some
> > more time I'll dig a little more on this line of enquiry to see if I
> > can confirm or deny the above theory.
> 
> Did you check whether we're already setting explicit_line when
> parsing "b -line N", i.e., when using the explicit locations syntax?

In current HEAD explicit_line will only get set for the clear, edit,
list, and 'info line' commands.  Any variation of setting breakpoints
will never set explicit_line.

I'll post an updated series very shortly.

Thanks,
Andrew

> 
> > 
> >>
> >> Maybe I'm misremembering.
> >>
> >> In any case, if you change this, then you also should change
> >> the function's entry comment:
> >>
> >>  /* Adjust SAL to the first instruction past the function prologue.
> >>     If the PC was explicitly specified, the SAL is not changed.
> >>     If the line number was explicitly specified, at most the SAL's PC
> >>     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>     is updated.  If SAL is already past the prologue, then do nothing.  */
> >>     ^^^^^^^^^^
> > 
> > Would this be OK?  (I'm not pushing anything until the above questions
> > are resolved):
> 
> Sure.
> 
> > 
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index c10e6b3e358..6e7e32fb4d8 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3673,8 +3673,10 @@ skip_prologue_using_lineinfo (CORE_ADDR func_addr, struct symtab *symtab)
> >  
> >  /* Adjust SAL to the first instruction past the function prologue.
> >     If the PC was explicitly specified, the SAL is not changed.
> > -   If the line number was explicitly specified, at most the SAL's PC
> > -   is updated.  If SAL is already past the prologue, then do nothing.  */
> > +   If the line number was explicitly specified then the SAL can still be
> > +   updated, unless the language for SAL is assembler, in which case the SAL
> > +   will be left unchanged.
> > +   If SAL is already past the prologue, then do nothing.  */
> >  
> >  void
> >  skip_prologue_sal (struct symtab_and_line *sal)
> Thanks,
> Pedro Alves