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:

> On 1/16/25 11:37 AM, Andrew Burgess wrote:
>> 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?
>
> I applied all other suggestions.
>
> This naming came directly from the downstream patch, the point is to 
> parse a range described by a CFI but not by debug information.

Ahh, OK.  Not sure why I didn't grok that meaning yesterday.  Maybe it's
OK to just leave it then?  Or maybe just s/DIEs/debug info/?

>
> What do you think about this wording:
> 'Verify unwinding works based only on CFI information'

Yeah, or that.

Sorry, I think I was just being a bit slow when I read that yesterday!

Thanks,
Andrew

>
> ?
>
>>
>> OK with these fixes.
>>
>> Approved-By: Andrew Burgess <aburgess@redhat.com>
>>
>> Thanks,
>> Andrew
>>
>>> +}
>>> -- 
>>> 2.47.0
>
>
> -- 
> Cheers,
> Guinevere Larsen
> She/Her/Hers