gdb: Don't skip prologue for explicit line breakpoints in assembler

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

> 
> 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?  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.

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

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.

> 
> 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):




Thanks,
Andrew




>  
>  void
>  skip_prologue_sal (struct symtab_and_line *sal)
>  {
> 
> Thanks,
> Pedro Alves
  

* Andrew Burgess <andrew.burgess@embecosm.com> [2019-06-22 12:05:58 +0100]:

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

The decode_all_digits function split into decode_digits_ordinary and
decode_digits_list_mode with commit f8eba3c61629b3c03a back in Dec
2011, I suspect that the behaviour you recall would have stopped
working then.  Though I haven't confirmed this (building such old
versions of GDB is time consuming).

Related to what we're discussing seems to be commit a9e408182d2faaed5
from Jan 2018, where we appear to double down on having the breakpoint
file and line update to reflect where the breakpoint was placed, not
where the breakpoint ended up.

The question then would be if we can confirm GDB did used to behave
the way you recall some time ago, do we want to go back to that
behaviour now?  And is that a blocker for my change going in?  Yes if
we did want to go back to the old behaviour then part of my patch
would probably end up being reverted - as I suspect would
a9e408182d2faaed5.  I'm happy to keep digging on this to see if I can
confirm if/when the behaviour changed if that helps bring clarity to
this issue.

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):
> 
> 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,
> Andrew
> 
> 
> 
> 
> >  
> >  void
> >  skip_prologue_sal (struct symtab_and_line *sal)
> >  {
> > 
> > Thanks,
> > Pedro Alves
  

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 should probably delete that sal->explicit_line reference.

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

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

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

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

> 
>>
>> 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
  

On 6/22/19 12:23 PM, Andrew Burgess wrote:

>> 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 like, 98.64% sure older gdbs behave like that.  But I can't rule out
misremembering without trying an old gdb, which I don't have built handy
at the moment either.

>>
>> 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.
> 
> The decode_all_digits function split into decode_digits_ordinary and
> decode_digits_list_mode with commit f8eba3c61629b3c03a back in Dec
> 2011, I suspect that the behaviour you recall would have stopped
> working then.  Though I haven't confirmed this (building such old
> versions of GDB is time consuming).

Indeed.

> 
> Related to what we're discussing seems to be commit a9e408182d2faaed5
> from Jan 2018, where we appear to double down on having the breakpoint
> file and line update to reflect where the breakpoint was placed, not
> where the breakpoint ended up.

Indeed.  Thanks for digging that up.

> 
> The question then would be if we can confirm GDB did used to behave
> the way you recall some time ago, do we want to go back to that
> behaviour now?  

No, I don't think we want to go back.

I've thought for a few years that "info breakpoints" should show
BOTH the canonical spec behind each breakpoint, and the actual
location(s) where the breakpoint is inserted.  It wouldn't
be that hard, even.  I cooked up a prototype patch for that.
I'll post it as a follow up.

> And is that a blocker for my change going in?  Yes if
> we did want to go back to the old behaviour then part of my patch
> would probably end up being reverted - as I suspect would
> a9e408182d2faaed5.  I'm happy to keep digging on this to see if I can
> confirm if/when the behaviour changed if that helps bring clarity to
> this issue.

Thanks,
Pedro Alves
  

* 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
  

Changes since v1:

 - There's now two patches, with #1 being a new clean up change,
 - Patch #2 has an additional updated comment.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb: Remove unneeded parameter from set_breakpoint_location_function
  gdb: Don't skip prologue for explicit line breakpoints in assembler

 gdb/ChangeLog                                      | 18 +++++++++++
 gdb/breakpoint.c                                   | 14 ++++-----
 gdb/linespec.c                                     |  3 +-
 gdb/symtab.c                                       | 21 ++++++++-----
 gdb/testsuite/ChangeLog                            |  5 ++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S   | 35 ++++++++++++++++++++++
 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp | 35 ++++++++++++++++++++++
 7 files changed, 113 insertions(+), 18 deletions(-)
 create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.S
 create mode 100644 gdb/testsuite/gdb.arch/amd64-break-on-asm-line.exp
  

On 7/1/19 6:12 PM, Andrew Burgess wrote:
>> 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.

OK, but I was also curious to know whether your patch already handles that
case, or whether we need to set explicit_line somewhere else too.
Maybe it already works if we end up in decode_digits_ordinary too
with the explicit syntax.

Thanks,
Pedro Alves
  

* Pedro Alves <palves@redhat.com> [2019-07-01 19:21:43 +0100]:

> On 7/1/19 6:12 PM, Andrew Burgess wrote:
> >> 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.
> 
> OK, but I was also curious to know whether your patch already handles that
> case, or whether we need to set explicit_line somewhere else too.
> Maybe it already works if we end up in decode_digits_ordinary too
> with the explicit syntax.

Sorry I misunderstood your question.

Yes, if I use 'break -line N' then I do end up in
decode_digits_ordinary, so explicit_line will be set after my patch.

Thanks,
Andrew


> 
> Thanks,
> Pedro Alves