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

[Bruno Larsen <blarsen@redhat.com>]

On 6/8/22 12:37, Pedro Alves wrote:
> On 2022-05-26 16:10, Bruno Larsen via Gdb-patches wrote:
>> When running gdb.base/skip-solib.exp, the backtrace tests could fail if
>> the compiler did not emit epilogue information for trivial epilogues,
> 
> I found "emit epilogue information" very confusing FWIW.  When I read that the first time,
> I thought this was talking about unwind info for the prologue after the stack is destroyed,
> and I wondered why not fix clang instead.
> 
> I think the following would be more accurate:
> 
>   "fail with compilers that associate prologue insns with the function's last
>    statement line instead of the function's closing brace".

Yeah, I think this is a better explanation, but s/prologue/epilogue, right?

In the simplest terms, we need one more step to leave a function, because the pop and return instructions are not marked as belonging to the last brace. I tried to make this concept more specific by saying that clang doesn't emit epilogue information, Have I misunderstood or misnamed it?

> 
>> despite the feature being fully functional.  As an example, when testing
>> skipping the function square, the testsuite would show
>>
> 
> 
>> ---
>>   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 \\(\\)\\).*"
> 
> FWIW, I'd remove the "regexp" from the function's name, just call it gdb_step_until.

Alright, but this sounds more like a review of the first patch than this one.

> 
> 
>>   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
> 
> "add epilogue information" here falls in the "very confusing" camp for me.
> 
>> +# 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"
>> +    return
>> +}
> 
> That begs the question -- what if clang changes in the future?  Nobody
> will remember to update this.
> 
> I think it would be better to add a procedure to lib/gdb.exp that detects this
> automatically, if possible.  I.e., with the gdb.base/skip.exp program, note:
> 
>   (gdb) list foo
>   36        return 0;
>   37      }
>   38
>   39      int
>   40      foo ()
>   41      {
>   42        return 0;
>   43      }
>   44
>   45      static void
>   (gdb)
>   46      test_skip (void)
>   47      {
>   48      }
>   49
>   50      static void
>   51      end_test_skip_file_and_function (void)
>   52      {
>   53        abort ();
>   54      }
>   55
>   (gdb)
> 
> when the program is compiled with clang, we get this:
> 
>   (gdb) info line 42
>   Line 42 of "../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x201804 <foo+4> and ends at 0x201810 <test_skip_file_and_function>.
>   (gdb) info line 43
>   Line 43 of "../../../src/gdb/testsuite/gdb.base/skip.c" is at address 0x201830 <test_skip> but contains no code.
> 
> while if the program is compiled with gcc, we get this:
> 
>   (gdb) info line 42
>   Line 42 of "/home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x117d <foo+8> and ends at 0x1182 <foo+13>.
>   (gdb) info line 43
>   Line 43 of "/home/pedro/rocm/gdb/build/gdb/testsuite/../../../src/gdb/testsuite/gdb.base/skip.c" starts at address 0x1182 <foo+13> and ends at 0x1184 <test_skip>.
> 
> 
> Or even with a simple "just main" program, like so:
> 
>   (gdb) list
>   1       int
>   2       main ()
>   3       {
>   4         return 0;
>   5       }
> 
> we get this with gcc:
> 
>   (gdb) info line 4
>   Line 4 of "main.c" starts at address 0x1131 <main+8> and ends at 0x1136 <main+13>.
>   (gdb) info line 5
>   Line 5 of "main.c" starts at address 0x1136 <main+13> and ends at 0x1138.
>   (gdb)
> 
> and this with clang:
> 
>   (gdb) info line 4
>   Line 4 of "main.c" starts at address 0x40111d <main+13> and ends at 0x40111f.
>   (gdb) info line 5
>   Line number 5 is out of range for "main.c".
>   (gdb)
> 
> I think that we can use that to write a caching proc like:
> 
>   # Return true if the compiler emits line information associating prologue insns with
>   # the function's closing brace.  Return false if not, meaning the prologue
>   # associates prologue instructions with function's last line with a statement.
> 
>   gdb_caching_proc have_prologue_line_info {
>       use gdb_simple_compile the simple main program from above
>       
>       use "info line 5", and return false if we get "out of range", otherwise return true.
>   }
> 

This sounds like a good idea, I would only change the default behavior to returning false, unless we saw specifically "start at address.*and ends at". This would make GDB changes more pronounced, as GCC would start having different results.

Cheers!
Bruno Larsen