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

Carl Love via Gdb-patches
2023-05-16T22:55:09Z

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