Re: [PATCH, v2] Fix reverse stepping multiple contiguous PC ranges over the line table

[will schmidt <will_schmidt@vnet.ibm.com>]

On Thu, 2022-03-31 at 14:52 +0100, Luis Machado via Gdb-patches wrote:
> v2:
> - Check if both the line and symtab match for a particular line table entry.
> 
> --
> 
> When running GDB's testsuite on aarch64-linux/Ubuntu 20.04 (also spotted on
> the ppc backend), I noticed some failures in gdb.reverse/solib-precsave.exp
> and gdb.reverse/solib-reverse.exp.
> 
> The failure happens around the following code:
> 
> 38  b[1] = shr2(17);		/* middle part two */
> 40  b[0] = 6;   b[1] = 9;	/* generic statement, end part two */
> 42  shr1 ("message 1\n");	/* shr1 one */
> 
> Normal execution:
> 
> - step from line 38 will land on line 40.
> - step from line 40 will land on line 42.
> 
> Reverse execution:
> 
> - step from line 42 will land on line 40.
> - step from line 40 will land on line 40.
> - step from line 40 will land on line 38.
> 
> The problem here is that line 40 contains two contiguous but distinct
> PC ranges in the line table, like so:
> 
> Line 40 - [0x7ec ~ 0x7f4]
> Line 40 - [0x7f4 ~ 0x7fc]


<snip>


> I'm not particularly happy with how we go back in the ranges (using "pc - 1").
> Feedback would be welcome.

I suppose there could be a loop of some sort that iterates backwards to
a valid line; though I'd think pc - 1 is sufficient? 

> 
> Validated on aarch64-linux/Ubuntu 20.04/18.04. Carl Love has verified that it
> does fix a similar issue on ppc.
> 
> Ubuntu 18.04 doesn't actually run into these failures because the compiler
> doesn't generate distinct PC ranges for the same line.
> 
> I see similar failures on x86_64 in the gdb.reverse tests
> (gdb.reverse/step-reverse.exp and gdb.reverse/step-reverse.exp). Those are
> also fixed by this patch, although Carl's testcase doesn't fail for x86_64.
> 
> There seems to be a corner case where a line table has only one instruction,
> and while stepping that doesn't take the same path through infrun. I think it
> needs some more investigation. Therefore this is a tentative patch for now.


Are you (or Carl) continuing to pursue that edge case? 

Unless you think the contents here would be significantly invalidated, may
be worth moving forrward with this as at least an incremental improvement.  

> 
> Co-authored-by: Carl Love <cel@us.ibm.com>
> ---
>  gdb/infrun.c                                  |  51 +++++++-
>  gdb/symtab.c                                  |  49 ++++++++
>  gdb/symtab.h                                  |  16 +++
>  gdb/testsuite/gdb.reverse/map-to-same-line.c  |  30 +++++
>  .../gdb.reverse/map-to-same-line.exp          | 114 ++++++++++++++++++


<snip>

Code changes all seem reasonable.    Just one comment on the commentary
here. 


> diff --git a/gdb/testsuite/gdb.reverse/map-to-same-line.exp b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> new file mode 100644
> index 00000000000..da407b50e94
> --- /dev/null
> +++ b/gdb/testsuite/gdb.reverse/map-to-same-line.exp
> @@ -0,0 +1,114 @@
> +
> +# The purpose of this test is to create a DWARF line table that says the
> +# two assignment statements are on the same line.  The expect test checks
> +# to make sure gdb reverse steps over each statement individually, i.e.
> +# not over the line containing both assignments.  */
> +
> +load_lib dwarf.exp
> +
> +# This test can only be run on targets which support DWARF-2 and use gas.
> +if {![dwarf2_support]} {
> +    unsupported "dwarf2 support required for this test"
> +    return 0
> +}
> +
> +if [get_compiler_info] {
> +    return -1
> +}
> +
> +# The DWARF assembler requires the gcc compiler.
> +if {!$gcc_compiled} {
> +    unsupported "gcc is required for this test"
> +    return 0
> +}
> +
> +standard_testfile .c .S
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> +    return -1
> +}
> +
> +set asm_file [standard_output_file $srcfile2]
> +Dwarf::assemble $asm_file {
> +    global srcdir subdir srcfile
> +    declare_labels integer_label L
> +
> +    # Find start address and length of program
> +    lassign [function_range main [list ${srcdir}/${subdir}/$srcfile]] \
> +        main_start main_len
> +    set main_end "$main_start + $main_len"
> +
> +    cu {} {
> +        compile_unit {
> +            {language @DW_LANG_C}
> +            {name map-to-same-line.c}
> +            {stmt_list $L DW_FORM_sec_offset}
> +            {low_pc 0 addr}
> +        } {
> +            subprogram {
> +                {external 1 flag}
> +                {name main}
> +                {low_pc $main_start addr}
> +                {high_pc $main_len DW_FORM_data4}
> +            }
> +        }
> +    }
> +
> +    lines {version 2 default_is_stmt 1} L {
> +        include_dir "${srcdir}/${subdir}"
> +	file_name "$srcfile" 1
> +
> +	# Generate the line table program with the assignments to j and k
> +	# are listed on the same source line in the table.
> +	program {
> +	    {DW_LNE_set_address $main_start}
> +            {line [gdb_get_line_number "TAG: main prologue"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address i_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment i"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address j_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment j"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address k_assignment}

THough it's covered in the section comment, a comment here to clarify
 "Force the Dwarf line number for k_assignment to the j assignment here."  may be useful. 
I was explicitly looking for it, and missed it in my first scan through. :-)  



Thanks,
-Will


> +	    {line [gdb_get_line_number "TAG: assignment j"]}
> +	    {DW_LNS_copy}

> +
> +	    {DW_LNE_set_address l_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment l"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_set_address m_assignment}
> +	    {line [gdb_get_line_number "TAG: assignment m"]}
> +	    {DW_LNS_copy}
> +
> +	    {DW_LNE_end_sequence}
> +	}
> +    }
> +}
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} \
> +        [list $srcfile $asm_file] {nodebug} ] } {
> +    return -1
> +}
> +
> +if ![runto_main] {
> +    return -1
> +}
> +
> +if [supports_process_record] {
> +    # Activate process record/replay
> +    gdb_test_no_output "record" "turn on process record"
> +}
> +
> +set break_at_l [gdb_get_line_number "TAG: assignment l" ]
> +
> +gdb_test "tbreak $break_at_l" "Temporary breakpoint .*"  "breakpoint l = 10"
> +
> +gdb_test "continue" "Temporary breakpoint .*" "run to l = 10"
> +# j = 3 and k = 4 are on the same line.  The reverse step stops at j = 3
> +gdb_test "reverse-step" ".* j = 3;.*" "reverse-step to j = 3"
> +gdb_test "reverse-step" ".* i = 1;.*" "reverse-step to i = 1"