Re: [PATCH v2 1/1] btrace: Resume recording after disconnect.

[Pedro Alves <palves at redhat dot com>]

Hi Tim,

Thanks for the update.

On 07/19/2016 08:45 AM, Tim Wiederhake wrote:

> diff --git a/gdb/testsuite/gdb.btrace/reconnect.exp b/gdb/testsuite/gdb.btrace/reconnect.exp
> new file mode 100644
> index 0000000..266ecb4
> --- /dev/null
> +++ b/gdb/testsuite/gdb.btrace/reconnect.exp
> @@ -0,0 +1,73 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2016 Free Software Foundation, Inc.
> +#
> +# Contributed by Intel Corp. <tim.wiederhake@intel.com>
> +#
> +# 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/>.
> +
> +load_lib gdbserver-support.exp
> +
> +if { [skip_btrace_tests] } { return -1 }
> +if { [skip_gdbserver_tests] } { return -1 }
> +
> +standard_testfile
> +if [prepare_for_testing $testfile.exp $testfile $srcfile] {
> +    return -1
> +}
> +
> +# Make sure we're disconnected and no recording is active, in case
> +# we're testing with an extended-remote board, therefore already
> +# connected.
> +gdb_test "record stop" ".*" "preparation record stop"
> +gdb_test "disconnect" ".*" "preparation disconnect"
> +
> +# Start fresh gdbserver.
> +set gdbserver_reconnect_p 1
> +set res [gdbserver_start "" $binfile]
> +set gdbserver_protocol [lindex $res 0]
> +set gdbserver_gdbport [lindex $res 1]
> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
> +
> +# Create a record.
> +gdb_test_no_output "record btrace" "record btrace enable" "first record btrace"
> +gdb_test "stepi 19" "0x.* in .* from target.*" "stepi"
> +gdb_test "info record" [multi_line \
> +  "Active record target: .*" \
> +  "Recorded 19 instructions in .+ functions \\(. gaps\\) for thread 1 \\(Thread .*\\)."
> +] "first info record"
> +
> +# Reconnect.
> +gdb_test "disconnect" "Ending remote debugging." "first reconnect"
> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
> +
> +# Test if we can access the recorded data from first connect.
> +# Note: BTS loses the first function call entry with its associated
> +# instructions for technical reasons.  This is why we test for
> +# "a number between 10 and 19", so we catch at least the case where
> +# there are 0 instructions in the record.
> +gdb_test "info record" [multi_line \
> +  "Active record target: .*" \
> +  "Recorded 1. instructions in .+ functions \\(. gaps\\) for thread 1 \\(Thread .*\\)."
> +] "second info record"
> +
> +# Stop recording.
> +gdb_test "record stop" "Process record is stopped and all execution logs are deleted."
> +
> +# Reconnect.
> +gdb_test "disconnect" "Ending remote debugging." "second reconnect"
> +gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport
> +
> +# Test that recording is now off.
> +gdb_test "info record" "No record target is currently active." "third info record"
> 

Looks mostly good to me, but I'd like to point out something about
test grouping.

A better way to group the separate tests of the testcase is not
to put explicit "second", "third", etc. in the test messages but
to use with_test_prefix.  Something like:

with_test_prefix "tracing" {
  // disconnect, reconnect, info record
}

with_test_prefix "not tracing" {
  // record stop, disconnect, reconnect, info record
}

In many cases, this avoids writing an explicit test message
even (third argument of gdb_test), and has one other more important
advantage -- it makes sure that test messages that are emitted deep
within common procedures like gdb_target_cmd (should it fail) end up
with a unique prefix as well.

Otherwise, LGTM.  Markus should have the last word, though.

Thanks,
Pedro Alves