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

[Carl Love <cel@us.ibm.com>]

Bruno:

On Fri, 2022-05-06 at 12:04 -0300, Bruno Larsen wrote:

<snip>

> >  >.
> > +
> > +# The purpose of this test is to create a DWARF line table that
> > contains two
> > +# or more entries for the same line.  When stepping (forwards or
> > backwards),
> > +# GDB should step over the entire line and not just a particular
> > entry in the
> > +# line table.
> > +
> 
> Could you add a comment here after the copyright blurb explaining why
> this testcase exists? Something similar to the find_line_range_start
> comment should be enough, jsut so the next unrelated person who looks
> at this code in 2 years has some context as to what is going on.

OK, updated the comment to say the purpose is to test the
find_line_range_start functionality.  I did reorganize the comment a
bit to make things flow better.  Specifically, started with the
expected behaviour of GDB followed by testing find_line_range_start to
ensure GDB works as expected.

> 
> > +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
> > +}
> 
> There should probably be a test here for supports_reverse. Or
> supports_process_record. I'm not sure what the difference is between
> these 2 checks.

Yes, I added if ![supports_process_record] test here to make sure the
system supports the record and replay.  

<snip>
> 
> > +
> > +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"
> > +}
> since we would have tested above if the target supports
> recording/reversing, we should be good to just use record here,
> without the if clause. I suggest doing it this way because there is
> no point in this whole test if the target doesn't support reverse
> execution.

Yes, removed the supports_process_record test.
> > 

Will post v5 of the patch for review.

Thanks for the input. 

                          Carl Love