RE: [PATCH v5 10/10] btrace: Extend ptwrite event decoding.

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

Hello Felix,

>Call the ptwrite filter function whenever a ptwrite event is decoded.
>The returned string is written to the aux_data string table and a
>corresponding auxiliary instruction is appended to the function segment.
>---
> gdb/NEWS                                  |   6 +
> gdb/btrace.c                              |  53 +++
> gdb/config.in                             |   3 +
> gdb/configure                             |  11 +
> gdb/doc/python.texi                       | 150 +++++++++
> gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 379 ++++++++++++++++++++++
> gdb/testsuite/gdb.btrace/ptwrite.c        |  37 +++
> gdb/testsuite/gdb.btrace/ptwrite.exp      | 219 +++++++++++++
> gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 374 +++++++++++++++++++++
> gdb/testsuite/lib/gdb.exp                 |  74 +++++
> gdbsupport/common.m4                      |   2 +
> gdbsupport/config.in                      |   3 +
> gdbsupport/configure                      |  11 +
> 13 files changed, 1322 insertions(+)
> create mode 100644 gdb/testsuite/gdb.btrace/i386-ptwrite.S
> create mode 100644 gdb/testsuite/gdb.btrace/ptwrite.c
> create mode 100644 gdb/testsuite/gdb.btrace/ptwrite.exp
> create mode 100644 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S

This looks good to me with a few nits and one test bug.


>+	    if (!btinfo->functions.empty ()
>+		&& !btinfo->functions.back ().insn.empty ())
>+	      flags = btinfo->functions.back ().insn.back ().flags;
>+
>+	    /* Update insn list with ptw payload insn.  */
>+	    struct btrace_insn ptw_insn = { 0 };
>+	    ptw_insn.aux_data_index = btinfo->aux_data.size () - 1;
>+	    ptw_insn.flags = flags;
>+	    ptw_insn.iclass = BTRACE_INSN_AUX;

Why use 0 as IP and not IP?

Let's assign the members in the order in which they are declared.


>+void
>+ptwrite64 (int value)
>+{
>+  asm volatile ("PTWRITE %0;" : : "b" (value));
>+}
>+
>+void
>+ptwrite32 (int value)
>+{
>+  asm volatile ("PTWRITE %0;" : : "b" (value));
>+}

What's the difference between the two functions?

Could we use the intrinsic functions we call out in the documentation
or would different compilers spell them differently?


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

How about "next 2"?


>+# Test auxiliary type in python
>+gdb_test_multiline "auxiliary type in python" \
>+    "python" "" \
>+    "h = gdb.current_recording().instruction_history" "" \
>+    "for insn in h:" "" \
>+    "    if hasattr(insn, 'decoded'):" "" \
>+    "        print(insn.decoded.decode())" "" \
>+    "    elif hasattr(insn, 'data'):" "" \
>+    "        print(insn.data)" "" \
>+    "end" \
>+    [multi_line \
>+	".*mov    %eax,%ebx" \
>+	"ptwrite %ebx" \

We'd want %rbx here, I assume.


>+# Run a test on the target to see if it supports ptwrite instructions and
>+# if GDB can decode ptwrite events.  Return 0 if so, 1 if it does not.
>+
>+gdb_caching_proc skip_btrace_ptw_tests {
>+    global srcdir subdir gdb_prompt inferior_exited_re decimal
>+
>+    set me "skip_ptw_tests"
>+    if { [skip_btrace_pt_tests] } {
>+	verbose "$me:  target does not support btrace, returning 1" 2

Should this say 'target does not support btrace pt'?

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