Re: [PATCH v3 02/14] Change gdb.base/skip-solib.exp deal with lack of epilogue information

[Bruno Larsen <blarsen@redhat.com>]

Cheers!
Bruno Larsen

On 5/30/22 11:04, Andrew Burgess wrote:
> Bruno Larsen via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
>> When running gdb.base/skip-solib.exp, the backtrace tests could fail if
>> the compiler did not emit epilogue information for trivial epilogues,
>> despite the feature being fully functional.  As an example, when testing
>> skipping the function square, the testsuite would show
>>
>> Breakpoint 1, main () at (...)/binutils-gdb/gdb/testsuite/gdb.base/skip-solib-main.c:5
>> 5         return square(0);
>> (gdb) step
>> 0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
>> (gdb) PASS: gdb.base/skip-solib.exp: ignoring solib file: step
>> bt
>>   #0  0x00007ffff7cef560 in __libc_start_call_main () from /lib64/libc.so.6
>>   #1  0x00007ffff7cef60c in __libc_start_main_impl () from /lib64/libc.so.6
>>   #2  0x0000000000401065 in _start ()
>> (gdb) FAIL: gdb.base/skip-solib.exp: ignoring solib file: bt
>>
>> Which means that the feature is working, the testsuite is just
>> mis-identifying it.  To avoid this problem, the skipped function calls
>> have been sent to a line before `return`, so epilogues won't factor in.
>>
>> This commit has also changed a few hardcoded steps to leave functions to
>> the newly introduced gdb_step_until_regexp to leave those functions.
>> ---
>>
>> No change in v3
>>
>> v2: chagned to use step_until_regexp instead of finish
>>
>> ---
>>   gdb/testsuite/gdb.base/skip-inline.exp   | 18 +++++++++++-------
>>   gdb/testsuite/gdb.base/skip-solib-lib.c  |  3 ++-
>>   gdb/testsuite/gdb.base/skip-solib-main.c |  3 ++-
>>   gdb/testsuite/gdb.base/skip-solib.exp    | 12 ++++++++++--
>>   4 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/gdb/testsuite/gdb.base/skip-inline.exp b/gdb/testsuite/gdb.base/skip-inline.exp
>> index f6e9926b66c..327ea676140 100644
>> --- a/gdb/testsuite/gdb.base/skip-inline.exp
>> +++ b/gdb/testsuite/gdb.base/skip-inline.exp
>> @@ -35,16 +35,20 @@ gdb_test "skip function foo" "Function foo will be skipped when stepping\."
>>   gdb_test "bt" "\\s*\\#0\\s+main.*" "in the main"
>>   gdb_test "step" ".*" "step into baz, since foo will be skipped"
>>   gdb_test "bt" "\\s*\\#0\\s+baz.*" "in the baz, since foo was skipped"
>> -gdb_test "step" ".*" "step in the baz"
>> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz"
>> -gdb_test "step" ".*" "step back to main"
>> +gdb_step_until_regexp ".*x = 0; x = baz \\(foo \\(\\)\\).*"
>>   gdb_test "bt" "\\s*\\#0\\s+main.*" "again in the main"
>>   gdb_test "step" ".*" "step again into baz, since foo will be skipped"
>>   gdb_test "bt" "\\s*\\#0\\s+baz.*" "again in the baz"
>> -gdb_test "step" ".*" "step in the baz, again"
>> -gdb_test "bt" "\\s*\\#0\\s+baz.*" "still in the baz, again"
>> -gdb_test "step" ".*" "step back to main, again"
>> -gdb_test "bt" "\\s*\\#0\\s+main.*" "again back to main"
>> +gdb_step_until_regexp "main \\(\\) at .*" "step back to main, again"
>> +gdb_test "bt" "\\s*\\#0.*main.*" "again back to main"
>> +
>> +# because clang doesn't add epilogue information, having a set number of
> 
> Capital letter - 'Because'.

Fixed.

