Re: [PATCH v8 5/5] gdb/testsuite: Test for a backtrace through object without debuginfo

[Andrew Burgess <aburgess@redhat.com>]

Guinevere Larsen <guinevere@redhat.com> writes:

> Fedora has been carrying this test since back in the Project Archer
> days. A change back then caused GDB to stop being able to backtrace when
> only some of the object files had debug information. Even though the
> changed code never seems to have made its way into the main GDB project,
> I think it makes sense to bring the test along to ensure something like
> this doesn't pass unnoticed.
>
> Co-Authored-By: Jan Kratochvil <jan@jankratochvil.net>
> Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> ---
>  .../backtrace-through-cu-nodebug-caller.c     | 28 ++++++
>  .../backtrace-through-cu-nodebug-main.c       | 32 +++++++
>  .../gdb.base/backtrace-through-cu-nodebug.exp | 95 +++++++++++++++++++
>  3 files changed, 155 insertions(+)
>  create mode 100644 gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-caller.c
>  create mode 100644 gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-main.c
>  create mode 100644 gdb/testsuite/gdb.base/backtrace-through-cu-nodebug.exp
>
> diff --git a/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-caller.c b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-caller.c
> new file mode 100644
> index 00000000000..3a63d72a468
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-caller.c
> @@ -0,0 +1,28 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2005-2024 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +typedef int (*callback_t) (void);
> +
> +int
> +caller (callback_t callback)
> +{
> +  /* Ensure some frame content to push away the return address.  */
> +  volatile const long one = 1;
> +
> +  /* Modify the return value to prevent any tail-call optimization.  */
> +  return (*callback) () - one;
> +}
> diff --git a/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-main.c b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-main.c
> new file mode 100644
> index 00000000000..3e7ac57a166
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug-main.c
> @@ -0,0 +1,32 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2005-2024 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +typedef int (*callback_t) (void);
> +
> +extern int caller (callback_t callback);
> +
> +int
> +callback (void)
> +{
> +  return 1;
> +}
> +
> +int
> +main (void)
> +{
> +  return caller (callback);
> +}
> diff --git a/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug.exp b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug.exp
> new file mode 100644
> index 00000000000..c0940b406a8
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/backtrace-through-cu-nodebug.exp
> @@ -0,0 +1,95 @@
> +# Copyright 2010-2024 Free Software Foundation, Inc.

Remember to update the copyright year throughout.

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test that GDB can generate accurate backtraces even if some of the stack
> +# trace goes through a function with no debug information.
> +
> +standard_testfile -caller.c -main.c
> +set objmainfile ${testfile}-main.o
> +set objcallerfile ${testfile}-caller.o
> +
> +# recompile the inferior with or without CFI information, then run the
> +# inferior until the point where the important test starts
> +# returns TRUE on an ERROR.

Needs converting to two sentences ('.' after 'starts').  Plus capital
'R' for each sentence.

I wonder if it would be better to return TRUE on success, and FALSE on
error, because .... see below ...

> +proc prepare_test {has_cfi} {
> +    global srcdir subdir srcfile srcfile2 objmainfile objcallerfile binfile
> +    if {$has_cfi} {
> +	set extension "cfi"
> +	if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" \
> +	     "${srcdir}/${subdir}/${objcallerfile}" \
> +	     object [list {additional_flags=-fomit-frame-pointer \
> +		 -funwind-tables -fasynchronous-unwind-tables}]] != "" } {
> +	    untested "couldn't compile with cfi"
> +	    return true
> +      }
> +    } else {
> +	set extension "no-cfi"
> +	if {[gdb_compile "${srcdir}/${subdir}/${srcfile}" \
> +	     "${srcdir}/${subdir}/${objcallerfile}" \
> +	     object [list {additional_flags=-fomit-frame-pointer \
> +		 -fno-unwind-tables \
> +		 -fno-asynchronous-unwind-tables}]] != "" } {
> +	    untested "couldn't compile without cfi"
> +	    return true
> +      }
> +    }
> +    if {[gdb_compile [list "${srcdir}/${subdir}/${objmainfile}" \
> +	    "${srcdir}/${subdir}/${objcallerfile}"] \
> +	    "${binfile}-${extension}" binfile {}] != ""} {
> +	untested "couldn't link object files"
> +	return true
> +    }
> +
> +    clean_restart "$binfile-${extension}"
> +
> +    with_test_prefix "${extension}" {
> +
> +	if ![runto callback] then {
> +	   fail "has_cfi=$has_cfi: Can't run to callback"
> +	   return true
> +	}
> +	gdb_test_no_output "maint frame-unwinder disable ARCH"
> +	return false
> +    }
> +}
> +
> +if {[gdb_compile "${srcdir}/${subdir}/${srcfile2}" \
> +	"${srcdir}/${subdir}/${objmainfile}" \
> +	object {debug}] != "" } {
> +    untested "couldn't compile main file"
> +    return
> +}
> +
> +if { [prepare_test false] } {
> +     untested ${testfile}.exp
> +} else {
> +    gdb_test "bt" "Required frame unwinder may have been disabled.*" \
> +	"verify unwind fail without CFI"
> +}

When something goes wrong with the prepare_test call we've already
emitted an 'untested' or 'fail' message, thus I think we can just write:

 if { [prepare_test false] } {
   ... perform the test ...
 }

Of course, this assumes that the true/false return value for
prepare_test have been switched.  Currently you'd write:

 if { ![prepare_test false] } {
   ... perform the test ...
 }

which just seems weird.

Also, the test here says "verify unwind fail without CFI".  And we _do_
check for the error message about disabled unwinders.  But if the `bt`
worked this test would still pass just fine.  We should probably be
checking that we only print frame #0.  Something like:

    gdb_test "bt" \
	[multi_line \
	     "\[^\r\n\]+Required frame unwinder may have been disabled, \[^\r\n\]+" \
	     "#0\\s+callback \\(\\) \[^\r\n\]+"] \
	"verify unwind fail without CFI"

should do the job I think.

> +
> +if { [prepare_test true] } {
> +     untested ${testfile}.exp
> +} else {

Same suggestion here about avoiding the extra 'untested' call.

> +    if { [istarget "arm*-*-*"] } {
> +	setup_kfail backtrace/31950 *-*-*
> +    }
> +    set text {[^\r\n]+}
> +    # #0  callback () at ...
> +    # #1  0x00000000004004e9 in caller ()
> +    # #2  0x00000000004004cd in main () at ...
> +    gdb_test "bt" \
> +	"#0 +callback $text\r\n#1 $text in caller $text\r\n#2 $text in main $text" \
> +	"verify unwinding works for CFI without DIEs"

The test name here seems weird.  Maybe: 'verify unwinding works for CUs
without CFI' would be better?

OK with these fixes.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew

> +}
> -- 
> 2.47.0