[PATCH 2/2] gdb: Add support for tracking the DWARF line table is-stmt field

[Andrew Burgess]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-02-05 17:55:37 +0000]:

> On 2/5/20 12:37 PM, Andrew Burgess wrote:
> > This commit brings support for the DWARF line table is_stmt field to
> > GDB.  The is_stmt field is used by the compiler when a single source
> > line is split into multiple assembler instructions, especially if the
> > assembler instructions are interleaved with instruction from other
> > source lines.
> > 
> > The compiler will set the is_stmt flag false from some instructions
> > from the source lines, these instructions are not a good place to
> > insert a breakpoint in order to stop at the source line.
> > Instructions which are marked with the is_stmt flag true are a good
> > place to insert a breakpoint for that source line.
> > 
> > Currently GDB ignores all instructions for which is_stmt is false.
> > This is fine in a lot of cases, however, there are some cases where
> > this means the debug experience is not as good as it could be.
> > 
> > Consider stopping at a random instruction, currently this instruction
> > will be attributed to the last line table entry before this point for
> > which is_stmt was true - as these are the only line table entries that
> > GDB tracks.  This can easily be incorrect in code with even a low
> > level of optimisation.
> > 
> > With is_stmt tracking in place, when stopping at a random instruction
> > we now attribute the instruction back to the real source line, even
> > when is_stmt is false for that instruction in the line table.
> > 
> > When inserting breakpoints we still select line table entries for
> > which is_stmt is true, so the breakpoint placing behaviour should not
> > change.
> > 
> > When stepping though code (at the line level, not the instruction
> > level) we will still stop at instruction where is_stmt is true, I
> > think this is more likely to be the desired behaviour.
> > 
> > Instruction stepping is, of course, unchanged, stepping one
> > instruction at a time, but we should now report more accurate line
> > table information with each instruction step.
> > 
> > The original motivation for this work was a patch posted by Bernd
> > here:
> >   https://sourceware.org/ml/gdb-patches/2019-11/msg00792.html
> > 
> > As part of that thread it was suggested that many issues would be
> > resolved if GDB supported line table views, this isn't something I've
> > attempted in this patch, though reading the spec, it seems like this
> > would be a useful feature to support in GDB in the future.  The spec
> > is here:
> >   http://dwarfstd.org/ShowIssue.php?issue=170427.1
> > 
> > And Bernd gives a brief description of the benefits here:
> >   https://sourceware.org/ml/gdb-patches/2020-01/msg00147.html
> > 
> > With that all said, I think that there is benefit to having proper
> > is_stmt support regardless of whether we have views support, so I
> > think we should consider getting this (or something like this) in
> > first, and then building view support on top of this.
> > 
> > The gdb.cp/next-inline.exp test is based off a test proposed by Bernd
> > Edlinger in message:
> >   https://sourceware.org/ml/gdb-patches/2019-12/msg00842.html
> > 
> 
> Thanks Andrew, I played a bit with debugging the gcc internals,
> and I think the debug-experience is good, the problem with step
> over are completely gone, also in the original context.
> 
> Just a few comments on the patch follow below.
> 
> > gdb/ChangeLog:
> > 
> > 	* buildsym-legacy.c (record_line): Pass extra parameter to
> > 	record_line.
> > 	* buildsym.c (buildsym_compunit::record_line): Take an extra
> > 	parameter, reduce duplication in the line table, and record the
> > 	is_stmt flag in the line table.
> > 	* buildsym.h (buildsym_compunit::record_line): Add extra
> > 	parameter.
> > 	* disasm.c (do_mixed_source_and_assembly_deprecated): Ignore
> > 	non-statement lines.
> > 	* dwarf2read.c (dwarf_record_line_1): Add extra parameter, pass
> > 	this to the symtab builder.
> > 	(dwarf_finish_line): Pass extra parameter to dwarf_record_line_1.
> > 	(lnp_state_machine::record_line): Pass a suitable is_stmt flag
> > 	through to dwarf_record_line_1.
> > 	* infrun.c (process_event_stop_test): When stepping, don't stop at
> > 	a non-statement instruction, and only refresh the step info when
> > 	we land in the middle of a line's range.
> 
> did you mean, when we don't land in the middle of a line ?

No, in this case I did write the correct thing.

So GDB had/has some code designed to "improve" the handling of looping
constructs.  I think the idea is to handle cases like this:

  0x100      LN=5
  0x104 <-\
  0x108   |
  0x10c   |  LN=6
  0x110   |
  0x114 --/

So when line 6 branches back to the middle of line 5, we don't stop
(because we are in the middle of line 5), but we do want to stop at
the start of line 6, so we "reset" the starting point back to line 5.

This is done by calling set_step_info in process_event_stop_test, in
infrun.c.

What we actually did though was whenever we were at an address that
didn't exactly match a SAL (in the example above, marked LN=5, LN=6)
we called set_step_info.  This worked fine, at address 0x104 we reset
back to LN=5, same at address 0x108.

However, in the new world things can look different, for example, like
this:

  0x100      LN=5,STMT=1
  0x104
  0x108      LN=6,STMT=0
  0x10c      LN=6,STMT=1
  0x110
  0x114

In this world, when we move forward to 0x100 we don't have a matching
SAL, so we get the SAL for address 0x100.  We can then safely call
set_step_info like we did before, that's fine.  But when we step to
0x108 we do now have a matching SAL, but we don't want to stop yet as
this address is 'STMT=0'.  However, if we call set_step_info then GDB
is going to think we are stepping away from LN=6, and so will refuse
to stop at address 0x10c.

The proposal in this commit is that if we land at an address which
doesn't specifically match a SAL, 0x104, 0x110, 0x114 in the above
example, then we do call set_step_info, but otherwise we don't.

There are going to be situations where the behaviour of GDB changes
after this patch, but I don't think we can avoid that.  What we had
previously was just a heuristic.  I think enabling this new
information in GDB will allow us to do better overall, so I think we
should make this change and fix any issues as they show up.

> 
> > --- a/gdb/stack.c
> > +++ b/gdb/stack.c
> > @@ -330,7 +330,7 @@ frame_show_address (struct frame_info *frame,
> >        return false;
> >      }
> >  
> > -  return get_frame_pc (frame) != sal.pc;
> > +  return get_frame_pc (frame) != sal.pc || !sal.is_stmt;
> >  }
> >  
> >  /* See frame.h.  */
> > @@ -1036,6 +1036,7 @@ print_frame_info (const frame_print_options &fp_opts,
> >    int location_print;
> >    struct ui_out *uiout = current_uiout;
> >  
> > +
> >    if (!current_uiout->is_mi_like_p ()
> >        && fp_opts.print_frame_info != print_frame_info_auto)
> >      {
> 
> Is this white space change intentional?

Ooops.  Fixed.

I also fixed the space before tab issue you pointed out in another
mail.

I see you have a patch that depends on this one, but I'd like to leave
this on the mailing list for a little longer before I merge it to give
others a chance to comment.

I'll probably merge this over the weekend if there's no other
feedback.

Thanks,
Andrew