> 
>> +# steps puts clang more and more out of sync with gcc.  It is unlikely that
>> +# the effort of keeping both outputs will be useful.
>> +if {[test_compiler_info "clang-*"]} {
>> +    untested "Multiple steps are not supported with clang"
> 
> I'd reword this as "Multiple step tests are not supported with clang"
> just to avoid confusion.  Multiple steps are fine, it's the tests
> themselves we're not proposing to support with clang.

good catch! Fixed.

> 
>> +    return
>> +}
>>   
>>   if ![runto_main] {
>>       return
>> diff --git a/gdb/testsuite/gdb.base/skip-solib-lib.c b/gdb/testsuite/gdb.base/skip-solib-lib.c
>> index b2c4d86d703..341f1440a3b 100644
>> --- a/gdb/testsuite/gdb.base/skip-solib-lib.c
>> +++ b/gdb/testsuite/gdb.base/skip-solib-lib.c
>> @@ -7,5 +7,6 @@ int multiply(int a, int b)
>>   
>>   int square(int num)
>>   {
>> -  return multiply(num, num);
>> +  int res = multiply(num, num);
>> +  return res;
>>   }
>> diff --git a/gdb/testsuite/gdb.base/skip-solib-main.c b/gdb/testsuite/gdb.base/skip-solib-main.c
>> index 746bb5f36bb..a3b6d417935 100644
>> --- a/gdb/testsuite/gdb.base/skip-solib-main.c
>> +++ b/gdb/testsuite/gdb.base/skip-solib-main.c
>> @@ -2,5 +2,6 @@ int square(int num);
>>   
>>   int main()
>>   {
>> -  return square(0);
>> +  int s = square(0);
>> +  return s;
>>   }
>> diff --git a/gdb/testsuite/gdb.base/skip-solib.exp b/gdb/testsuite/gdb.base/skip-solib.exp
>> index 0f2ce7e1ad8..8e61725ad1b 100644
>> --- a/gdb/testsuite/gdb.base/skip-solib.exp
>> +++ b/gdb/testsuite/gdb.base/skip-solib.exp
>> @@ -82,7 +82,7 @@ with_test_prefix "ignoring solib file" {
>>       # We shouldn't step into square(), since we skipped skip-solib-lib.c.
>>       #
>>       gdb_test "step" ""
>> -    gdb_test "bt" "#0\\s+main.*"
>> +    gdb_test "bt 1" "#0\\s+main.*"
> 
> I don't object to this change, but it's not necessary.
> 
>>   }
>>   
>>   #
>> @@ -114,5 +114,13 @@ with_test_prefix "ignoring solib function" {
>>       # the last line of square.
>>       #
>>       gdb_test "step" ""
>> -    gdb_test "bt" "#0\\s+square.*"
>> +    gdb_test "bt 1" "#0\\s+square.*" "skipped multiply"
>> +#    gdb_test_multiple "bt 1" "skipped multiply" {
>> +#	-re "#0\\s+square.*" {
>> +#	    pass "skipped multiply"
>> +#	}
>> +#	-re "#0.*main.*" {
>> +#	    pass "skipped multiply"
>> +#	}
>> +#    }
> 
> So, I was worried while looking at this change that we might end up
> removing all the places where we try to step through code like:
> 
>    int square(int num)
>    {
>      return multiply(num, num);
>    }
> 
> Which I think would be a shame.  However, I notice you (accidentally)
> left in some commented out code - and I think we might be able to use
> that.
> 
> What if we reverted the change to the `square` function, and then, for
> this last test did this:
> 
>      gdb_test_multiple "bt 1" "skipped multiply" {
> 	-re "#0\\s+square.*" {
> 	    pass $gdb_test_name
> 	}
> 	-re "#0.*main.*" {
> 	    # Clang doesn't add sufficient epilogue information to
> 	    # allow GDB to stop within square after the call to
> 	    # multiply.  We accept ending up back in main, but only
> 	    # for clang.
> 	    if [test_compiler_info clang*] {
> 		pass $gdb_test_name
> 	    }
> 	}
>      }
> 
> For any compiler that, like gcc, emits some line info for the function
> epilogue, the test passes.  But we let clang pass even though it appears
> to unwind the extra frame.
> 
> Thoughts?

As we spoke off-list, I am of the opinion that tests should only be used to test what they are trying to test. So the ideal solution would be having a test that tries to step through epilogues in different configurations, and we can safely assume that it works for the rest of the tests. I am working in one such test, which I'll send soon.

In the meantime, I'm not opposed to the specific change you proposed, as it is readable and small, so I can also do this if you think it is worth doing.

> 
> Thanks,
> Andrew
>