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

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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 ?

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




Thanks
Bernd.