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

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

Simon:

On Thu, 2023-05-11 at 12:01 -0400, Simon Marchi wrote:
> > 

<snip>

> I'd like to help reviewing this, but I don't have much time at the
> moment, so just a few comments on one test to start with.
> 
> > +   This test is used to test the reverse-step and reverse-next
> > instruction
> > +   execution for a source line that contains multiple function
> > calls.  */
> > +
> > +void
> > +func1 ()
> > +{
> > +} // END FUNC1
> 
> Use /* */ for comments, for consistency with the rest of the code
> base.
> 

OK, changed all instances of // comment.

> > +
> > +void
> > +func2 ()
> > +{
> > +} // END FUNC2
> > +
> > +int main ()
> > +{
> > +  int a, b;
> > +  a = 1;
> > +  b = 2;
> > +  func1 (); func2 ();
> > +  a = a + b;     // START REVERSE TEST
> > +}
> > 

<snip>

> > +
> > +require supports_reverse
> > +
> > +# This test uses the gcc no-column-info command which was added in
> > gcc 7.1.
> > +if {![test_compiler_info {gcc-*}]
> > +    || [test_compiler_info {gcc-[1-6]-*}]} {
> > +    return
> > +}
> 
> I would prefer not to filter out by compiler explicitly like that.
> It would be useful for the test to run with other compilers too.

OK, implemented the gdb_compile options.  Moved the specific gcc
version test there.

> 
> > +
> > +proc run_tests {} {
> > +    global srcfile

<snip>

> > +
> > +set srcfile  func-map-to-same-line.c
> > +set executable func-map-to-same-line
> > +
> > +# test with gcc column info enabled
> > +set options [list debug additional_flags=]
> > +
> > +if {[build_executable "failed to prepare" $executable $srcfile
> > $options] == -1}\
> > + {
> > +    return -1
> > +}
> > +
> > +clean_restart $executable
> > +
> > +with_test_prefix "with-column-info" {
> > +    run_tests
> > +}
> 
> So, the above assumes that the compiler generates column-info by
> default, which has not historically been the case for GCC (it started
> to
> emit columns by default with version 8, according to my
> tests).  Other
> compilers may choose to not emit them by default.
> 
> I think it would make sense to make gdb_compile recognize the new
> "column-info" and "no-column-info" options, which would translate to
> the
> right flags for the given compiler.  gdb_compile already handles the
> nitty gritty details of choosing compiler flags for specific compiler
> versions.  This way, individual tests don't contain compiler flags
> that
> are possibly compiler-specific.

OK, I think I have this implemented as suggested.  It does seem to
work.  I added both gcc and clang support.  Per the link to clang which
does have line info options.

I put the new gdb_compile option support in a separate patch, followed
by the updated "Fix reverse stepping multiple.... " patch.

I tested the test case with:

  make check  RUNTESTFLAGS='CC_FOR_TARGET=clang GDB=.../gdb gdb.reverse/func-map-to-same-line.exp' > out

and 

  make check  RUNTESTFLAGS='GDB=.../gdb gdb.reverse/func-map-to-same-line.exp' > out

I looked in the gdb/testsuite/gdb.log file to verify that the compiler
line (gcc or clang) explicitly has the  -gcolumn-info for the first
test and  -gno-column-info for the second test.  The expected number
of success were seen for the test with gcc and clang.  It all looks
like it works.

> 
> > +
> > +#test with gcc column info disabled
> > +set options [list debug additional_flags=-gno-column-info]
> > +
> > +if {[build_executable "failed to prepare" $executable $srcfile
> > $options] == -1}\
> > + {
> > +    return -1
> > +}
> > +
> > +set $executable executable_without_column_info
> > +clean_restart $executable
> > +
> > +with_test_prefix "no-column-info" {
> > +    run_tests
> > +}
> 
> This would probably be a good use for foreach_with_prefix (if you can
> make it work), to make things more compact:
> 
>   foreach_with_prefix with_column_info {yes no} {
>   }
> 
> ... or something like that.

Changed to the foreach_with_prefix.  

Thanks for the review.

Will post version 5 as a series of two patches.

           Carl