RE: [PATCH v3 12/12] btrace: Extend ptwrite event decoding.

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

Thanks, Felix,

>+	case ptev_ptwrite:
>+	  {
>+	    uint64_t *ip = nullptr;
>+	    gdb::unique_xmalloc_ptr<char> ptw_string = nullptr;
>+	    struct btrace_insn ptw_insn;
>+	    btrace_insn_flags flags;

We're meanwhile declaring variables where they are initialized and used.  The rest of the code has not been updated but new code should follow this.

>+
>+	    /* Lookup the ip if available.  */
>+	    if (event.ip_suppressed == 0)
>+	      ip = &event.variant.ptwrite.ip;
>+	    else if (!btinfo->functions.empty ()
>+		     && !btinfo->functions.back ().insn.empty ())
>+	      ip = &btinfo->functions.back ().insn.back ().pc;

This isn't necessary; libipt will fill in the IP.  It suffices to check for EVENT.IP_SUPPRESSED.

>+
>+	    if (btinfo->ptw_callback_fun != nullptr)

This is probably the first check we should do, i.e. if it is nullptr, break right at the beginning.

>+	      ptw_string = btinfo->ptw_callback_fun (
>+					  &event.variant.ptwrite.payload, ip,
>+					  btinfo->ptw_listener);

The indentation looks a bit odd.  You may break before '= btinfo->...'  if the line is getting too long, otherwise.

>+
>+	    if (ptw_string == nullptr)
>+	      break;
>+
>+	    btinfo->aux_data.emplace_back (ptw_string.get ());
>+
>+	    if (!btinfo->functions.empty ()
>+		&& !btinfo->functions.back ().insn.empty ())
>+	      flags = btinfo->functions.back ().insn.back ().flags;
>+	    else
>+	      flags = 0;
>+
>+	    /* Update insn list with ptw payload insn.  */

We should probably start by memset()ing ptw_insn to all-zero.

>+If an inferior uses the instruction, @value{GDBN} inserts the raw payload value

Maybe add 'by default' to stress that this behavior can be overwritten, which is documented below.

>+load_lib gdb-python.exp
>+
>+if { [skip_btrace_pt_tests] } {
>+    unsupported "Target does not support record btrace pt."
>+    return -1
>+}
>+
>+if { [skip_btrace_ptw_tests] } {
>+    unsupported "Hardware does not support ptwrite instructions."
>+    return -1
>+}
>+
>+# Test libipt version (must be >= 2.0.0).
>+if {[require_libipt_version 2 0 0]} {
>+    unsupported "Libipt doesn't support ptwrite decoding."
>+    return -1
>+}

We shouldn't need to check the patch version.  See comments on a previous patch regarding feature checking.  We can probably combine it with skip_btrace_ptw_tests.

>+### 1. Default testrun
>+
>+# Setup recording
>+gdb_test_no_output "set record instruction-history-size unlimited" "Default: set
>unlimited"
>+gdb_test_no_output "record btrace pt" "Default: record btrace pt"
>+gdb_test "next" ".*" "Default: first next"
>+gdb_test "next" ".*" "Default: second next"

You may use with_test_prefix to add the "Default: " prefix to the test output.  This groups tests and makes it a bit more readable IMHO.  For the above group, the first two wouldn't need any additional test name string.  And if you combined the latter two into "next 2", it wouldn't either.

>+# Test payload printing during stepping
>+gdb_test "record goto 10" "No such instruction\."
>+gdb_test "record goto 9" ".*ptwrite64.* at .*ptwrite.c:23.*"
>+gdb_test "stepi" ".*\\\[42\\\].*"
>+gdb_test "reverse-stepi" ".*\\\[42\\\].*"
>+gdb_test "continue" ".*\\\[42\\\].*\\\[43\\\].*"
>+gdb_test "reverse-continue" ".*\\\[43\\\].*\\\[42\\\].*"

Between [42] and [43] we wouldn't get any other output we need to ignore, would we?  There's multi_line to capture multiple lines of output.

>+# Test auxiliary type in python
>+gdb_test_multiline "auxiliary type in python" \
>+  "python" "" \
>+  "h = gdb.current_recording().instruction_history" "" \
>+  "for insn in h: print(insn)" "" \
>+  "end" [multi_line \
>+  ".*RecordAuxiliary.*" \
>+  ".*RecordAuxiliary.*" \
>+  ]

Could we inspect the auxiliary records to see if we get the correct strings?  Also, it would be nice to print the surrounding instructions instead of ignoring all the output.

If we just printed the instruction_history, wouldn't we get output very similar to the CLI?


>+### 2. Test listener registration
>+### 2.1 Custom listener
>+
>+gdb_test_multiline "Custom: register listener in python" \
>+  "python" "" \
>+  "def my_listener(payload, ip):" "" \
>+  "    if  payload == 66:" "" \
>+  "        return \"payload: {0}, ip: {1:#x}\".format(payload, ip)" "" \
>+  "    else:" "" \
>+  "        return None" "" \
>+  "import gdb.ptwrite" "" \
>+  "gdb.ptwrite.register_listener(my_listener)" "" \
>+  "end" ""
>+
>+gdb_test "record instruction-history 1" [multi_line \
>+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
>+  "\[0-9\]+\t   \\\[payload: 66, ip: $hex\\\]" \
>+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
>+  "\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:.*" \

What output do we need to ignore here at the beginning of every other line?


>+# Run a test on the target to see if it supports ptwrite instructions.
>+# Return 0 if so, 1 if it does not.  Based on 'check_vmx_hw_available'
>+# from the GCC testsuite.

See above comments.  I think we should simply record a single ptwrite inside main() and check that GDB can decode the trace without errors - and check 'info record' that we actually decoded something.

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