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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

Sorry, I forgot to send these out on Friday.

Felix 

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 29. Juni 2022 15:36
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v5 10/10] btrace: Extend ptwrite event decoding.
> 
> 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?

My bad. My intention was to initialize every member to 0. Will be
fixed in the next version.

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

Done.

> 
> >+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?

No difference, I did it this way to have a bit of a function-call-history
to show. I renamed them to ptwrite1 and ptwrite2, to not imply
any sizes.
 
> Could we use the intrinsic functions we call out in the documentation
> or would different compilers spell them differently?

I switched to intrinsics. I tested clang, gcc and icx. All the same.
It makes the assembly files a bit bigger. But actually
simplifies the .exp files a bit.
 
> >+### 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"?

Done.
 
> >+# 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.

Actually, there is ebx emitted even for -m64. In my new version,
I had to change it a bit to accept both rax and eax for the mov before.
The ptwrite instruction still is only compiled to eax. Maybe because we
pass an int?

> >+# 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'?

Yes, will be changed.
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