RE: [PATCH 1/1] testsuite, btrace: update btrace testsuite to test all btrace recording methods

["Metzger, Markus T" <markus.t.metzger@intel.com>]

I agree that the allow_btrace_tests should check whether any btrace recording format
is supported by the target.  This would then leave this part as-is.  For recording format
specific tests, we leave the format-specific check.

Whether we call them allow_btrace_<format>_tests or target_supports_btrace_<format>
I don't have a preference.  We already have allow_btrace_pt_tests, which we need for
require checks, so sticking to that naming convention may make most sense.


>>> +++ b/gdb/testsuite/gdb.btrace/tsx.exp
>>> @@ -15,7 +15,7 @@
>>>   # You should have received a copy of the GNU General Public License
>>>   # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>>
>>> -require allow_btrace_pt_tests allow_tsx_tests
>>> +require allow_btrace_pt_tests allow_tsx_tests target_supports_btrace
>
>Larsen > Do you happen to know why this test only works with pt style btrace?

Speculation is only indicated by PT.  With BTS, we get control flow but no indication
whether the code was executed speculatively inside a TSX region.


>>> --- a/gdb/testsuite/gdb.python/py-record-btrace.exp
>>> +++ b/gdb/testsuite/gdb.python/py-record-btrace.exp
>>> +require allow_btrace_bts_tests allow_btrace_pt_tests allow_python_tests
>>> +require target_supports_btrace
>
>Larsen > In this test you're requiring that both pt and bts methods of
>recording
>are available. Is that really necessary?
>> Also, the target_supports_btrace here is reduntant, since the previous
>line is even more restrictive.
>
>Abdul > "allow_btrace_pt_tests" function is not needed here and will be
>removed in V2.  As mentioned above target_supports_btrace is prerequisite
>before checking if the recording method "bts" or "pt" is supported so that is
>the reason for having it.

Unless the test is recording format specific, we should not require that a particular
recording format is supported.

This will be covered with the changes to allow_btrace_tests discussed above.


>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>> index 9a906f0f349..241aec7b161 100644
>> --- a/gdb/testsuite/lib/gdb.exp
>> +++ b/gdb/testsuite/lib/gdb.exp
>> @@ -4008,17 +4008,25 @@ gdb_caching_proc allow_avx512fp16_tests {} {
>>       return $allow_avx512fp16_tests
>>   }
>>
>> -# Run a test on the target to see if it supports btrace hardware.  Return 1 if
>so,
>> -# 0 if it does not.  Based on 'check_vmx_hw_available' from the GCC
>testsuite.
>> -
>> -gdb_caching_proc allow_btrace_tests {} {
>> -    global srcdir subdir gdb_prompt inferior_exited_re
>> +# Check if btrace is supported on the target.  Return 1 if
>> +# so, 0 if it does not.
>>
>> -    set me "allow_btrace_tests"
>> +gdb_caching_proc target_supports_btrace {} {
>>       if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } {
>> -	verbose "$me:  target does not support btrace, returning 0" 2
>> +	verbose "target_supports_btrace:  target does not support btrace,
>returning 0" 2
>>   	return 0
>>       }
>> +    return 1
>
>As I mentioned previously, I think the proc name shouldn't be changed,
>and the new proc should be called target_supports_btrace with a
>parameter, or target_supports_btrace_method and the parameter.
>
>Also, now you're not  testing further than if the target is an intel
>board. I think it would be better if first you tried the istarget above,
>then checked if either bts or pt is supported using the procs below.
>Since they are caching procs, there's no problem in calling them here
>then calling them again in the test files anyway.
>
>> +}
>> +
>> +# Run a test on the target to see if it supports btrace 'bts' method.  Return
>> +# 1 if so, 0 if it does not.  Based on 'check_vmx_hw_available' from the GCC
>> +# testsuite.
>> +
>> +gdb_caching_proc allow_btrace_bts_tests {} {
>> +    global srcdir subdir gdb_prompt inferior_exited_re
>> +
>> +    set me "allow_btrace_bts_tests"
>>
>>       # Compile a test program.
>>       set src { int main() { return 0; } }
>> @@ -4037,19 +4045,19 @@ gdb_caching_proc allow_btrace_tests {} {
>>       }
>>       # In case of an unexpected output, we return 2 as a fail value.
>>       set allow_btrace_tests 2
>> -    gdb_test_multiple "record btrace" "check btrace support" {
>> -        -re "You can't do that when your target is.*\r\n$gdb_prompt $" {
>> +    gdb_test_multiple "record btrace bts" "check btrace bts support" {
>> +	-re "You can't do that when your target is.*\r\n$gdb_prompt $" {
>>   	    set allow_btrace_tests 0
>> -        }
>> -        -re "Target does not support branch tracing.*\r\n$gdb_prompt $" {
>> +	}
>> +	-re "Target does not support branch tracing.*\r\n$gdb_prompt $" {
>>   	    set allow_btrace_tests 0
>> -        }
>> -        -re "Could not enable branch tracing.*\r\n$gdb_prompt $" {
>> +	}
>> +	-re "Could not enable branch tracing.*\r\n$gdb_prompt $" {
>>   	    set allow_btrace_tests 0
>> -        }
>> -        -re "^record btrace\r\n$gdb_prompt $" {
>> +	}
>> +	-re "^record btrace bts\r\n$gdb_prompt $" {
>>   	    set allow_btrace_tests 1
>> -        }
>> +	}
>>       }
>>       gdb_exit
>>       remote_file build delete $obj
>> @@ -4066,10 +4074,6 @@ gdb_caching_proc allow_btrace_pt_tests {} {
>>       global srcdir subdir gdb_prompt inferior_exited_re
>>
>>       set me "allow_btrace_pt_tests"
>> -    if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } {
>> -	verbose "$me:  target does not support btrace, returning 1" 2
>> -	return 0
>> -    }
>>
>>       # Compile a test program.
>>       set src { int main() { return 0; } }
>> @@ -4112,6 +4116,22 @@ gdb_caching_proc allow_btrace_pt_tests {} {
>>       return $allow_btrace_pt_tests
>>   }
>>
>> +# Wrapper function to check if input btrace method is supported.  Returns
>1
>> +# if it is supported otherwise returns 0.
>> +
>> +proc allow_btrace_tests {method} {
>> +    if {[string match "pt" "${method}"]} {
>> +	return [allow_btrace_pt_tests]
>> +    } elseif {[string match "bts" "${method}"]} {
>> +	return [allow_btrace_bts_tests]
>> +    }
>> +
>> +    verbose -log "warning: unknown btrace recording method '${method}'"
>> +    # Skip test for unknown method name.
>> +    return 0
>> +}
>> +
>> +
>>   # Run a test on the target to see if it supports Aarch64 SVE hardware.
>>   # Return 1 if so, 0 if it does not.  Note this causes a restart of GDB.
>>

We'll end up with almost identical function that only differ in the recording
format that is requested when enabling recording.  It would be better to turn
this around and have one generic function that takes the recording format
as argument and caching functions for the specific recording formats that
call the generic function.

Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928