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

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

Hello Abdul,

>With this change, gdb.btrace will, instead of selecting the
>default recording method, run all tests for all available and
>applicable methods. This increases testing coverage.

I reviewed this with 'git show -w'.


>+foreach_with_prefix method {"bts" "pt"} {
>+    if { ![target_supports_btrace $method] } {
>+	unsupported "target does not support record-btrace ${method}"
>+	continue
>+    }

The terminology is 'recording format', not 'recording method'.

In the unsupported string, the $method should already be part of the test prefix.

>+
>+    clean_restart "${testfile}"
>+    if ![runto_main] {
>+	continue
>+    }

We don't need that since the test is going to start over...

>+
>+    # start fresh - without an executable
>+    gdb_exit
>+    gdb_start

...right here.

>+
>+    # record cannot be stopped, if it was never active
>+    gdb_test "record stop" "No recording is currently active\\..*" "record stop
>without target"
>+
>+    # btrace cannot be enabled without a running inferior
>+    gdb_test "record btrace ${method}" "The program is not being run\\."
>"record btrace without running program"
>+
>+    # no function and no instruction history without btrace enabled
>+    gdb_test "record function-call-history" "No recording is currently
>active\\..*" "record function-call-history without target"
>+    gdb_test "record instruction-history" "No recording is currently active\\..*"
>"record instruction-history without target"
>+    gdb_test "info record" "No recording is currently active\\." "info record
>without target"
>+
>+    clean_restart "${testfile}"
>+    if ![runto_main] {
>+	continue
>+    }
>+
>+    # enable btrace
>+    gdb_test_no_output "record btrace ${method}" "record btrace ${method}"

There already is a test prefix supplying $method.  We don't need to repeat that
in the test name.  There are more instances in this file.


>+    # trace the code between the two breakpoints
>+    gdb_continue_to_breakpoint "cont to bp.1" ".*$srcfile:$bp_1\r\n.*"
>+    # increase the BTS buffer size - the trace can be quite big for bts method
>+    gdb_test_no_output "set record btrace ${method} buffer-size 128000"

The comment no longer matches the code.

>+    # moving forward and expect to see the latest 6 entries
>+    gdb_test "record function-call-history /l +" [multi_line \
>+	"\[0-9\]*\tinc\tat $srcfile:22,2\[34\]" \
>+	"\[0-9\]*\tmain\tat $srcfile:40,41" \
>+	"\[0-9\]*\tinc\tat $srcfile:22,2\[34\]" \
>+	"\[0-9\]*\tmain\tat $srcfile:40,41" \
>+	"\[0-9\]*\tinc\tat $srcfile:22,2\[34\]" \
>+	"\[0-9\]*\tmain\tat $srcfile:40,43" \
>+
>+    ] "forward /l - 2"

Why this empty line in the multi_line pattern?


>diff --git a/gdb/testsuite/gdb.btrace/multi-inferior.exp
>b/gdb/testsuite/gdb.btrace/multi-inferior.exp
>index 6996b182e65..06ca93a9686 100644
>--- a/gdb/testsuite/gdb.btrace/multi-inferior.exp
>+++ b/gdb/testsuite/gdb.btrace/multi-inferior.exp

>+	gdb_test_no_output "record btrace ${method}" "record btrace
>${method}"
>+    }

The $method is already part of the test prefix.  We don't need to repeat it
in the test message.  We may be able to just drop the message and use
the default.


>diff --git a/gdb/testsuite/gdb.btrace/reconnect.exp
>b/gdb/testsuite/gdb.btrace/reconnect.exp
>index 41f702a38b3..349d7f7cda9 100644

>-# Test that recording is now off.
>-with_test_prefix "third" {
>-  gdb_test "info record" "No recording is currently active."
>+    # Test that recording is now off.
>+    with_test_prefix "third" {
>+	gdb_test "info record" "No recording is currently active."
>+    }
>+    gdb_test "disconnect" ".*"
> }

Is there an extra

    gdb_test "disconnect" ".*"

at the end?  It shows more clearly with 'git show -w'.


>diff --git a/gdb/testsuite/gdb.btrace/tailcall-only.exp
>b/gdb/testsuite/gdb.btrace/tailcall-only.exp
>index ae3b04e3b66..c90ba8b1bd3 100644
>--- a/gdb/testsuite/gdb.btrace/tailcall-only.exp
>+++ b/gdb/testsuite/gdb.btrace/tailcall-only.exp


