[PATCH 2/2] gdb: Add support for tracking the DWARF line table is-stmt field
Bernd Edlinger
bernd.edlinger@hotmail.de
Wed Feb 5 17:55:00 GMT 2020
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.
More information about the Gdb-patches
mailing list