>-# We can neither finish nor return.
>-gdb_test "finish" "Cannot find the caller frame.*"
>-gdb_test_multiple "return" "return" {
>-  -re "Make .* return now.*y or n. $" {
>-    send_gdb "y\n"
>-    exp_continue
>-  }

This gets changed from exp_continue...

>+    # We can neither finish nor return.
>+    gdb_test "finish" "Cannot find the caller frame.*"
>+    gdb_test_multiple "return" "return" {
>+	-re "Make .* return now.*y or n. $" {
>+	    send_gdb "y\n"
>+	    continue
>+	}

...to continue.


>diff --git a/gdb/testsuite/gdb.btrace/tsx.exp
>b/gdb/testsuite/gdb.btrace/tsx.exp
>index d312b15027c..11e230c8547 100644
>--- a/gdb/testsuite/gdb.btrace/tsx.exp
>+++ 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 allow_btrace_tests

Do we really need allow_btrace_tests when we already require
allow_btrace_pt_tests?


>diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
>index fe4ac7d2719..62eadc9696f 100644
>--- a/gdb/testsuite/lib/gdb.exp
>+++ b/gdb/testsuite/lib/gdb.exp
>@@ -4008,68 +4008,29 @@ 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.
>+# Check if btrace is supported on the target.  Return 1 if
>+# so, 0 if it does not.
>
> gdb_caching_proc allow_btrace_tests {} {
>-    global srcdir subdir gdb_prompt inferior_exited_re
>-
>-    set me "allow_btrace_tests"
>     if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } {
>-	verbose "$me:  target does not support btrace, returning 0" 2
>-	return 0
>-    }
>-
>-    # Compile a test program.
>-    set src { int main() { return 0; } }
>-    if {![gdb_simple_compile $me $src executable]} {
>+	verbose "target_supports_btrace:  target does not support btrace,
>returning 0" 2

Why 'target_supports_btrace:'?

Since we now check the different recording formats, I don't think this
target check adds anything.

> 	return 0
>     }
>
>-    # No error message, compilation succeeded so now run it via gdb.
>-
>-    gdb_exit
>-    gdb_start
>-    gdb_reinitialize_dir $srcdir/$subdir
>-    gdb_load $obj
>-    if ![runto_main] {
>+    if { ![allow_btrace_bts_tests] && ![allow_btrace_pt_tests] } {
> 	return 0
>     }

The inverted logic may be easier to read and extend, i.e.

if { [allow_btrace_bts_tests] } {
  return 1
}
if { [allow_btrace_pt_tests] } {
  return 1
}
return 0

>-    # 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 $" {
>-	    set allow_btrace_tests 0
>-        }
>-        -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 $" {
>-	    set allow_btrace_tests 0
>-        }
>-        -re "^record btrace\r\n$gdb_prompt $" {
>-	    set allow_btrace_tests 1
>-        }
>-    }
>-    gdb_exit
>-    remote_file build delete $obj
>-
>-    verbose "$me:  returning $allow_btrace_tests" 2
>-    return $allow_btrace_tests
>+    return 1
> }
>
>-# Run a test on the target to see if it supports btrace pt hardware.
>+# Run a test on the target to see if it supports btrace input 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_pt_tests {} {
>+proc check_btrace_method_support {method} {
>     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
>-    }
>+    set me "check_btrace_${method}_support"

Should this be 'check_btrace_method_support ${method}'`?

>
>     # Compile a test program.
>     set src { int main() { return 0; } }
>@@ -4087,31 +4048,62 @@ gdb_caching_proc allow_btrace_pt_tests {} {
> 	return 0
>     }
>     # In case of an unexpected output, we return 2 as a fail value.
>-    set allow_btrace_pt_tests 2
>-    gdb_test_multiple "record btrace pt" "check btrace pt support" {
>-        -re "You can't do that when your target is.*\r\n$gdb_prompt $" {
>-	    set allow_btrace_pt_tests 0
>-        }
>-        -re "Target does not support branch tracing.*\r\n$gdb_prompt $" {
>-	    set allow_btrace_pt_tests 0
>-        }
>-        -re "Could not enable branch tracing.*\r\n$gdb_prompt $" {
>-	    set allow_btrace_pt_tests 0
>-        }
>-        -re "support was disabled at compile time.*\r\n$gdb_prompt $" {
>-	    set allow_btrace_pt_tests 0
>-        }
>-        -re "^record btrace pt\r\n$gdb_prompt $" {
>-	    set allow_btrace_pt_tests 1
>-        }
>+    set supports_method 2
>+    gdb_test_multiple "record btrace ${method}" "check btrace ${method}
>support" {
>+	-re -wrap "You can't do that when your target is.*" {
>+	    set supports_method 0
>+	}
>+	-re -wrap "Target does not support branch tracing.*" {
>+	    set supports_method 0
>+	}
>+	-re -wrap "Could not enable branch tracing.*" {
>+	    set supports_method 0
>+	}
>+	-re -wrap "support was disabled at compile time.*" {
>+	    set supports_method 0
>+	}

We need at least one more fail case:

(gdb) record btrace foo
Undefined record btrace command: "foo".  Try "help record btrace".

>+	-re -wrap "" {
>+	    set supports_method 1
>+	}
>     }
>     gdb_exit
>     remote_file build delete $obj
>
>-    verbose "$me:  returning $allow_btrace_pt_tests" 2
>-    return $allow_btrace_pt_tests
>+    verbose "$me:  returning $supports_method" 2
>+    return $supports_method
> }
>
>+# Run a test on the target to see if it supports btrace 'bts' method.  Return
>+# 1 if so, 0 if it does not.
>+
>+gdb_caching_proc allow_btrace_bts_tests {} {
>+    return [check_btrace_method_support "bts"]
>+}

This 'method' sounds a bit odd.  Without, it would read
'check_btrace_support "bts"'.  We could make it
'btrace_supports "bts"' if we wanted to.

I'm wondering if we even need those allow_btrace_<method>_tests.
Their purpose is to cache the result, but could we declare the underlying
check_btrace_method_support caching?

We'd still want a better name, e.g.

>+proc target_supports_btrace {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.
>
>--
>2.34.1

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