public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v5 00/10] Extensions for PTWRITE
@ 2022-06-22 11:43 Felix Willgerodt
  2022-06-22 11:43 ` [PATCH v5 01/10] btrace: Introduce auxiliary instructions Felix Willgerodt
                   ` (9 more replies)
  0 siblings, 10 replies; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

The older revisions can be found here:
V1: https://sourceware.org/pipermail/gdb-patches/2019-May/157933.html
V2: https://sourceware.org/pipermail/gdb-patches/2021-June/179908.html
V3: https://sourceware.org/pipermail/gdb-patches/2021-June/180035.html
v4: https://sourceware.org/pipermail/gdb-patches/2022-May/188772.html

The documentation parts that got approved by Eli didn't change, apart
from a mechanical change that changed ptwrite_listener to ptwrite_filter.

Apart from that I only made some minor changes:
* Changed every mention of ptwrite_listener to ptwrite_filter.
* Made the ptw callback return std::string instead of a 'malloced' char ptr.
* Simplified gdb.exp/skip_btrace_ptw_tests.
* Some minor changes to comments and a couple of other shortcomings in the
the last three patches. The first 7 are unchanged.

Regards,
Felix


Felix Willgerodt (10):
  btrace: Introduce auxiliary instructions.
  btrace: Enable auxiliary instructions in record instruction-history.
  btrace: Enable auxiliary instructions in record function-call-history.
  btrace: Handle stepping and goto for auxiliary instructions.
  python: Introduce gdb.RecordAuxiliary class.
  python: Add clear() to gdb.Record.
  btrace, gdbserver: Add ptwrite to btrace_config_pt.
  btrace, linux: Enable ptwrite packets.
  btrace, python: Enable ptwrite filter registration.
  btrace: Extend ptwrite event decoding.

 gdb/NEWS                                      |   6 +
 gdb/btrace.c                                  |  65 ++-
 gdb/btrace.h                                  |  39 +-
 gdb/config.in                                 |   3 +
 gdb/configure                                 |  11 +
 gdb/data-directory/Makefile.in                |   1 +
 gdb/disasm-flags.h                            |   1 +
 gdb/doc/gdb.texinfo                           |  33 +-
 gdb/doc/python.texi                           | 168 ++++++++
 gdb/extension-priv.h                          |   5 +
 gdb/extension.c                               |  13 +
 gdb/extension.h                               |   3 +
 gdb/features/btrace-conf.dtd                  |   1 +
 gdb/guile/guile.c                             |   1 +
 gdb/nat/linux-btrace.c                        |  29 ++
 gdb/python/lib/gdb/ptwrite.py                 |  86 ++++
 gdb/python/py-record-btrace.c                 | 158 +++++++-
 gdb/python/py-record-btrace.h                 |  11 +
 gdb/python/py-record.c                        |  89 +++-
 gdb/python/py-record.h                        |   3 +
 gdb/python/python-internal.h                  |   3 +
 gdb/python/python.c                           |   2 +
 gdb/record-btrace.c                           | 108 ++++-
 gdb/record.c                                  |  10 +
 gdb/record.h                                  |   5 +-
 gdb/remote.c                                  |  30 ++
 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/gdb.python/py-record-btrace.exp |   6 +-
 gdb/testsuite/lib/gdb.exp                     |  74 ++++
 gdbserver/linux-low.cc                        |   1 +
 gdbserver/server.cc                           |  15 +
 gdbsupport/btrace-common.h                    |   6 +
 gdbsupport/common.m4                          |   2 +
 gdbsupport/config.in                          |   3 +
 gdbsupport/configure                          |  11 +
 38 files changed, 1976 insertions(+), 35 deletions(-)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py
 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

-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 01/10] btrace: Introduce auxiliary instructions.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:10   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history Felix Willgerodt
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Auxiliary instructions are pseudo instructions pointing to auxiliary data.
This auxiliary data can be printed in all commands displaying (record
function-call-history, record instruction-history) or stepping through
(stepi etc.) the execution history, which will be introduced in the next
commits.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/btrace.c        |  2 ++
 gdb/btrace.h        | 24 +++++++++++++++++++++---
 gdb/doc/gdb.texinfo |  4 ++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index c4f0d4911ab..3305ebfb58f 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1823,6 +1823,8 @@ btrace_clear_history (struct btrace_thread_info *btinfo)
   btinfo->insn_history = NULL;
   btinfo->call_history = NULL;
   btinfo->replay = NULL;
+
+  btinfo->aux_data.clear ();
 }
 
 /* Clear the branch trace maintenance histories in BTINFO.  */
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 0fcf669597a..be4ce424ed5 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -52,7 +52,10 @@ enum btrace_insn_class
   BTRACE_INSN_RETURN,
 
   /* The instruction is an unconditional jump.  */
-  BTRACE_INSN_JUMP
+  BTRACE_INSN_JUMP,
+
+  /* The instruction is a pseudo instruction containing auxiliary data.  */
+  BTRACE_INSN_AUX
 };
 
 /* Instruction flags.  */
@@ -68,8 +71,19 @@ DEF_ENUM_FLAGS_TYPE (enum btrace_insn_flag, btrace_insn_flags);
    This represents a single instruction in a branch trace.  */
 struct btrace_insn
 {
-  /* The address of this instruction.  */
-  CORE_ADDR pc;
+  union
+  {
+    /* The address of this instruction.  Applies to btrace_insn with
+       iclass == BTRACE_INSN_OTHER or
+       iclass == BTRACE_INSN_CALL or
+       iclass == BTRACE_INSN_RETURN or
+       iclass == BTRACE_INSN_JUMP.  */
+    CORE_ADDR pc;
+
+    /* Index into btrace_info::aux_data.  Applies to btrace_insn with
+       iclass == BTRACE_INSN_AUX.  */
+    uint64_t aux_data_index;
+  };
 
   /* The size of this instruction in bytes.  */
   gdb_byte size;
@@ -330,6 +344,10 @@ struct btrace_thread_info
      function segment i will be at index (i - 1).  */
   std::vector<btrace_function> functions;
 
+  /* Optional auxiliary information that is printed in all commands
+     displaying or stepping through the execution history.  */
+  std::vector<std::string> aux_data;
+
   /* The function level offset.  When added to each function's LEVEL,
      this normalizes the function levels such that the smallest level
      becomes zero.  */
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 38ae249f62d..6ad3b543f3b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7506,6 +7506,10 @@ Moxie, PowerPC, PowerPC64, S/390, and x86 (i386/amd64) running
 GNU/Linux.  Process record and replay can be used both when native
 debugging, and when remote debugging via @code{gdbserver}.
 
+When recording an inferior, @value{GDBN} may print additional auxiliary
+information during stepping commands and commands displaying the execution
+history.
+
 For architecture environments that support process record and replay,
 @value{GDBN} provides the following commands:
 
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
  2022-06-22 11:43 ` [PATCH v5 01/10] btrace: Introduce auxiliary instructions Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:10   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history Felix Willgerodt
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
is encountered in the instruction-history.  Printing is active by default,
it can be silenced with the /a modifier.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/disasm-flags.h  |  1 +
 gdb/doc/gdb.texinfo |  3 +++
 gdb/record-btrace.c | 14 ++++++++++++++
 gdb/record.c        |  5 +++++
 4 files changed, 23 insertions(+)

diff --git a/gdb/disasm-flags.h b/gdb/disasm-flags.h
index 025b6893941..4c920971d99 100644
--- a/gdb/disasm-flags.h
+++ b/gdb/disasm-flags.h
@@ -33,6 +33,7 @@ enum gdb_disassembly_flag
     DISASSEMBLY_OMIT_PC = (0x1 << 4),
     DISASSEMBLY_SOURCE = (0x1 << 5),
     DISASSEMBLY_SPECULATIVE = (0x1 << 6),
+    DISASSEMBLY_OMIT_AUX_INSN = (0x1 << 7),
   };
 DEF_ENUM_FLAGS_TYPE (enum gdb_disassembly_flag, gdb_disassembly_flags);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6ad3b543f3b..d69fb99cb97 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7922,6 +7922,9 @@ To better align the printed instructions when the trace contains
 instructions from more than one function, the function name may be
 omitted by specifying the @code{/f} modifier.
 
+Printing auxiliary information is enabled by default and can be
+omitted with the @code{/a} modifier.
+
 Speculatively executed instructions are prefixed with @samp{?}.  This
 feature is not available for all recording formats.
 
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 3f8a69dd04f..0d8d1042d53 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -821,6 +821,20 @@ btrace_insn_history (struct ui_out *uiout,
 	  btrace_ui_out_decode_error (uiout, btrace_insn_get_error (&it),
 				      conf->format);
 	}
+      else if (insn->iclass == BTRACE_INSN_AUX)
+	{
+	  if ((flags & DISASSEMBLY_OMIT_AUX_INSN) != 0)
+	    continue;
+
+	  uiout->field_fmt ("insn-number", "%u", btrace_insn_number (&it));
+	  uiout->text ("\t");
+	  uiout->spaces (3);
+	  uiout->text ("[");
+	  uiout->field_fmt (
+	      "aux-data", "%s",
+	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
+	  uiout->text ("]\n");
+	}
       else
 	{
 	  struct disasm_insn dinsn;
diff --git a/gdb/record.c b/gdb/record.c
index 17a5df262bd..94b8c7b7ed3 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -486,6 +486,9 @@ get_insn_history_modifiers (const char **arg)
 
 	  switch (*args)
 	    {
+	    case 'a':
+	      modifiers |= DISASSEMBLY_OMIT_AUX_INSN;
+	      break;
 	    case 'm':
 	    case 's':
 	      modifiers |= DISASSEMBLY_SOURCE;
@@ -853,6 +856,8 @@ With a /m or /s modifier, source lines are included (if available).\n\
 With a /r modifier, raw instructions in hex are included.\n\
 With a /f modifier, function names are omitted.\n\
 With a /p modifier, current position markers are omitted.\n\
+With a /a modifier, omits output of auxiliary data, which is enabled \
+by default.\n\
 With no argument, disassembles ten more instructions after the previous \
 disassembly.\n\
 \"record instruction-history -\" disassembles ten instructions before a \
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
  2022-06-22 11:43 ` [PATCH v5 01/10] btrace: Introduce auxiliary instructions Felix Willgerodt
  2022-06-22 11:43 ` [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:10   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions Felix Willgerodt
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
is encountered in the function-call-history.  Printing is
active by default, it can be silenced with the /a modifier.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/btrace.h        |  6 +++++-
 gdb/doc/gdb.texinfo |  5 +++--
 gdb/record-btrace.c | 21 +++++++++++++++++++++
 gdb/record.c        |  5 +++++
 gdb/record.h        |  5 ++++-
 5 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/gdb/btrace.h b/gdb/btrace.h
index be4ce424ed5..69ce2f3330d 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -105,7 +105,11 @@ enum btrace_function_flag
 
   /* The 'up' link points to a tail call.  This obviously only makes sense
      if bfun_up_links_to_ret is clear.  */
-  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1)
+  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1),
+
+  /* Indicates that at least one auxiliary instruction is in the current
+     function segment.  */
+  BFUN_AUX_DECODED = (1 << 2)
 };
 DEF_ENUM_FLAGS_TYPE (enum btrace_function_flag, btrace_function_flags);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d69fb99cb97..38ad8a3dd79 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7978,8 +7978,9 @@ that function, the source lines for this instruction sequence (if the
 @code{/l} modifier is specified), and the instructions numbers that form
 the sequence (if the @code{/i} modifier is specified).  The function names
 are indented to reflect the call stack depth if the @code{/c} modifier is
-specified.  The @code{/l}, @code{/i}, and @code{/c} modifiers can be given
-together.
+specified.  Printing auxiliary information is enabled by default and can be
+omitted with the @code{/a} modifier.  The @code{/l}, @code{/i}, @code{/a},
+and @code{/c} modifiers can be given together.
 
 @smallexample
 (@value{GDBP}) @b{list 1, 10}
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 0d8d1042d53..ea023ff9a37 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1153,6 +1153,23 @@ btrace_get_bfun_name (const struct btrace_function *bfun)
     return "??";
 }
 
+static void
+btrace_print_aux_insn (struct ui_out *uiout,
+		       const struct btrace_function *bfun,
+		       const struct btrace_thread_info *btinfo)
+{
+  for (const btrace_insn &insn : bfun->insn)
+    {
+      if (insn.iclass == BTRACE_INSN_AUX)
+	{
+	  uiout->text ("\t\t[");
+	  uiout->field_fmt ("aux-data", "%s",
+			    btinfo->aux_data.at (insn.aux_data_index).c_str ());
+	  uiout->text ("]\n");
+	}
+    }
+}
+
 /* Disassemble a section of the recorded function trace.  */
 
 static void
@@ -1228,6 +1245,10 @@ btrace_call_history (struct ui_out *uiout,
 	}
 
       uiout->text ("\n");
+
+      if (((flags & RECORD_DONT_PRINT_AUX) == 0)
+	  && ((bfun->flags & BFUN_AUX_DECODED) != 0))
+	btrace_print_aux_insn(uiout, bfun, btinfo);
     }
 }
 
diff --git a/gdb/record.c b/gdb/record.c
index 94b8c7b7ed3..956475b57c7 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -636,6 +636,9 @@ get_call_history_modifiers (const char **arg)
 	    case 'c':
 	      modifiers |= RECORD_PRINT_INDENT_CALLS;
 	      break;
+	    case 'a':
+	      modifiers |= RECORD_DONT_PRINT_AUX;
+	      break;
 	    default:
 	      error (_("Invalid modifier: %c."), *args);
 	    }
@@ -880,6 +883,8 @@ Without modifiers, it prints the function name.\n\
 With a /l modifier, the source file and line number range is included.\n\
 With a /i modifier, the instruction number range is included.\n\
 With a /c modifier, the output is indented based on the call stack depth.\n\
+With a /a modifier, omits output of auxiliary data, which is enabled \
+by default.\n\
 With no argument, prints ten more lines after the previous ten-line print.\n\
 \"record function-call-history -\" prints ten lines before a previous ten-line \
 print.\n\
diff --git a/gdb/record.h b/gdb/record.h
index 0fbca9d44ce..d882f313c1d 100644
--- a/gdb/record.h
+++ b/gdb/record.h
@@ -62,7 +62,10 @@ enum record_print_flag
   RECORD_PRINT_INSN_RANGE = (1 << 1),
 
   /* Indent based on call stack depth (if applicable).  */
-  RECORD_PRINT_INDENT_CALLS = (1 << 2)
+  RECORD_PRINT_INDENT_CALLS = (1 << 2),
+
+  /* Deactivate printing auxiliary data (if applicable).  */
+  RECORD_DONT_PRINT_AUX = (1 << 3)
 };
 DEF_ENUM_FLAGS_TYPE (enum record_print_flag, record_print_flags);
 
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (2 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:11   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class Felix Willgerodt
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Print the auxiliary data when stepping. Don't allow to goto an auxiliary
instruction.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/record-btrace.c | 68 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 13 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index ea023ff9a37..5cf32088b19 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2375,9 +2375,13 @@ record_btrace_single_step_forward (struct thread_info *tp)
     return btrace_step_stopped ();
 
   /* Skip gaps during replay.  If we end up at a gap (at the end of the trace),
-     jump back to the instruction at which we started.  */
+     jump back to the instruction at which we started.  If we're stepping a
+     BTRACE_INSN_AUX instruction, print the auxiliary data and skip the
+     instruction.  */
+
   start = *replay;
-  do
+
+  for (;;)
     {
       unsigned int steps;
 
@@ -2385,12 +2389,27 @@ record_btrace_single_step_forward (struct thread_info *tp)
 	 of the execution history.  */
       steps = btrace_insn_next (replay, 1);
       if (steps == 0)
+        {
+          *replay = start;
+          return btrace_step_no_history ();
+        }
+
+      const struct btrace_insn *insn = btrace_insn_get (replay);
+      if (insn == nullptr)
+	continue;
+
+      /* If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
+	 data and skip the instruction.  */
+      if (insn->iclass == BTRACE_INSN_AUX)
 	{
-	  *replay = start;
-	  return btrace_step_no_history ();
+	  gdb_printf ("[%s]\n",
+		      btinfo->aux_data.at (insn->aux_data_index).c_str ());
+	  continue;
 	}
+
+      /* We have an instruction, we are done.  */
+      break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Determine the end of the instruction trace.  */
   btrace_insn_end (&end, btinfo);
@@ -2421,9 +2440,12 @@ record_btrace_single_step_backward (struct thread_info *tp)
 
   /* If we can't step any further, we reached the end of the history.
      Skip gaps during replay.  If we end up at a gap (at the beginning of
-     the trace), jump back to the instruction at which we started.  */
+     the trace), jump back to the instruction at which we started.
+     If we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
+     data and skip the instruction.  */
   start = *replay;
-  do
+
+  for (;;)
     {
       unsigned int steps;
 
@@ -2433,8 +2455,22 @@ record_btrace_single_step_backward (struct thread_info *tp)
 	  *replay = start;
 	  return btrace_step_no_history ();
 	}
+
+      const struct btrace_insn *insn = btrace_insn_get (replay);
+      if (insn == nullptr)
+	continue;
+
+      /* Check if we're stepping a BTRACE_INSN_AUX instruction and skip it.  */
+      if (insn->iclass == BTRACE_INSN_AUX)
+	{
+	  gdb_printf ("[%s]\n",
+		      btinfo->aux_data.at (insn->aux_data_index).c_str ());
+	  continue;
+	}
+
+      /* We have an instruction, we are done.  */
+      break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Check if we're stepping a breakpoint.
 
@@ -2856,25 +2892,31 @@ record_btrace_target::goto_record_end ()
 /* The goto_record method of target record-btrace.  */
 
 void
-record_btrace_target::goto_record (ULONGEST insn)
+record_btrace_target::goto_record (ULONGEST insn_number)
 {
   struct thread_info *tp;
   struct btrace_insn_iterator it;
   unsigned int number;
   int found;
 
-  number = insn;
+  number = insn_number;
 
   /* Check for wrap-arounds.  */
-  if (number != insn)
+  if (number != insn_number)
     error (_("Instruction number out of range."));
 
   tp = require_btrace_thread ();
 
   found = btrace_find_insn_by_number (&it, &tp->btrace, number);
 
-  /* Check if the instruction could not be found or is a gap.  */
-  if (found == 0 || btrace_insn_get (&it) == NULL)
+  /* Check if the instruction could not be found or is a gap or an
+     auxilliary instruction.  */
+  if (found == 0)
+    error (_("No such instruction."));
+
+  const struct btrace_insn *insn = btrace_insn_get (&it);
+
+  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
     error (_("No such instruction."));
 
   record_btrace_set_replay (tp, &it);
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (3 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:11   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 06/10] python: Add clear() to gdb.Record Felix Willgerodt
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Auxiliary instructions are no real instructions and get their own object
class, similar to gaps. gdb.Record.instruction_history is now possibly a
list of gdb.RecordInstruction, gdb.RecordGap or gdb.RecordAuxiliary
objects.

This patch is in preparation for the new ptwrite feature, which is based on
auxiliary instructions.
---
 gdb/doc/python.texi           | 13 +++++++
 gdb/python/py-record-btrace.c | 34 ++++++++++------
 gdb/python/py-record.c        | 73 ++++++++++++++++++++++++++++++++++-
 gdb/python/py-record.h        |  3 ++
 4 files changed, 110 insertions(+), 13 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 9e2e97b1e74..0c471416ea6 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3852,6 +3852,19 @@ the current recording method.
 A human readable string with the reason for the gap.
 @end defvar
 
+Some @value{GDBN} features write auxiliary information into the execution
+history.  This information is represented by a @code{gdb.RecordAuxiliary} object
+in the instruction list.  It has the following attributes:
+
+@defvar RecordAuxiliary.number
+An integer identifying this auxiliary.  @code{number} corresponds to the numbers
+seen in @code{record instruction-history} (@pxref{Process Record and Replay}).
+@end defvar
+
+@defvar RecordAuxiliary.data
+A string representation of the auxiliary data.
+@end defvar
+
 A @code{gdb.RecordFunctionSegment} object has the following attributes:
 
 @defvar RecordFunctionSegment.number
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 85401010f0a..e08619ffd64 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -45,7 +45,8 @@ struct btpy_list_object {
   /* Stride size.  */
   Py_ssize_t step;
 
-  /* Either &BTPY_CALL_TYPE or &RECPY_INSN_TYPE.  */
+  /* Either &recpy_func_type, &recpy_insn_type, &recpy_aux_type or
+     &recpy_gap_type.  */
   PyTypeObject* element_type;
 };
 
@@ -141,10 +142,11 @@ btrace_func_from_recpy_func (const PyObject * const pyobject)
 }
 
 /* Looks at the recorded item with the number NUMBER and create a
-   gdb.RecordInstruction or gdb.RecordGap object for it accordingly.  */
+   gdb.RecordInstruction, gdb.RecordGap or gdb.RecordAuxiliary object
+   for it accordingly.  */
 
 static PyObject *
-btpy_insn_or_gap_new (thread_info *tinfo, Py_ssize_t number)
+btpy_item_new (thread_info *tinfo, Py_ssize_t number)
 {
   btrace_insn_iterator iter;
   int err_code;
@@ -163,6 +165,13 @@ btpy_insn_or_gap_new (thread_info *tinfo, Py_ssize_t number)
       return recpy_gap_new (err_code, err_string, number);
     }
 
+  const struct btrace_insn *insn = btrace_insn_get (&iter);
+  gdb_assert (insn != nullptr);
+
+  if (insn->iclass == BTRACE_INSN_AUX)
+    return recpy_aux_new (
+	iter.btinfo->aux_data.at (insn->aux_data_index).c_str (), number);
+
   return recpy_insn_new (tinfo, RECORD_METHOD_BTRACE, number);
 }
 
@@ -441,8 +450,10 @@ btpy_list_length (PyObject *self)
 }
 
 /* Implementation of
-   BtraceList.__getitem__ (self, key) -> BtraceInstruction and
-   BtraceList.__getitem__ (self, key) -> BtraceFunctionCall.  */
+   BtraceList.__getitem__ (self, key) -> BtraceInstruction,
+   BtraceList.__getitem__ (self, key) -> BtraceFunctionCall,
+   BtraceList.__getitem__ (self, key) -> BtraceAuxilliary and
+   BtraceList.__getitem__ (self, key) -> BtraceGap.  */
 
 static PyObject *
 btpy_list_item (PyObject *self, Py_ssize_t index)
@@ -456,10 +467,10 @@ btpy_list_item (PyObject *self, Py_ssize_t index)
 
   number = obj->first + (obj->step * index);
 
-  if (obj->element_type == &recpy_insn_type)
-    return recpy_insn_new (obj->thread, RECORD_METHOD_BTRACE, number);
-  else
+  if (obj->element_type == &recpy_func_type)
     return recpy_func_new (obj->thread, RECORD_METHOD_BTRACE, number);
+  else
+    return btpy_item_new (obj->thread, number);
 }
 
 /* Implementation of BtraceList.__getitem__ (self, slice) -> BtraceList.  */
@@ -646,8 +657,7 @@ recpy_bt_replay_position (PyObject *self, void *closure)
   if (tinfo->btrace.replay == NULL)
     Py_RETURN_NONE;
 
-  return btpy_insn_or_gap_new (tinfo,
-			       btrace_insn_number (tinfo->btrace.replay));
+  return btpy_item_new (tinfo, btrace_insn_number (tinfo->btrace.replay));
 }
 
 /* Implementation of
@@ -669,7 +679,7 @@ recpy_bt_begin (PyObject *self, void *closure)
     Py_RETURN_NONE;
 
   btrace_insn_begin (&iterator, &tinfo->btrace);
-  return btpy_insn_or_gap_new (tinfo, btrace_insn_number (&iterator));
+  return btpy_item_new (tinfo, btrace_insn_number (&iterator));
 }
 
 /* Implementation of
@@ -691,7 +701,7 @@ recpy_bt_end (PyObject *self, void *closure)
     Py_RETURN_NONE;
 
   btrace_insn_end (&iterator, &tinfo->btrace);
-  return btpy_insn_or_gap_new (tinfo, btrace_insn_number (&iterator));
+  return btpy_item_new (tinfo, btrace_insn_number (&iterator));
 }
 
 /* Implementation of
diff --git a/gdb/python/py-record.c b/gdb/python/py-record.c
index 51084dfac72..47bbd290144 100644
--- a/gdb/python/py-record.c
+++ b/gdb/python/py-record.c
@@ -49,6 +49,12 @@ static PyTypeObject recpy_gap_type = {
   PyVarObject_HEAD_INIT (NULL, 0)
 };
 
+/* Python RecordAuxiliary type.  */
+
+PyTypeObject recpy_aux_type = {
+  PyVarObject_HEAD_INIT (nullptr, 0)
+};
+
 /* Python RecordGap object.  */
 struct recpy_gap_object
 {
@@ -64,6 +70,18 @@ struct recpy_gap_object
   Py_ssize_t number;
 };
 
+/* Python RecordAuxiliary object.  */
+typedef struct
+{
+  PyObject_HEAD
+
+  /* Auxiliary data.  */
+  const char *data;
+
+  /* Element number.  */
+  Py_ssize_t number;
+} recpy_aux_object;
+
 /* Implementation of record.method.  */
 
 static PyObject *
@@ -477,6 +495,43 @@ recpy_gap_reason_string (PyObject *self, void *closure)
   return PyUnicode_FromString (obj->reason_string);
 }
 
+/* Create a new gdb.Auxiliary object.  */
+
+PyObject *
+recpy_aux_new (const char *data, Py_ssize_t number)
+{
+  recpy_aux_object * const obj = PyObject_New (recpy_aux_object,
+					       &recpy_aux_type);
+
+  if (obj == nullptr)
+   return nullptr;
+
+  obj->data = data;
+  obj->number = number;
+
+  return (PyObject *) obj;
+}
+
+/* Implementation of Auxiliary.number [int].  */
+
+static PyObject *
+recpy_aux_number (PyObject *self, void *closure)
+{
+  const recpy_aux_object * const obj = (const recpy_aux_object *) self;
+
+  return gdb_py_object_from_longest (obj->number).release ();
+}
+
+/* Implementation of Auxiliary.data [str].  */
+
+static PyObject *
+recpy_aux_data (PyObject *self, void *closure)
+{
+  const recpy_aux_object * const obj = (const recpy_aux_object *) self;
+
+  return PyUnicode_FromString (obj->data);
+}
+
 /* Record method list.  */
 
 static PyMethodDef recpy_record_methods[] = {
@@ -542,6 +597,14 @@ static gdb_PyGetSetDef recpy_gap_getset[] = {
   { NULL }
 };
 
+/* RecordAuxiliary member list.  */
+
+static gdb_PyGetSetDef recpy_aux_getset[] = {
+  { "number", recpy_aux_number, nullptr, "element number", nullptr},
+  { "data", recpy_aux_data, nullptr, "data", nullptr},
+  { nullptr }
+};
+
 /* Sets up the record API in the gdb module.  */
 
 int
@@ -581,10 +644,18 @@ gdbpy_initialize_record (void)
   recpy_gap_type.tp_doc = "GDB recorded gap object";
   recpy_gap_type.tp_getset = recpy_gap_getset;
 
+  recpy_aux_type.tp_new = PyType_GenericNew;
+  recpy_aux_type.tp_flags = Py_TPFLAGS_DEFAULT;
+  recpy_aux_type.tp_basicsize = sizeof (recpy_aux_object);
+  recpy_aux_type.tp_name = "gdb.RecordAuxiliary";
+  recpy_aux_type.tp_doc = "GDB recorded auxiliary object";
+  recpy_aux_type.tp_getset = recpy_aux_getset;
+
   if (PyType_Ready (&recpy_record_type) < 0
       || PyType_Ready (&recpy_insn_type) < 0
       || PyType_Ready (&recpy_func_type) < 0
-      || PyType_Ready (&recpy_gap_type) < 0)
+      || PyType_Ready (&recpy_gap_type) < 0
+      || PyType_Ready (&recpy_aux_type) < 0)
     return -1;
   else
     return 0;
diff --git a/gdb/python/py-record.h b/gdb/python/py-record.h
index 2c8cfd6e4a5..d1d784e758e 100644
--- a/gdb/python/py-record.h
+++ b/gdb/python/py-record.h
@@ -71,4 +71,7 @@ extern PyObject *recpy_func_new (thread_info *thread, enum record_method method,
 extern PyObject *recpy_gap_new (int reason_code, const char *reason_string,
 				Py_ssize_t number);
 
+/* Create a new gdb.RecordGap object.  */
+extern PyObject *recpy_aux_new (const char *data, Py_ssize_t number);
+
 #endif /* PYTHON_PY_RECORD_H */
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 06/10] python: Add clear() to gdb.Record.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (4 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:11   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt Felix Willgerodt
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

This function allows to clear the trace data from python, forcing to
re-decode the trace for successive commands.
---
 gdb/doc/python.texi                           |  5 +++++
 gdb/python/py-record-btrace.c                 | 13 +++++++++++++
 gdb/python/py-record-btrace.h                 |  3 +++
 gdb/python/py-record.c                        | 16 ++++++++++++++++
 gdb/testsuite/gdb.python/py-record-btrace.exp |  6 +++++-
 5 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 0c471416ea6..a25a49fcb8a 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3796,6 +3796,11 @@ A @code{gdb.Record} object has the following methods:
 Move the replay position to the given @var{instruction}.
 @end defun
 
+@defun Record.clear ()
+Clear the trace data of the current recording.  This forces re-decoding of the
+trace for successive commands.
+@end defun
+
 The common @code{gdb.Instruction} class that recording method specific
 instruction objects inherit from, has the following attributes:
 
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index e08619ffd64..8b3ae8c3fab 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -801,6 +801,19 @@ recpy_bt_goto (PyObject *self, PyObject *args)
   Py_RETURN_NONE;
 }
 
+/* Implementation of BtraceRecord.clear (self) -> None.  */
+
+PyObject *
+recpy_bt_clear (PyObject *self, PyObject *args)
+{
+  const recpy_record_object * const record = (recpy_record_object *) self;
+  thread_info *const tinfo = record->thread;
+
+  btrace_clear (tinfo);
+
+  Py_RETURN_NONE;
+}
+
 /* BtraceList methods.  */
 
 static PyMethodDef btpy_list_methods[] =
diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-btrace.h
index 8ef26c860db..b25b121d255 100644
--- a/gdb/python/py-record-btrace.h
+++ b/gdb/python/py-record-btrace.h
@@ -31,6 +31,9 @@ extern PyObject *recpy_bt_format (PyObject *self, void *closure);
 /* Implementation of record.goto (instruction) -> None.  */
 extern PyObject *recpy_bt_goto (PyObject *self, PyObject *value);
 
+/* Implementation of BtraceRecord.clear (self) -> None.  */
+extern PyObject *recpy_bt_clear (PyObject *self, PyObject *args);
+
 /* Implementation of record.instruction_history [list].  */
 extern PyObject *recpy_bt_instruction_history (PyObject *self, void *closure);
 
diff --git a/gdb/python/py-record.c b/gdb/python/py-record.c
index 47bbd290144..0aef0e0ae1b 100644
--- a/gdb/python/py-record.c
+++ b/gdb/python/py-record.c
@@ -127,6 +127,19 @@ recpy_goto (PyObject *self, PyObject *value)
   return PyErr_Format (PyExc_NotImplementedError, _("Not implemented."));
 }
 
+/* Implementation of record.clear () -> None.  */
+
+static PyObject *
+recpy_clear (PyObject *self, PyObject *value)
+{
+  const recpy_record_object * const obj = (recpy_record_object *) self;
+
+  if (obj->method == RECORD_METHOD_BTRACE)
+    return recpy_bt_clear (self, value);
+
+  return PyErr_Format (PyExc_NotImplementedError, _("Not implemented."));
+}
+
 /* Implementation of record.replay_position [instruction]  */
 
 static PyObject *
@@ -538,6 +551,9 @@ static PyMethodDef recpy_record_methods[] = {
   { "goto", recpy_goto, METH_VARARGS,
     "goto (instruction|function_call) -> None.\n\
 Rewind to given location."},
+  { "clear", recpy_clear, METH_VARARGS,
+    "clear () -> None.\n\
+Clears the trace."},
   { NULL }
 };
 
diff --git a/gdb/testsuite/gdb.python/py-record-btrace.exp b/gdb/testsuite/gdb.python/py-record-btrace.exp
index e9fdd06154e..82d887dbd18 100644
--- a/gdb/testsuite/gdb.python/py-record-btrace.exp
+++ b/gdb/testsuite/gdb.python/py-record-btrace.exp
@@ -88,7 +88,11 @@ with_test_prefix "instruction " {
     gdb_test "python print(repr(i.data))" "<memory at $hex>"
     gdb_test "python print(i.decoded)" ".*"
     gdb_test "python print(i.size)" "$decimal"
-    gdb_test "python print(i.is_speculative)" "False"
+    gdb_test "python print(i.is_speculative)" "False"    
+    gdb_test_no_output "python r.clear()"
+    gdb_test "python insn = r.instruction_history"
+    gdb_test_no_output "python i = insn\[0\]"
+    gdb_test "python print(i.size)" "$decimal"
 }
 
 with_test_prefix "function call" {
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (5 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 06/10] python: Add clear() to gdb.Record Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:11   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 08/10] btrace, linux: Enable ptwrite packets Felix Willgerodt
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

This enables gdb and gdbserver to communicate about ptwrite support.  If
ptwrite support would be enabled unconditionally, GDBs with older libipt
versions would break.
---
 gdb/btrace.c                 |  7 ++++++-
 gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
 gdb/features/btrace-conf.dtd |  1 +
 gdb/remote.c                 | 30 ++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc       |  1 +
 gdbserver/server.cc          | 15 +++++++++++++++
 gdbsupport/btrace-common.h   |  6 ++++++
 7 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 3305ebfb58f..0f5e73f35c4 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -2273,7 +2273,7 @@ parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
 			  std::vector<gdb_xml_value> &attributes)
 {
   struct btrace_config *conf;
-  struct gdb_xml_value *size;
+  struct gdb_xml_value *size, *ptwrite;
 
   conf = (struct btrace_config *) user_data;
   conf->format = BTRACE_FORMAT_PT;
@@ -2282,10 +2282,15 @@ parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
   size = xml_find_attribute (attributes, "size");
   if (size != NULL)
     conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
+
+  ptwrite = xml_find_attribute (attributes, "ptwrite");
+  if (ptwrite != nullptr)
+    conf->pt.ptwrite = (bool) *(ULONGEST *) ptwrite->value.get ();
 }
 
 static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
   { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { "ptwrite", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, nullptr },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 38ad8a3dd79..fa6e3eb2f73 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -42808,6 +42808,11 @@ These are the currently defined stub features and their properties:
 @tab @samp{-}
 @tab Yes
 
+@item @samp{Qbtrace-conf:pt:ptwrite}
+@tab Yes
+@tab @samp{-}
+@tab Yes
+
 @item @samp{QNonStop}
 @tab No
 @tab @samp{-}
@@ -43119,6 +43124,9 @@ The remote stub understands the @samp{Qbtrace-conf:bts:size} packet.
 @item Qbtrace-conf:pt:size
 The remote stub understands the @samp{Qbtrace-conf:pt:size} packet.
 
+@item Qbtrace-conf:pt:ptwrite
+The remote stub understands the @samp{Qbtrace-conf:pt:ptwrite} packet.
+
 @item swbreak
 The remote stub reports the @samp{swbreak} stop reason for memory
 breakpoints.
@@ -43620,6 +43628,18 @@ The ring buffer size has been set.
 A badly formed request or an error was encountered.
 @end table
 
+@item Qbtrace-conf:pt:ptwrite=@var{value}
+Control recording of ptwrite packets.  This allows for backwards-
+compatibility.
+
+Reply:
+@table @samp
+@item OK
+The ptwrite config parameter has been set.
+@item E.errtext
+A badly formed request or an error was encountered.
+@end table
+
 @end table
 
 @node Architecture-Specific Protocol Details
@@ -46262,6 +46282,7 @@ The formal DTD for the branch trace configuration format is given below:
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
 @end smallexample
 
 @include agentexpr.texi
diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
index c71b11f2087..e8cf32adf02 100644
--- a/gdb/features/btrace-conf.dtd
+++ b/gdb/features/btrace-conf.dtd
@@ -12,3 +12,4 @@
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
diff --git a/gdb/remote.c b/gdb/remote.c
index ed834228829..a8aa559ca4e 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2204,6 +2204,9 @@ enum {
   /* Support for the Qbtrace-conf:pt:size packet.  */
   PACKET_Qbtrace_conf_pt_size,
 
+  /* Support for the Qbtrace-conf:pt:ptwrite packet.  */
+  PACKET_Qbtrace_conf_pt_ptwrite,
+
   /* Support for exec events.  */
   PACKET_exec_event_feature,
 
@@ -5422,6 +5425,8 @@ static const struct protocol_feature remote_protocol_features[] = {
     PACKET_exec_event_feature },
   { "Qbtrace-conf:pt:size", PACKET_DISABLE, remote_supported_packet,
     PACKET_Qbtrace_conf_pt_size },
+  { "Qbtrace-conf:pt:ptwrite", PACKET_DISABLE, remote_supported_packet,
+    PACKET_Qbtrace_conf_pt_ptwrite },
   { "vContSupported", PACKET_DISABLE, remote_supported_packet, PACKET_vContSupported },
   { "QThreadEvents", PACKET_DISABLE, remote_supported_packet, PACKET_QThreadEvents },
   { "no-resumed", PACKET_DISABLE, remote_supported_packet, PACKET_no_resumed },
@@ -14106,6 +14111,28 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
 
       rs->btrace_config.pt.size = conf->pt.size;
     }
+
+  packet = &remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite];
+  if (packet_config_support (packet) == PACKET_ENABLE
+      && conf->pt.ptwrite != rs->btrace_config.pt.ptwrite)
+    {
+      pos = buf;
+      pos += xsnprintf (pos, endbuf - pos, "%s=%d", packet->name,
+                        conf->pt.ptwrite);
+
+      putpkt (buf);
+      getpkt (&rs->buf, 0);
+
+      if (packet_ok (buf, packet) == PACKET_ERROR)
+	{
+	  if (buf[0] == 'E' && buf[1] == '.')
+	    error (_("Failed to sync ptwrite config: %s"), buf + 2);
+	  else
+	    error (_("Failed to sync ptwrite config."));
+	}
+
+      rs->btrace_config.pt.ptwrite = conf->pt.ptwrite;
+    }
 }
 
 /* Read TP's btrace configuration from the target and store it into CONF.  */
@@ -15328,6 +15355,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_size],
        "Qbtrace-conf:pt:size", "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite],
+       "Qbtrace-conf:pt:ptwrite", "btrace-conf-pt-ptwrite", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vContSupported],
 			 "vContSupported", "verbose-resume-supported", 0);
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 8b8614f6ed4..35cd76d9fe3 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6798,6 +6798,7 @@ linux_process_target::read_btrace_conf (const btrace_target_info *tinfo,
 	case BTRACE_FORMAT_PT:
 	  buffer_xml_printf (buffer, "<pt");
 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
 	  buffer_xml_printf (buffer, "/>\n");
 	  break;
 	}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index f9c02a9c6da..d46e9633cc7 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -542,6 +542,20 @@ handle_btrace_conf_general_set (char *own_buf)
 
       current_btrace_conf.pt.size = (unsigned int) size;
     }
+  else if (strncmp (op, "pt:ptwrite=", strlen ("pt:ptwrite=")) == 0)
+    {
+      char *endp = nullptr;
+
+      errno = 0;
+      int ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);
+      if (endp == nullptr || *endp != 0 || errno != 0 || ptwrite > 1)
+	{
+	  strcpy (own_buf, "E.Bad ptwrite value.");
+	  return -1;
+	}
+
+      current_btrace_conf.pt.ptwrite = (bool) ptwrite;
+    }
   else
     {
       strcpy (own_buf, "E.Bad Qbtrace configuration option.");
@@ -2162,6 +2176,7 @@ supported_btrace_packets (char *buf)
   strcat (buf, ";Qbtrace-conf:bts:size+");
   strcat (buf, ";Qbtrace:pt+");
   strcat (buf, ";Qbtrace-conf:pt:size+");
+  strcat (buf, ";Qbtrace-conf:pt:ptwrite+");
   strcat (buf, ";Qbtrace:off+");
   strcat (buf, ";qXfer:btrace:read+");
   strcat (buf, ";qXfer:btrace-conf:read+");
diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
index 3c55ae92185..96d11d77d33 100644
--- a/gdbsupport/btrace-common.h
+++ b/gdbsupport/btrace-common.h
@@ -117,6 +117,12 @@ struct btrace_config_pt
      This is unsigned int and not size_t since it is registered as
      control variable for "set record btrace pt buffer-size".  */
   unsigned int size;
+
+  /* Configuration bit for ptwrite packets.
+
+     If this is set, gdb will try to enable ptwrite packets if the OS
+     supports this.  */
+  bool ptwrite;
 };
 
 /* A branch tracing configuration.
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 08/10] btrace, linux: Enable ptwrite packets.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (6 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28  9:12   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration Felix Willgerodt
  2022-06-22 11:43 ` [PATCH v5 10/10] btrace: Extend ptwrite event decoding Felix Willgerodt
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

Enable ptwrite in the PT config, if it is supported by the kernel.
---
 gdb/nat/linux-btrace.c | 29 +++++++++++++++++++++++++++++
 gdb/record-btrace.c    |  5 +++++
 2 files changed, 34 insertions(+)

diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
index b0d6dcd7cf1..53f1a468bcc 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -415,6 +415,29 @@ cpu_supports_bts (void)
     }
 }
 
+/* Check whether the linux target supports Intel Processor Trace PTWRITE.  */
+
+static bool
+linux_supports_ptwrite ()
+{
+  static const char filename[]
+      = "/sys/bus/event_source/devices/intel_pt/caps/ptwrite";
+  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
+
+  if (file.get () == nullptr)
+    return false;
+
+  int status, found = fscanf (file.get (), "%d", &status);
+
+  if (found != 1)
+    {
+      warning (_("Failed to determine ptwrite support from %s."), filename);
+      return false;
+    }
+
+  return status == 1;
+}
+
 /* The perf_event_open syscall failed.  Try to print a helpful error
    message.  */
 
@@ -624,6 +647,12 @@ linux_enable_pt (ptid_t ptid, const struct btrace_config_pt *conf)
   pt->attr.exclude_hv = 1;
   pt->attr.exclude_idle = 1;
 
+  if (conf->ptwrite && linux_supports_ptwrite ())
+    {
+      pt->attr.config |= 0x1000;
+      tinfo->conf.pt.ptwrite = conf->ptwrite;
+    }
+
   errno = 0;
   scoped_fd fd (syscall (SYS_perf_event_open, &pt->attr, pid, -1, -1, 0));
   if (fd.get () < 0)
diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 5cf32088b19..b573506b754 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -3289,4 +3289,9 @@ to see the actual buffer size."), NULL, show_record_pt_buffer_size_value,
 
   record_btrace_conf.bts.size = 64 * 1024;
   record_btrace_conf.pt.size = 16 * 1024;
+#if (LIBIPT_VERSION >= 0x200)
+  record_btrace_conf.pt.ptwrite = true;
+#else
+  record_btrace_conf.pt.ptwrite = false;
+#endif
 }
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (7 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 08/10] btrace, linux: Enable ptwrite packets Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-28 13:59   ` Metzger, Markus T
  2022-06-22 11:43 ` [PATCH v5 10/10] btrace: Extend ptwrite event decoding Felix Willgerodt
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

With this patch a default ptwrite filter is registered upon start of GDB.
It prints the plain ptwrite payload as hex.  The default filter can be
overwritten by registering a custom filter in python or by registering
"None", for no output at all.  Registering a filter function creates per
thread copies to allow unique internal states per thread.
---
 gdb/btrace.c                   |   3 +
 gdb/btrace.h                   |   9 +++
 gdb/data-directory/Makefile.in |   1 +
 gdb/extension-priv.h           |   5 ++
 gdb/extension.c                |  13 ++++
 gdb/extension.h                |   3 +
 gdb/guile/guile.c              |   1 +
 gdb/python/lib/gdb/ptwrite.py  |  86 +++++++++++++++++++++++++
 gdb/python/py-record-btrace.c  | 111 +++++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.h  |   8 +++
 gdb/python/python-internal.h   |   3 +
 gdb/python/python.c            |   2 +
 12 files changed, 245 insertions(+)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 0f5e73f35c4..d8c4b77bbc5 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -34,6 +34,7 @@
 #include "gdbsupport/rsp-low.h"
 #include "gdbcmd.h"
 #include "cli/cli-utils.h"
+#include "extension.h"
 #include "gdbarch.h"
 
 /* For maintenance commands.  */
@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
   uint64_t offset;
   int status;
 
+  apply_ext_lang_ptwrite_filter (btinfo);
+
   for (;;)
     {
       struct pt_insn insn;
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 69ce2f3330d..ccb87419757 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -352,6 +352,15 @@ struct btrace_thread_info
      displaying or stepping through the execution history.  */
   std::vector<std::string> aux_data;
 
+  /* Function pointer to the ptwrite callback.  Returns the string returned
+     by the ptwrite filter function or nullptr if no string is supposed to
+     be printed.  */
+  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
+				   const void *ptw_filter);
+
+  /* Function pointer to the ptwrite filter function.  */
+  void *ptw_filter = nullptr;
+
   /* The function level offset.  When added to each function's LEVEL,
      this normalizes the function levels such that the smallest level
      becomes zero.  */
diff --git a/gdb/data-directory/Makefile.in b/gdb/data-directory/Makefile.in
index cf5226f3961..506c16af4b5 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -75,6 +75,7 @@ PYTHON_FILE_LIST = \
 	gdb/frames.py \
 	gdb/printing.py \
 	gdb/prompt.py \
+	gdb/ptwrite.py \
 	gdb/styling.py \
 	gdb/types.py \
 	gdb/unwinder.py \
diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
index 7c74e721c57..b3bed3c88ab 100644
--- a/gdb/extension-priv.h
+++ b/gdb/extension-priv.h
@@ -183,6 +183,11 @@ struct extension_language_ops
      enum ext_lang_frame_args args_type,
      struct ui_out *out, int frame_low, int frame_high);
 
+  /* Used for registering the ptwrite filter to the current thread.  */
+  void (*apply_ptwrite_filter)
+    (const struct extension_language_defn *extlang,
+     struct btrace_thread_info *btinfo);
+
   /* Update values held by the extension language when OBJFILE is discarded.
      New global types must be created for every such value, which must then be
      updated to use the new types.
diff --git a/gdb/extension.c b/gdb/extension.c
index 5a805bea00e..0f6e3e02f0e 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -550,6 +550,19 @@ apply_ext_lang_frame_filter (struct frame_info *frame,
   return EXT_LANG_BT_NO_FILTERS;
 }
 
+/* Used for registering the ptwrite filter to the current thread.  */
+
+void
+apply_ext_lang_ptwrite_filter (btrace_thread_info *btinfo)
+{
+  for (const struct extension_language_defn *extlang : extension_languages)
+    {
+      if (extlang->ops != nullptr
+	  && extlang->ops->apply_ptwrite_filter != nullptr)
+	extlang->ops->apply_ptwrite_filter (extlang, btinfo);
+    }
+}
+
 /* Update values held by the extension language when OBJFILE is discarded.
    New global types must be created for every such value, which must then be
    updated to use the new types.
diff --git a/gdb/extension.h b/gdb/extension.h
index 47839ea50be..bd05d61cd50 100644
--- a/gdb/extension.h
+++ b/gdb/extension.h
@@ -295,6 +295,9 @@ extern enum ext_lang_bt_status apply_ext_lang_frame_filter
    enum ext_lang_frame_args args_type,
    struct ui_out *out, int frame_low, int frame_high);
 
+extern void apply_ext_lang_ptwrite_filter
+  (struct btrace_thread_info *btinfo);
+
 extern void preserve_ext_lang_values (struct objfile *, htab_t copied_types);
 
 extern const struct extension_language_defn *get_breakpoint_cond_ext_lang
diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
index 14b191ded62..86f92a476af 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -124,6 +124,7 @@ static const struct extension_language_ops guile_extension_ops =
   gdbscm_apply_val_pretty_printer,
 
   NULL, /* gdbscm_apply_frame_filter, */
+  NULL, /* gdbscm_load_ptwrite_listener, */
 
   gdbscm_preserve_values,
 
diff --git a/gdb/python/lib/gdb/ptwrite.py b/gdb/python/lib/gdb/ptwrite.py
new file mode 100644
index 00000000000..d1944f5cdc2
--- /dev/null
+++ b/gdb/python/lib/gdb/ptwrite.py
@@ -0,0 +1,86 @@
+# Ptwrite utilities.
+# Copyright (C) 2022 Free Software Foundation, Inc.
+
+# 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/>.
+
+"""Utilities for working with ptwrite filters."""
+
+import gdb
+from copy import deepcopy
+
+
+def default_filter(payload, ip):
+    """Default filter that is active upon starting GDB."""
+    return "{:x}".format(payload)
+
+# This dict contains the per thread copies of the filter function and the
+# global template filter, from which the copies are created.
+_ptwrite_filter = {"global" : default_filter}
+
+
+def _update_filter_dict(thread_list):
+    """Helper function to update the filter dict.
+
+    Discards filter copies of threads that already exited and registers
+    copies of the filter for new threads."""
+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
+    lwp_list = [i.ptid[1] for i in thread_list]
+
+    # clean-up old filters
+    for key in _ptwrite_filter.keys():
+      if key not in lwp_list and key != "global":
+        _ptwrite_filter.pop(key)
+
+    # Register filter for new threads
+    for key in lwp_list:
+        if key not in _ptwrite_filter.keys():
+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
+
+
+def _clear_traces(thread_list):
+    """Helper function to clear the trace of all threads in THREAD_LIST."""
+    current_thread = gdb.selected_thread()
+
+    recording = gdb.current_recording()
+
+    if (recording is not None):
+        for thread in thread_list:
+            thread.switch()
+            recording.clear()
+
+        current_thread.switch()
+
+
+def register_filter(filter):
+    """Register the ptwrite filter function."""
+    if filter is not None and not callable(filter):
+        raise TypeError("filter must be callable!")
+
+    # Clear the traces of all threads to force re-decoding with
+    # the new filter.
+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
+    _clear_traces(thread_list)
+
+    _ptwrite_filter.clear()
+    _ptwrite_filter["global"] = filter
+
+    _update_filter_dict(thread_list)
+
+
+def get_filter():
+    """Returns the filters of the current thread."""
+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
+    _update_filter_dict(thread_list)
+
+    return _ptwrite_filter[gdb.selected_thread().ptid[1]]
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 8b3ae8c3fab..80c2871cf2a 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -762,6 +762,117 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
   return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
 }
 
+/* Helper function that calls the ptwrite filter PTW_FILTER with
+   PAYLOAD and IP as arguments.  Returns a pointer to the string that will
+   be printed or nullptr if nothing should be printed.  IP can be nullptr,
+   PAYLOAD must point to a valid integer.  */
+std::string
+recpy_call_filter (const uint64_t payload, const uint64_t ip,
+		   const void *ptw_filter)
+{
+  std::string result;
+
+  if ((PyObject *) ptw_filter == Py_None)
+    return result;
+  else if ((PyObject *) ptw_filter == nullptr)
+    error (_("No valid ptwrite filter."));
+
+  /* As Python is started as a seperate thread, we need to
+     acquire the GIL to safely call the filter function.  */
+  PyGILState_STATE gstate = PyGILState_Ensure ();
+
+  PyObject *py_payload = PyLong_FromUnsignedLongLong (payload);
+  PyObject *py_ip;
+
+  if (ip == 0)
+    {
+      py_ip = Py_None;
+      Py_INCREF (Py_None);
+    }
+  else
+    py_ip = PyLong_FromUnsignedLongLong (ip);
+
+  PyObject *py_result = PyObject_CallFunctionObjArgs ((PyObject *) ptw_filter,
+						      py_payload, py_ip,
+						      nullptr);
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      Py_DECREF (py_ip);
+      Py_DECREF (py_payload);
+      PyGILState_Release (gstate);
+      error (_("Error while executing Python code."));
+    }
+
+  Py_DECREF (py_ip);
+  Py_DECREF (py_payload);
+
+  if (py_result == Py_None)
+    {
+      Py_DECREF (py_result);
+      PyGILState_Release (gstate);
+      return result;
+    }
+
+  result = gdbpy_obj_to_string (py_result).get ();
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      Py_DECREF (py_result);
+      PyGILState_Release (gstate);
+      error (_("Error while executing Python code."));
+    }
+
+  Py_DECREF (py_result);
+  PyGILState_Release (gstate);
+
+  return result;
+}
+
+/* Helper function returning the current ptwrite filter.  Returns nullptr
+   in case of errors.  */
+
+PyObject *
+get_ptwrite_filter ()
+{
+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
+
+  if (PyErr_Occurred ())
+  {
+    gdbpy_print_stack ();
+    return nullptr;
+  }
+
+  PyObject *ptw_filter = PyObject_CallMethod (module,
+					      "get_filter",
+					      nullptr);
+
+  Py_DECREF (module);
+
+  if (PyErr_Occurred ())
+    gdbpy_print_stack ();
+
+  return ptw_filter;
+}
+
+/* Used for registering the default ptwrite filter to the current thread.  A
+   pointer to this function is stored in the python extension interface.  */
+
+void
+gdbpy_load_ptwrite_filter (const struct extension_language_defn *extlang,
+			   struct btrace_thread_info *btinfo)
+{
+  if (!gdb_python_initialized || btinfo == nullptr)
+    return;
+
+  gdbpy_enter enter_py;
+
+  btinfo->ptw_callback_fun = &recpy_call_filter;
+  btinfo->ptw_filter = get_ptwrite_filter ();
+}
+
 /* Implementation of BtraceRecord.goto (self, BtraceInstruction) -> None.  */
 
 PyObject *
diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-btrace.h
index b25b121d255..9b7fa369007 100644
--- a/gdb/python/py-record-btrace.h
+++ b/gdb/python/py-record-btrace.h
@@ -91,4 +91,12 @@ extern PyObject *recpy_bt_func_prev (PyObject *self, void *closure);
 /* Implementation of RecordFunctionSegment.next [RecordFunctionSegment].  */
 extern PyObject *recpy_bt_func_next (PyObject *self, void *closure);
 
+/* Helper function returning the current ptwrite filter.  */
+extern PyObject *get_ptwrite_filter ();
+
+/* Callback function for the ptwrite filter.  */
+extern std::string recpy_call_filter (const uint64_t payload,
+				      const uint64_t ip,
+				      const void *ptw_filter);
+
 #endif /* PYTHON_PY_RECORD_BTRACE_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 5ff9989af83..0bf1360d7f7 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -359,6 +359,9 @@ extern enum ext_lang_rc gdbpy_apply_val_pretty_printer
    struct ui_file *stream, int recurse,
    const struct value_print_options *options,
    const struct language_defn *language);
+extern void gdbpy_load_ptwrite_filter
+  (const struct extension_language_defn *extlang,
+   struct btrace_thread_info *btinfo);
 extern enum ext_lang_bt_status gdbpy_apply_frame_filter
   (const struct extension_language_defn *,
    struct frame_info *frame, frame_filter_flags flags,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index 8f526bba84e..e538b62931a 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -151,6 +151,8 @@ static const struct extension_language_ops python_extension_ops =
 
   gdbpy_apply_frame_filter,
 
+  gdbpy_load_ptwrite_filter,
+
   gdbpy_preserve_values,
 
   gdbpy_breakpoint_has_cond,
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v5 10/10] btrace: Extend ptwrite event decoding.
  2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
                   ` (8 preceding siblings ...)
  2022-06-22 11:43 ` [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration Felix Willgerodt
@ 2022-06-22 11:43 ` Felix Willgerodt
  2022-06-29 13:35   ` Metzger, Markus T
  9 siblings, 1 reply; 32+ messages in thread
From: Felix Willgerodt @ 2022-06-22 11:43 UTC (permalink / raw)
  To: gdb-patches, markus.t.metzger

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

diff --git a/gdb/NEWS b/gdb/NEWS
index 5576c355b7a..b6b2417b360 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 12
 
+* GDB now supports printing of ptwrite payloads from the Intel Processor
+  Trace during 'record instruction-history', 'record function-call-history',
+  all stepping commands and in  Python.  Printing is customizable via a
+  ptwrite filter function in  Python.  By default, the raw ptwrite
+  payload is printed for each ptwrite that is encountered.
+
 * "info breakpoints" now displays enabled breakpoint locations of
   disabled breakpoints as in the "y-" state.  For example:
 
diff --git a/gdb/btrace.c b/gdb/btrace.c
index d8c4b77bbc5..b5e18412458 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1253,6 +1253,53 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 		   bfun->insn_offset - 1, offset);
 
 	  break;
+#if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
+	case ptev_ptwrite:
+	  {
+	    uint64_t ip = 0;
+	    std::string ptw_string;
+	    btrace_insn_flags flags = 0;
+
+	    /* Lookup the ip if available.  */
+	    if (event.ip_suppressed == 0)
+	      ip = event.variant.ptwrite.ip;
+
+	    if (btinfo->ptw_callback_fun != nullptr)
+	      ptw_string
+		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
+					    ip, btinfo->ptw_filter);
+
+	    if (ptw_string.empty ())
+	      break;
+
+	    btinfo->aux_data.emplace_back (ptw_string);
+
+	    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;
+
+	    if (ip != 0)
+	      bfun = ftrace_update_function (btinfo, ip);
+	    else
+	      {
+		if (btinfo->functions.empty ())
+		  bfun = ftrace_new_function (btinfo, NULL, NULL);
+		else
+		  bfun = &btinfo->functions.back ();
+	      }
+
+	    bfun->flags |= BFUN_AUX_DECODED;
+	    ftrace_update_insns (bfun, ptw_insn);
+
+	    break;
+	  }
+#endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
 	}
     }
 #endif /* defined (HAVE_PT_INSN_EVENT) */
@@ -2979,6 +3026,12 @@ pt_print_packet (const struct pt_packet *packet)
     case ppt_mnt:
       gdb_printf (("mnt %" PRIx64 ""), packet->payload.mnt.payload);
       break;
+
+#if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
+    case ppt_ptw:
+      gdb_printf (("ptw %" PRIx64 ""), packet->payload.ptw.payload);
+      break;
+#endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
     }
 }
 
diff --git a/gdb/config.in b/gdb/config.in
index a4975c68aad..d572c51ec79 100644
--- a/gdb/config.in
+++ b/gdb/config.in
@@ -460,6 +460,9 @@
 /* Define to 1 if `pl_tdname' is a member of `struct ptrace_lwpinfo'. */
 #undef HAVE_STRUCT_PTRACE_LWPINFO_PL_TDNAME
 
+/* Define to 1 if `variant.ptwrite' is a member of `struct pt_event'. */
+#undef HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE
+
 /* Define to 1 if `enabled' is a member of `struct pt_insn'. */
 #undef HAVE_STRUCT_PT_INSN_ENABLED
 
diff --git a/gdb/configure b/gdb/configure
index 1b821390801..7e2d23c361e 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -15327,6 +15327,17 @@ cat >>confdefs.h <<_ACEOF
 _ACEOF
 
 
+fi
+
+      ac_fn_c_check_member "$LINENO" "struct pt_event" "variant.ptwrite" "ac_cv_member_struct_pt_event_variant_ptwrite" "#include <intel-pt.h>
+"
+if test "x$ac_cv_member_struct_pt_event_variant_ptwrite" = xyes; then :
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE 1
+_ACEOF
+
+
 fi
 
       LIBS=$save_LIBS
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index a25a49fcb8a..b1d3a0587c0 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -6997,6 +6997,7 @@ registering objfile-specific pretty-printers and frame-filters.
 * gdb.printing::       Building and registering pretty-printers.
 * gdb.types::          Utilities for working with types.
 * gdb.prompt::         Utilities for prompt value substitution.
+* gdb.ptwrite::        Utilities for ptwrite filter registration.
 @end menu
 
 @node gdb.printing
@@ -7187,3 +7188,152 @@ substitute_prompt ("frame: \f, args: \p@{print frame-arguments@}")
 "frame: main, args: scalars"
 @end smallexample
 @end table
+
+@node gdb.ptwrite
+@subsubsection gdb.ptwrite
+@cindex gdb.ptwrite
+
+This module provides additional functionality for recording programs that
+make use of the @code{PTWRITE} instruction.  @code{PTWRITE} is a x86
+instruction that allows to write values into the Intel Processor Trace
+(@pxref{Process Record and Replay}).
+The @value{NGCC} built-in functions for it are:
+@smallexample
+void __builtin_ia32_ptwrite32 (unsigned);
+void __builtin_ia32_ptwrite64 (unsigned long long);
+@end smallexample
+
+If an inferior uses the instruction, @value{GDBN} by default inserts the
+raw payload value as auxiliary information into the execution history.
+Auxiliary information is by default printed during
+@code{record instruction-history}, @code{record function-call-history},
+and all stepping commands and is accessible in Python as a
+@code{RecordAuxiliary} object.
+
+@exdent Sample program:
+@smallexample
+@group
+void
+ptwrite64 (unsigned long long value)
+@{
+  __builtin_ia32_ptwrite64 (value);
+@}
+@end group
+
+@group
+int
+main (void)
+@{
+  ptwrite64 (0x42);
+  return 0; /* break here.  */
+@}
+@end group
+@end smallexample
+
+
+@exdent @value{GDBN} output after recording the sample program in pt format:
+@smallexample
+@group
+(gdb) record instruction-history 12,14
+12         0x0040074c <ptwrite64+16>:   ptwrite %rbx
+13         [42]
+14         0x00400751 <ptwrite64+21>:   mov -0x8(%rbp),%rbx
+(gdb) record function-call-history
+1       main
+2       ptwrite64
+                [42]
+3       main
+@end group
+@end smallexample
+
+The @code{gdb.ptwrite} module allows customizing the default output of ptwrite
+auxiliary information.  A custom Python function can be registered via
+@code{gdb.ptwrite.register_filter} as the ptwrite filter function.
+This function will be called with the ptwrite payload and PC as arguments
+during trace decoding.
+
+@findex gdb.ptwrite.register_filter
+@defun register_filter (@var{filter})
+Used to register the ptwrite filter.  The filter can be any callable
+object that accepts two arguments, the payload and PC.  It can return
+a string, which will be printed by @value{GDBN} during the aforementioned
+commands, or @code{None}, resulting in no output.  @code{None} can also be
+registered to deactivate printing.
+@end defun
+
+@findex gdb.ptwrite.get_filter
+@defun get_filter ()
+Returns the currently active ptwrite filter function.
+@end defun
+
+@findex gdb.ptwrite.default_filter
+@defun default_filter (@var{payload}, @var{ip})
+The filter function active by default.  It prints the payload in hexadecimal
+format.
+@end defun
+
+@value{GDBN} creates a new copy of the filter function for each thread to
+allow for independent internal states.  There is no support for registering
+different filters for different threads.  The filter can however
+distinguish between multiple threads with the help of
+@code{gdb.selected_thread().global_num} (@pxref{Threads In Python}) or
+similar.  For example:
+
+@smallexample
+@group
+(gdb) python-interactive
+>>> class my_filter():
+...    def __init__(self):
+...        self.var = 0
+...    def __call__(self, payload, ip):
+...        if gdb.selected_thread().global_num == 1:
+...            self.var += 1
+...            return f"counter: @{self.var@}, ip: @{ip:#x@}"
+...        else:
+...            return None
+...
+>>> import gdb.ptwrite
+>>> gdb.ptwrite.register_filter(my_filter())
+>>>
+@end group
+
+@group
+(gdb) record function-call-history 59,64
+59      pthread_create@@GLIBC_2.2.5
+60      job()
+61      task(void*)
+62      ptwrite64(unsigned long)
+                [counter: 1, ip: 0x401156]
+63      task(void*)
+64      ptwrite32(unsigned int)
+                [counter: 2, ip: 0x40116c]
+@end group
+
+@group
+(gdb) info threads 
+* 1    Thread 0x7ffff7fd8740 (LWP 25796) "ptw_threads" task ()
+    at bin/ptwrite/ptw_threads.c:45
+  2    Thread 0x7ffff6eb8700 (LWP 25797) "ptw_threads" task ()
+    at bin/ptwrite/ptw_threads.c:45
+@end group
+
+@group
+(gdb) thread 2
+[Switching to thread 2 (Thread 0x7ffff6eb8700 (LWP 25797))]
+#0  task (arg=0x0) at ptwrite_threads.c:45
+45        return NULL;
+@end group
+
+@group
+(gdb) record function-call-history 10,14
+10    start_thread
+11    task(void*)
+12    ptwrite64(unsigned long)
+13    task(void*)
+14    ptwrite32(unsigned int)
+@end group
+@end smallexample
+
+This @value{GDBN} feature is dependent on hardware and operating system
+support and requires the Intel Processor Trace decoder library in version
+2.0.0 or newer.
diff --git a/gdb/testsuite/gdb.btrace/i386-ptwrite.S b/gdb/testsuite/gdb.btrace/i386-ptwrite.S
new file mode 100644
index 00000000000..90eb0d5eaa1
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/i386-ptwrite.S
@@ -0,0 +1,379 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <markus.t.metzger@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/>.
+
+
+   This file has been generated using gcc version 10.3.1 20210422
+   (Red Hat 10.3.1-1):
+   gcc -S -dA -g -m32 ptwrite.c -o i386-ptwrite.S.  */
+
+	.file	"ptwrite.c"
+	.text
+.Ltext0:
+	.globl	ptwrite64
+	.type	ptwrite64, @function
+ptwrite64:
+.LFB0:
+	.file 1 "ptwrite.c"
+	# ptwrite.c:20:1
+	.loc 1 20 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	.cfi_offset 3, -12
+	# ptwrite.c:21:3
+	.loc 1 21 3
+	movl	8(%ebp), %eax
+	movl	%eax, %ebx
+#APP
+# 21 "ptwrite.c" 1
+	PTWRITE %ebx;
+# 0 "" 2
+	# ptwrite.c:22:1
+	.loc 1 22 1
+#NO_APP
+	nop
+	movl	-4(%ebp), %ebx
+	leave
+	.cfi_restore 5
+	.cfi_restore 3
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	ptwrite64, .-ptwrite64
+	.globl	ptwrite32
+	.type	ptwrite32, @function
+ptwrite32:
+.LFB1:
+	# ptwrite.c:26:1
+	.loc 1 26 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	.cfi_offset 3, -12
+	# ptwrite.c:27:3
+	.loc 1 27 3
+	movl	8(%ebp), %eax
+	movl	%eax, %ebx
+#APP
+# 27 "ptwrite.c" 1
+	PTWRITE %ebx;
+# 0 "" 2
+	# ptwrite.c:28:1
+	.loc 1 28 1
+#NO_APP
+	nop
+	movl	-4(%ebp), %ebx
+	leave
+	.cfi_restore 5
+	.cfi_restore 3
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	ptwrite32, .-ptwrite32
+	.globl	main
+	.type	main, @function
+main:
+.LFB2:
+	# ptwrite.c:32:1
+	.loc 1 32 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	.cfi_offset 5, -8
+	movl	%esp, %ebp
+	.cfi_def_cfa_register 5
+	# ptwrite.c:33:3
+	.loc 1 33 3
+	pushl	$66
+	call	ptwrite64
+	addl	$4, %esp
+	# ptwrite.c:34:3
+	.loc 1 34 3
+	pushl	$67
+	call	ptwrite32
+	addl	$4, %esp
+	# ptwrite.c:36:10
+	.loc 1 36 10
+	movl	$0, %eax
+	# ptwrite.c:37:1
+	.loc 1 37 1
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE2:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x87	# Length of Compilation Unit Info
+	.value	0x4	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF1	# DW_AT_producer: "GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -m32 -mtune=generic -march=i686 -g"
+	.byte	0xc	# DW_AT_language
+	.long	.LASF2	# DW_AT_name: "ptwrite.c"
+	.long	.LASF3	# DW_AT_comp_dir: "/root/gdb/gdb/testsuite/gdb.btrace"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0-.Ltext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF4	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	0x3b	# DW_AT_type
+	.long	.LFB2	# DW_AT_low_pc
+	.long	.LFE2-.LFB2	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_tail_call_sites
+	.uleb128 0x3	# (DIE (0x3b) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x4	# (DIE (0x42) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF5	# DW_AT_name: "ptwrite32"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x19	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	.LFB1	# DW_AT_low_pc
+	.long	.LFE1-.LFB1	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0x68	# DW_AT_sibling
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_formal_parameter)
+	.long	.LASF0	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x19	# DW_AT_decl_line
+	.byte	0x10	# DW_AT_decl_column
+	.long	0x3b	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.byte	0	# end of children of DIE 0x42
+	.uleb128 0x6	# (DIE (0x68) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF6	# DW_AT_name: "ptwrite64"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0-.LFB0	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.uleb128 0x5	# (DIE (0x7a) DW_TAG_formal_parameter)
+	.long	.LASF0	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x10	# DW_AT_decl_column
+	.long	0x3b	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.byte	0	# end of children of DIE 0x68
+	.byte	0	# end of children of DIE 0xb
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x17	# (DW_FORM_sec_offset)
+	.byte	0
+	.byte	0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0	# DW_children_no
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2116	# (DW_AT_GNU_all_tail_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0
+	.byte	0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.byte	0
+	.byte	0
+	.uleb128 0x6	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF aranges version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0	# Size of Segment Descriptor
+	.value	0	# Pad to 8 byte boundary
+	.value	0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0
+	.long	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -m32 -mtune=generic -march=i686 -g"
+.LASF6:
+	.string	"ptwrite64"
+.LASF0:
+	.string	"value"
+.LASF3:
+	.string	"/root/gdb/gdb/testsuite/gdb.btrace"
+.LASF5:
+	.string	"ptwrite32"
+.LASF2:
+	.string	"ptwrite.c"
+.LASF4:
+	.string	"main"
+	.ident	"GCC: (GNU) 10.3.1 20210422 (Red Hat 10.3.1-1)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.btrace/ptwrite.c b/gdb/testsuite/gdb.btrace/ptwrite.c
new file mode 100644
index 00000000000..04c4f97c823
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/ptwrite.c
@@ -0,0 +1,37 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2021 Free Software Foundation, Inc.
+
+ 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/>.  */
+
+void
+ptwrite64 (int value)
+{
+  asm volatile ("PTWRITE %0;" : : "b" (value));
+}
+
+void
+ptwrite32 (int value)
+{
+  asm volatile ("PTWRITE %0;" : : "b" (value));
+}
+
+int
+main (void)
+{
+  ptwrite64 (0x42);
+  ptwrite32 (0x43);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/ptwrite.exp b/gdb/testsuite/gdb.btrace/ptwrite.exp
new file mode 100644
index 00000000000..4ef4af8bba8
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/ptwrite.exp
@@ -0,0 +1,219 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# 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 gdb-python.exp
+
+if { [skip_btrace_ptw_tests] } {
+    unsupported "No support for ptwrite decoding."
+    return -1
+}
+
+set opts {}
+
+if [info exists COMPILE] {
+    # make check RUNTESTFLAGS="gdb.btrace/ptwrite.exp COMPILE=1"
+    standard_testfile ptwrite.c
+    lappend opts debug
+} elseif {[istarget "i?86-*-*"] || [istarget "x86_64-*-*"]} {
+    if {[is_amd64_regs_target]} {
+	standard_testfile x86_64-ptwrite.S
+    } else {
+	standard_testfile i386-ptwrite.S
+    }
+} else {
+    unsupported "target architecture not supported"
+    return -1
+}
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
+    return -1
+}
+
+if { ![runto_main] } {
+    untested "failed to run to main"
+    return -1
+}
+
+# This needs to be after runto_main
+if { [skip_python_tests] } {
+    unsupported "Configuration doesn't support python scripting."
+    return -1
+}
+
+### 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"
+
+with_test_prefix "Default" {
+    # Test record instruction-history
+    gdb_test "record instruction-history 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[42\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[43\\\].*" \
+	]
+
+    gdb_test "record instruction-history /a 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+.*" \
+	]
+
+    # Test function call history
+    gdb_test "record function-call-history 1,4" [multi_line \
+	"1\tmain" \
+	"2\tptwrite64" \
+	"\t\t\\\[42\\\]" \
+	"3\tmain" \
+	"4\tptwrite32" \
+	"\t\t\\\[43\\\]" \
+	]
+
+    gdb_test "record function-call-history /a 1,4" [multi_line \
+	"1\tmain" \
+	"2\tptwrite64" \
+	"3\tmain" \
+	"4\tptwrite32" \
+	]
+}
+
+# Test payload printing during stepping
+with_test_prefix "Stepping" {
+
+    # 32-bit and 64-bit have slightly different number of instructions.
+    if {[is_amd64_regs_target]} {
+	set aux_line "10"
+	set ptwrite_line "9"
+    } else {
+	set aux_line "9"
+	set ptwrite_line "8"
+    }
+
+    gdb_test "record goto $aux_line" "No such instruction\."
+    gdb_test "record goto $ptwrite_line" ".*ptwrite64.* at .*ptwrite.c:21.*"
+    gdb_test "stepi" ".*\\\[42\\\].*"
+    gdb_test "reverse-stepi" ".*\\\[42\\\].*"
+    gdb_test "continue" [multi_line \
+	    ".*\\\[42\\\]" \
+	    "\\\[43\\\].*" \
+	    ]
+    gdb_test "reverse-continue" [multi_line \
+	    ".*\\\[43\\\]" \
+	    "\\\[42\\\].*" \
+	    ]
+}
+
+# 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" \
+	"42" \
+	"nop.*" \
+	"mov    %eax,%ebx" \
+	"ptwrite %ebx" \
+	"43" \
+	"nop.*"
+    ]
+
+
+### 2. Test filter registration
+### 2.1 Custom filter
+with_test_prefix "Custom" {
+    gdb_test_multiline "register filter in python" \
+	"python" "" \
+	"def my_filter(payload, ip):" "" \
+	"    if  payload == 66:" "" \
+	"        return \"payload: {0}, ip: {1:#x}\".format(payload, ip)" "" \
+	"    else:" "" \
+	"        return None" "" \
+	"import gdb.ptwrite" "" \
+	"gdb.ptwrite.register_filter(my_filter)" "" \
+	"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\]+>:.*" \
+	]
+}
+
+### 2.2 None as filter
+with_test_prefix "None" {
+    gdb_test_multiline "register filter in python" \
+	"python" "" \
+	"import gdb.ptwrite" "" \
+	"gdb.ptwrite.register_filter(None)" "" \
+	"end" ""
+
+    gdb_test "record instruction-history 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:.*" \
+	"\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:.*" \
+	]
+}
+
+### 2.3 Lambdas as filter
+with_test_prefix "Lambdas" {
+    gdb_test_multiline "register filter in python" \
+	"python" "" \
+	"import gdb.ptwrite" "" \
+	"gdb.ptwrite.register_filter(lambda payload, ip: \"{}\".format(payload + 2))" "" \
+	"end" ""
+
+    gdb_test "record instruction-history 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[68\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[69\\\].*" \
+	] "Lambdas: record instruction-history 1"
+}
+
+### 2.4 Functors as filter
+with_test_prefix "Functors" {
+    gdb_test_multiline "register filter in python" \
+	"python" "" \
+	"import gdb.ptwrite" "" \
+	"class foobar(object):" "" \
+	"    def __init__(self):" "" \
+	"        self.variable = 0" "" \
+	"    def __call__(self, payload, ip):" "" \
+	"        self.variable += 1" "" \
+	"        return \"{}, {}\".format(self.variable, payload)" "" \
+	"gdb.ptwrite.register_filter(foobar())" "" \
+	"end" ""
+
+    gdb_test "record instruction-history 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[1, 66\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[2, 67\\\].*" \
+	] "Functors: record instruction-history 1"
+}
diff --git a/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S b/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
new file mode 100644
index 00000000000..043c2fa2a9a
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
@@ -0,0 +1,374 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2022 Free Software Foundation, Inc.
+
+   Contributed by Intel Corp. <markus.t.metzger@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/>.
+
+
+   This file has been generated using gcc version 10.3.1 20210422
+   (Red Hat 10.3.1-1):
+   gcc -S -dA -g ptwrite.c -o x86_64-ptwrite.S.  */
+
+	.file	"ptwrite.c"
+	.text
+.Ltext0:
+	.globl	ptwrite64
+	.type	ptwrite64, @function
+ptwrite64:
+.LFB0:
+	.file 1 "ptwrite.c"
+	# ptwrite.c:20:1
+	.loc 1 20 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	pushq	%rbx
+	.cfi_offset 3, -24
+	movl	%edi, -12(%rbp)
+	# ptwrite.c:21:3
+	.loc 1 21 3
+	movl	-12(%rbp), %eax
+	movl	%eax, %ebx
+#APP
+# 21 "ptwrite.c" 1
+	PTWRITE %ebx;
+# 0 "" 2
+	# ptwrite.c:22:1
+	.loc 1 22 1
+#NO_APP
+	nop
+	movq	-8(%rbp), %rbx
+	leave
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	ptwrite64, .-ptwrite64
+	.globl	ptwrite32
+	.type	ptwrite32, @function
+ptwrite32:
+.LFB1:
+	# ptwrite.c:26:1
+	.loc 1 26 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	pushq	%rbx
+	.cfi_offset 3, -24
+	movl	%edi, -12(%rbp)
+	# ptwrite.c:27:3
+	.loc 1 27 3
+	movl	-12(%rbp), %eax
+	movl	%eax, %ebx
+#APP
+# 27 "ptwrite.c" 1
+	PTWRITE %ebx;
+# 0 "" 2
+	# ptwrite.c:28:1
+	.loc 1 28 1
+#NO_APP
+	nop
+	movq	-8(%rbp), %rbx
+	leave
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	ptwrite32, .-ptwrite32
+	.globl	main
+	.type	main, @function
+main:
+.LFB2:
+	# ptwrite.c:32:1
+	.loc 1 32 1
+	.cfi_startproc
+# BLOCK 2 seq:0
+# PRED: ENTRY (FALLTHRU)
+	pushq	%rbp
+	.cfi_def_cfa_offset 16
+	.cfi_offset 6, -16
+	movq	%rsp, %rbp
+	.cfi_def_cfa_register 6
+	# ptwrite.c:33:3
+	.loc 1 33 3
+	movl	$66, %edi
+	call	ptwrite64
+	# ptwrite.c:34:3
+	.loc 1 34 3
+	movl	$67, %edi
+	call	ptwrite32
+	# ptwrite.c:36:10
+	.loc 1 36 10
+	movl	$0, %eax
+	# ptwrite.c:37:1
+	.loc 1 37 1
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE2:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0xa7	# Length of Compilation Unit Info
+	.value	0x4	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x8	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF1	# DW_AT_producer: "GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -mtune=generic -march=x86-64 -g"
+	.byte	0xc	# DW_AT_language
+	.long	.LASF2	# DW_AT_name: "ptwrite.c"
+	.long	.LASF3	# DW_AT_comp_dir: "/root/gdb/gdb/testsuite/gdb.btrace"
+	.quad	.Ltext0	# DW_AT_low_pc
+	.quad	.Letext0-.Ltext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x2d) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF4	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	0x4b	# DW_AT_type
+	.quad	.LFB2	# DW_AT_low_pc
+	.quad	.LFE2-.LFB2	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_tail_call_sites
+	.uleb128 0x3	# (DIE (0x4b) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x4	# (DIE (0x52) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF5	# DW_AT_name: "ptwrite32"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x19	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB1	# DW_AT_low_pc
+	.quad	.LFE1-.LFB1	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0x80	# DW_AT_sibling
+	.uleb128 0x5	# (DIE (0x70) DW_TAG_formal_parameter)
+	.long	.LASF0	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x19	# DW_AT_decl_line
+	.byte	0x10	# DW_AT_decl_column
+	.long	0x4b	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -28
+	.byte	0	# end of children of DIE 0x52
+	.uleb128 0x6	# (DIE (0x80) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF6	# DW_AT_name: "ptwrite64"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB0	# DW_AT_low_pc
+	.quad	.LFE0-.LFB0	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.uleb128 0x5	# (DIE (0x9a) DW_TAG_formal_parameter)
+	.long	.LASF0	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x10	# DW_AT_decl_column
+	.long	0x4b	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -28
+	.byte	0	# end of children of DIE 0x80
+	.byte	0	# end of children of DIE 0xb
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x17	# (DW_FORM_sec_offset)
+	.byte	0
+	.byte	0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0	# DW_children_no
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2116	# (DW_AT_GNU_all_tail_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0
+	.byte	0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.byte	0
+	.byte	0
+	.uleb128 0x6	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x2c	# Length of Address Ranges Info
+	.value	0x2	# DWARF aranges version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x8	# Size of Address
+	.byte	0	# Size of Segment Descriptor
+	.value	0	# Pad to 16 byte boundary
+	.value	0
+	.quad	.Ltext0	# Address
+	.quad	.Letext0-.Ltext0	# Length
+	.quad	0
+	.quad	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF3:
+	.string	"/root/gdb/gdb/testsuite/gdb.btrace"
+.LASF6:
+	.string	"ptwrite64"
+.LASF0:
+	.string	"value"
+.LASF1:
+	.string	"GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -mtune=generic -march=x86-64 -g"
+.LASF5:
+	.string	"ptwrite32"
+.LASF2:
+	.string	"ptwrite.c"
+.LASF4:
+	.string	"main"
+	.ident	"GCC: (GNU) 10.3.1 20210422 (Red Hat 10.3.1-1)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 4bc5f4f144c..e9317467390 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3661,6 +3661,80 @@ gdb_caching_proc skip_btrace_pt_tests {
     return $skip_btrace_tests
 }
 
+# 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
+	return 1
+    }
+
+    set src {
+	int
+	main ()
+	{
+	  asm volatile ("PTWRITE %0;" : : "b"(0x42));
+	  return 0;
+	}
+    }
+
+    if {![gdb_simple_compile $me $src executable]} {
+        return 1
+    }
+
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load "$obj"
+    if ![runto_main] {
+        return 1
+    }
+
+    gdb_test_no_output "record btrace pt" "$me: record btrace pt"
+
+    set skip_ptw_tests 2
+    gdb_test_multiple "next" "$me: next" {
+        -re -wrap ".*Illegal instruction.*" {
+            verbose -log "$me:  ptwrite instruction support not detected."
+            set skip_ptw_tests 2
+        }
+        -re -wrap ".*$inferior_exited_re normally.*" {
+            verbose -log "$me:  ptwrite support not detected."
+            set skip_ptw_tests 2
+        }
+        -re -wrap "$decimal.*(at|in).*" {
+            set skip_ptw_tests 1
+        }
+    }
+
+    if { $skip_ptw_tests == 1 } {
+	# Show the func-call-history to get the packet trace.
+	gdb_test "record function-call-history" ".*"
+
+	gdb_test_multiple "maintenance btrace packet-history 0,1000" \
+	    "$me: check decoding support" {
+	    -re  "ptw" {
+		verbose -log "$me:  ptwrite decoding support detected."
+		set skip_ptw_tests 0
+	    }
+	    -re ".*${gdb_prompt} $" {
+		verbose -log "$me:  ptwrite decoding support not detected."
+	    }
+	}
+    }
+
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me:  returning $skip_ptw_tests" 2
+    return $skip_ptw_tests
+}
+
+
 # Run a test on the target to see if it supports Aarch64 SVE hardware.
 # Return 0 if so, 1 if it does not.  Note this causes a restart of GDB.
 
diff --git a/gdbsupport/common.m4 b/gdbsupport/common.m4
index d3e9ecbe823..aebf484b515 100644
--- a/gdbsupport/common.m4
+++ b/gdbsupport/common.m4
@@ -179,6 +179,8 @@ AC_DEFUN([GDB_AC_COMMON], [
       AC_CHECK_FUNCS(pt_insn_event)
       AC_CHECK_MEMBERS([struct pt_insn.enabled, struct pt_insn.resynced], [], [],
 		       [#include <intel-pt.h>])
+      AC_CHECK_MEMBERS([struct pt_event.variant.ptwrite], [], [],
+		       [#include <intel-pt.h>])
       LIBS=$save_LIBS
     fi
   fi
diff --git a/gdbsupport/config.in b/gdbsupport/config.in
index a7ae23b4984..3d5543cb7b5 100644
--- a/gdbsupport/config.in
+++ b/gdbsupport/config.in
@@ -232,6 +232,9 @@
 /* Define to 1 if you have the <string.h> header file. */
 #undef HAVE_STRING_H
 
+/* Define to 1 if `variant.ptwrite' is a member of `struct pt_event'. */
+#undef HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE
+
 /* Define to 1 if `enabled' is a member of `struct pt_insn'. */
 #undef HAVE_STRUCT_PT_INSN_ENABLED
 
diff --git a/gdbsupport/configure b/gdbsupport/configure
index 618f487749f..1f4f3ff02a5 100755
--- a/gdbsupport/configure
+++ b/gdbsupport/configure
@@ -9604,6 +9604,17 @@ cat >>confdefs.h <<_ACEOF
 _ACEOF
 
 
+fi
+
+      ac_fn_c_check_member "$LINENO" "struct pt_event" "variant.ptwrite" "ac_cv_member_struct_pt_event_variant_ptwrite" "#include <intel-pt.h>
+"
+if test "x$ac_cv_member_struct_pt_event_variant_ptwrite" = xyes; then :
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE 1
+_ACEOF
+
+
 fi
 
       LIBS=$save_LIBS
-- 
2.34.3

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 01/10] btrace: Introduce auxiliary instructions.
  2022-06-22 11:43 ` [PATCH v5 01/10] btrace: Introduce auxiliary instructions Felix Willgerodt
@ 2022-06-28  9:10   ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:10 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>Auxiliary instructions are pseudo instructions pointing to auxiliary data.
>This auxiliary data can be printed in all commands displaying (record
>function-call-history, record instruction-history) or stepping through
>(stepi etc.) the execution history, which will be introduced in the next
>commits.
>
>This patch is in preparation for the new ptwrite feature, which is based on
>auxiliary instructions.
>---
> gdb/btrace.c        |  2 ++
> gdb/btrace.h        | 24 +++++++++++++++++++++---
> gdb/doc/gdb.texinfo |  4 ++++
> 3 files changed, 27 insertions(+), 3 deletions(-)

LGTM.

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history.
  2022-06-22 11:43 ` [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history Felix Willgerodt
@ 2022-06-28  9:10   ` Metzger, Markus T
  2022-06-28 11:28     ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:10 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
>is encountered in the instruction-history.  Printing is active by default,
>it can be silenced with the /a modifier.

I find it a bit strange that /a disables some output instead of enabling it.
Should we change the default?  I'd probably always use it, but then, I
also always supply /cli to 'record function-call-history'.


>This patch is in preparation for the new ptwrite feature, which is based on
>auxiliary instructions.
>---
> gdb/disasm-flags.h  |  1 +
> gdb/doc/gdb.texinfo |  3 +++
> gdb/record-btrace.c | 14 ++++++++++++++
> gdb/record.c        |  5 +++++
> 4 files changed, 23 insertions(+)

>+      else if (insn->iclass == BTRACE_INSN_AUX)
>+	{
>+	  if ((flags & DISASSEMBLY_OMIT_AUX_INSN) != 0)
>+	    continue;
>+
>+	  uiout->field_fmt ("insn-number", "%u", btrace_insn_number (&it));
>+	  uiout->text ("\t");
>+	  uiout->spaces (3);
>+	  uiout->text ("[");
>+	  uiout->field_fmt (
>+	      "aux-data", "%s",
>+	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());

The formatting is a bit unusual.

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history.
  2022-06-22 11:43 ` [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history Felix Willgerodt
@ 2022-06-28  9:10   ` Metzger, Markus T
  2022-09-19  8:59     ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:10 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
>is encountered in the function-call-history.  Printing is
>active by default, it can be silenced with the /a modifier.

I find it a bit strange that /a disables some output instead of enabling it.
Should we change the default?  I'd probably always use it, but then, I
also always supply /cli.


>This patch is in preparation for the new ptwrite feature, which is based on
>auxiliary instructions.
>---
> gdb/btrace.h        |  6 +++++-
> gdb/doc/gdb.texinfo |  5 +++--
> gdb/record-btrace.c | 21 +++++++++++++++++++++
> gdb/record.c        |  5 +++++
> gdb/record.h        |  5 ++++-
> 5 files changed, 38 insertions(+), 4 deletions(-)


>   /* The 'up' link points to a tail call.  This obviously only makes sense
>      if bfun_up_links_to_ret is clear.  */
>-  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1)
>+  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1),
>+
>+  /* Indicates that at least one auxiliary instruction is in the current
>+     function segment.  */
>+  BFUN_AUX_DECODED = (1 << 2)

Should this maybe be called BFUN_CONTAINS_AUX?


>+static void
>+btrace_print_aux_insn (struct ui_out *uiout,
>+		       const struct btrace_function *bfun,
>+		       const struct btrace_thread_info *btinfo)
>+{
>+  for (const btrace_insn &insn : bfun->insn)
>+    {
>+      if (insn.iclass == BTRACE_INSN_AUX)
>+	{
>+	  uiout->text ("\t\t[");

This should probably do

    uiout->field_skip ("index");
    uiout->text ("\t[");


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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions.
  2022-06-22 11:43 ` [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions Felix Willgerodt
@ 2022-06-28  9:11   ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:11 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>Print the auxiliary data when stepping. Don't allow to goto an auxiliary
>instruction.
>
>This patch is in preparation for the new ptwrite feature, which is based on
>auxiliary instructions.
>---
> gdb/record-btrace.c | 68 ++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 55 insertions(+), 13 deletions(-)

LGTM.


>+  const struct btrace_insn *insn = btrace_insn_get (&it);
>+
>+  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
>     error (_("No such instruction."));

Let's remove the empty line between those two statements to group them.

thanks,
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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class.
  2022-06-22 11:43 ` [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class Felix Willgerodt
@ 2022-06-28  9:11   ` Metzger, Markus T
  2022-07-11 12:48     ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:11 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>+  if (insn->iclass == BTRACE_INSN_AUX)
>+    return recpy_aux_new (
>+	iter.btinfo->aux_data.at (insn->aux_data_index).c_str (), number);

The formatting looks off.  It should be broken at the 2nd argument.


>@@ -64,6 +70,18 @@ struct recpy_gap_object
>   Py_ssize_t number;
> };
>
>+/* Python RecordAuxiliary object.  */
>+typedef struct
>+{
>+  PyObject_HEAD
>+
>+  /* Auxiliary data.  */
>+  const char *data;

Should this be a std::string instead of referencing the C string in the AUX_DATA?

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 06/10] python: Add clear() to gdb.Record.
  2022-06-22 11:43 ` [PATCH v5 06/10] python: Add clear() to gdb.Record Felix Willgerodt
@ 2022-06-28  9:11   ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:11 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>This function allows to clear the trace data from python, forcing to
>re-decode the trace for successive commands.
>---
> gdb/doc/python.texi                           |  5 +++++
> gdb/python/py-record-btrace.c                 | 13 +++++++++++++
> gdb/python/py-record-btrace.h                 |  3 +++
> gdb/python/py-record.c                        | 16 ++++++++++++++++
> gdb/testsuite/gdb.python/py-record-btrace.exp |  6 +++++-
> 5 files changed, 42 insertions(+), 1 deletion(-)

LGTM.

It would be good if someone with better python knowledge
reviewed this patch, too.

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt.
  2022-06-22 11:43 ` [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt Felix Willgerodt
@ 2022-06-28  9:11   ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:11 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>   conf = (struct btrace_config *) user_data;
>   conf->format = BTRACE_FORMAT_PT;
>@@ -2282,10 +2282,15 @@ parse_xml_btrace_conf_pt (struct gdb_xml_parser
>*parser,
>   size = xml_find_attribute (attributes, "size");
>   if (size != NULL)
>     conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
>+
>+  ptwrite = xml_find_attribute (attributes, "ptwrite");
>+  if (ptwrite != nullptr)
>+    conf->pt.ptwrite = (bool) *(ULONGEST *) ptwrite->value.get ();
> }
>
> static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
>   { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
>+  { "ptwrite", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, nullptr },

Should this be a boolean enum rather than a ULONGEST?


>+@item Qbtrace-conf:pt:ptwrite=@var{value}
>+Control recording of ptwrite packets.  This allows for backwards-
>+compatibility.
>+
>+Reply:
>+@table @samp
>+@item OK
>+The ptwrite config parameter has been set.
>+@item E.errtext
>+A badly formed request or an error was encountered.
>+@end table

We need to describe the allowed values and their meaning.  It might be
easier if this were a boolean yes/no or on/off.

See the save-restore attribute in gdb/features/gdb-target.dtd, for example.

We use this to indicate ptwrite decode support in GDB to gdbserver; we do not
really use this to configure ptwrite as we don't know, yet, whether the target
supports it.  This becomes clear in patch 8.

That sounds OK to me as I don't really see why one wouldn't want to enable
ptwrite, but it should be reflected in the RSP documentation.

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 08/10] btrace, linux: Enable ptwrite packets.
  2022-06-22 11:43 ` [PATCH v5 08/10] btrace, linux: Enable ptwrite packets Felix Willgerodt
@ 2022-06-28  9:12   ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28  9:12 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>Enable ptwrite in the PT config, if it is supported by the kernel.
>---
> gdb/nat/linux-btrace.c | 29 +++++++++++++++++++++++++++++
> gdb/record-btrace.c    |  5 +++++
> 2 files changed, 34 insertions(+)

LGTM.

>+  int status, found = fscanf (file.get (), "%d", &status);
>+
>+  if (found != 1)

Let's remove the empty line to group the statements.

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history.
  2022-06-28  9:10   ` Metzger, Markus T
@ 2022-06-28 11:28     ` Willgerodt, Felix
  2022-06-29 10:43       ` Metzger, Markus T
  0 siblings, 1 reply; 32+ messages in thread
From: Willgerodt, Felix @ 2022-06-28 11:28 UTC (permalink / raw)
  To: Metzger, Markus T, gdb-patches

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 11:10
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 02/10] btrace: Enable auxiliary instructions in record
> instruction-history.
> 
> Hello Felix,
> 
> >Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
> >is encountered in the instruction-history.  Printing is active by default,
> >it can be silenced with the /a modifier.
> 
> I find it a bit strange that /a disables some output instead of enabling it.
> Should we change the default?  I'd probably always use it, but then, I
> also always supply /cli to 'record function-call-history'.

I don't find it strange. /p and /f already disables some output:

(gdb) help record instruction-history 
Print disassembled instructions stored in the execution log.
With a /m or /s modifier, source lines are included (if available).
With a /r modifier, raw instructions in hex are included.
With a /f modifier, function names are omitted.
With a /p modifier, current position markers are omitted.

I think we should make the default the thing GDB users "would want".

> 
> >This patch is in preparation for the new ptwrite feature, which is based on
> >auxiliary instructions.
> >---
> > gdb/disasm-flags.h  |  1 +
> > gdb/doc/gdb.texinfo |  3 +++
> > gdb/record-btrace.c | 14 ++++++++++++++
> > gdb/record.c        |  5 +++++
> > 4 files changed, 23 insertions(+)
> 
> >+      else if (insn->iclass == BTRACE_INSN_AUX)
> >+	{
> >+	  if ((flags & DISASSEMBLY_OMIT_AUX_INSN) != 0)
> >+	    continue;
> >+
> >+	  uiout->field_fmt ("insn-number", "%u", btrace_insn_number (&it));
> >+	  uiout->text ("\t");
> >+	  uiout->spaces (3);
> >+	  uiout->text ("[");
> >+	  uiout->field_fmt (
> >+	      "aux-data", "%s",
> >+	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
> 
> The formatting is a bit unusual.

Agreed. How should I format it instead? The last line is the problem.

Should it be this:

	  uiout->field_fmt
	     ("aux-data", "%s",
	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());

Or should it be something like this:

	  uiout->field_fmt ("aux-data", "%s",
			    it.btinfo->aux_data.at
				(insn->aux_data_index).c_str ());

Or I can also add a temporary std:string:

	  std::string data
	    = it.btinfo->aux_data.at (insn->aux_data_index).c_str ();
	  uiout->field_fmt ("aux-data", "%s", data);

I can't really find a good way.

Thanks,
Felix
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-06-22 11:43 ` [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration Felix Willgerodt
@ 2022-06-28 13:59   ` Metzger, Markus T
  2022-07-11 12:48     ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-28 13:59 UTC (permalink / raw)
  To: Willgerodt, Felix, gdb-patches

Hello Felix,

>With this patch a default ptwrite filter is registered upon start of GDB.
>It prints the plain ptwrite payload as hex.  The default filter can be
>overwritten by registering a custom filter in python or by registering
>"None", for no output at all.  Registering a filter function creates per
>thread copies to allow unique internal states per thread.
>---
> gdb/btrace.c                   |   3 +
> gdb/btrace.h                   |   9 +++
> gdb/data-directory/Makefile.in |   1 +
> gdb/extension-priv.h           |   5 ++
> gdb/extension.c                |  13 ++++
> gdb/extension.h                |   3 +
> gdb/guile/guile.c              |   1 +
> gdb/python/lib/gdb/ptwrite.py  |  86 +++++++++++++++++++++++++
> gdb/python/py-record-btrace.c  | 111 +++++++++++++++++++++++++++++++++
> gdb/python/py-record-btrace.h  |   8 +++
> gdb/python/python-internal.h   |   3 +
> gdb/python/python.c            |   2 +
> 12 files changed, 245 insertions(+)
> create mode 100644 gdb/python/lib/gdb/ptwrite.py


> /* For maintenance commands.  */
>@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
>   uint64_t offset;
>   int status;
>
>+  apply_ext_lang_ptwrite_filter (btinfo);

A comment would be nice, here, to explain what this does.


>+  /* Function pointer to the ptwrite callback.  Returns the string returned
>+     by the ptwrite filter function or nullptr if no string is supposed to
>+     be printed.  */
>+  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
>+				   const void *ptw_filter);

The comment doesn't match the code.  It cannot return nullptr.

Why to we call the void * parameter ptw_filter instead of the usual context?
We probably want to call the callback itself ptw_filter and the void * argument
context.

We also seem to mix the terms ptwrite callback and ptwrite filter.

>+
>+  /* Function pointer to the ptwrite filter function.  */
>+  void *ptw_filter = nullptr;

Is this the function or some context for the function?


>diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
>index 14b191ded62..86f92a476af 100644
>--- a/gdb/guile/guile.c
>+++ b/gdb/guile/guile.c
>@@ -124,6 +124,7 @@ static const struct extension_language_ops
>guile_extension_ops =
>   gdbscm_apply_val_pretty_printer,
>
>   NULL, /* gdbscm_apply_frame_filter, */
>+  NULL, /* gdbscm_load_ptwrite_listener, */

This should probably be called ptwrite_filter.


>+def default_filter(payload, ip):
>+    """Default filter that is active upon starting GDB."""
>+    return "{:x}".format(payload)
>+
>+# This dict contains the per thread copies of the filter function and the
>+# global template filter, from which the copies are created.
>+_ptwrite_filter = {"global" : default_filter}

Why those leading underscores?


>+def _update_filter_dict(thread_list):
>+    """Helper function to update the filter dict.
>+
>+    Discards filter copies of threads that already exited and registers
>+    copies of the filter for new threads."""
>+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
>+    lwp_list = [i.ptid[1] for i in thread_list]
>+
>+    # clean-up old filters
>+    for key in _ptwrite_filter.keys():
>+      if key not in lwp_list and key != "global":
>+        _ptwrite_filter.pop(key)
>+
>+    # Register filter for new threads
>+    for key in lwp_list:
>+        if key not in _ptwrite_filter.keys():
>+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])

This function is called two times: once after we cleared all filters, and
once when looking up the filter for a given thread.  The first time, we
know that there are no existing filters; the second time, we are really
only interested in a single filter.

Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
exist, create a new one?

That leaves removing obsolete filters.  Could this be done with some
thread notification?


>+def _clear_traces(thread_list):
>+    """Helper function to clear the trace of all threads in THREAD_LIST."""
>+    current_thread = gdb.selected_thread()
>+
>+    recording = gdb.current_recording()
>+
>+    if (recording is not None):

Let's remove the empty line to group the statements.


>+/* Helper function that calls the ptwrite filter PTW_FILTER with
>+   PAYLOAD and IP as arguments.  Returns a pointer to the string that will
>+   be printed or nullptr if nothing should be printed.  IP can be nullptr,
>+   PAYLOAD must point to a valid integer.  */
>+std::string
>+recpy_call_filter (const uint64_t payload, const uint64_t ip,
>+		   const void *ptw_filter)

The comment doesn't match the code.  It cannot return nullptr.  Also,
IP and PAYLOAD are integers, not pointers.

I think we can shorten "calls the ptwrite filter PTW_FILTER" to just
"calls PTW_FILTER".


>+{
>+  std::string result;
>+
>+  if ((PyObject *) ptw_filter == Py_None)
>+    return result;
>+  else if ((PyObject *) ptw_filter == nullptr)
>+    error (_("No valid ptwrite filter."));

No need for else.  Should we check nullptr first?


>+/* Helper function returning the current ptwrite filter.  Returns nullptr
>+   in case of errors.  */
>+
>+PyObject *
>+get_ptwrite_filter ()
>+{
>+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
>+
>+  if (PyErr_Occurred ())
>+  {
>+    gdbpy_print_stack ();
>+    return nullptr;
>+  }

Do we want to print the stack without throwing an error?


>+/* Used for registering the default ptwrite filter to the current thread.  A

What does 'default' mean, here?  We're registering the callback that btrace
calls on PTW.  From btrace's perspective, this is the ptwrite filter.  From
python's perspective, this is maybe a proxy for the python filter.

>+   pointer to this function is stored in the python extension interface.  */
>+
>+void
>+gdbpy_load_ptwrite_filter (const struct extension_language_defn *extlang,
>+			   struct btrace_thread_info *btinfo)
>+{
>+  if (!gdb_python_initialized || btinfo == nullptr)
>+    return;

Isn't this an error or even an internal error case?


>+/* Callback function for the ptwrite filter.  */
>+extern std::string recpy_call_filter (const uint64_t payload,
>+				      const uint64_t ip,
>+				      const void *ptw_filter);

Should the comment say something like 'proxy for the python
ptwrite filter'?

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history.
  2022-06-28 11:28     ` Willgerodt, Felix
@ 2022-06-29 10:43       ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-29 10:43 UTC (permalink / raw)
  To: Willgerodt, Felix, Eli Zaretskii (eliz@gnu.org); +Cc: gdb-patches

Hello Felix,

>> >Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
>> >is encountered in the instruction-history.  Printing is active by default,
>> >it can be silenced with the /a modifier.
>>
>> I find it a bit strange that /a disables some output instead of enabling it.
>> Should we change the default?  I'd probably always use it, but then, I
>> also always supply /cli to 'record function-call-history'.
>
>I don't find it strange. /p and /f already disables some output:
>
>(gdb) help record instruction-history
>Print disassembled instructions stored in the execution log.
>With a /m or /s modifier, source lines are included (if available).
>With a /r modifier, raw instructions in hex are included.
>With a /f modifier, function names are omitted.
>With a /p modifier, current position markers are omitted.
>
>I think we should make the default the thing GDB users "would want".

Hmmm, OK, it does fit.  /p was added later so I had to make it omit things
to preserve backwards compatibility.  But /f was there from the beginning.

I'm OK either way; let's hear what Eli thinks.


>> >+      else if (insn->iclass == BTRACE_INSN_AUX)
>> >+	{
>> >+	  if ((flags & DISASSEMBLY_OMIT_AUX_INSN) != 0)
>> >+	    continue;
>> >+
>> >+	  uiout->field_fmt ("insn-number", "%u", btrace_insn_number (&it));
>> >+	  uiout->text ("\t");
>> >+	  uiout->spaces (3);
>> >+	  uiout->text ("[");
>> >+	  uiout->field_fmt (
>> >+	      "aux-data", "%s",
>> >+	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
>>
>> The formatting is a bit unusual.
>
>Agreed. How should I format it instead? The last line is the problem.
>
>Should it be this:
>
>	  uiout->field_fmt
>	     ("aux-data", "%s",
>	      it.btinfo->aux_data.at (insn->aux_data_index).c_str ());
>
>Or should it be something like this:
>
>	  uiout->field_fmt ("aux-data", "%s",
>			    it.btinfo->aux_data.at
>				(insn->aux_data_index).c_str ());
>
>Or I can also add a temporary std:string:
>
>	  std::string data
>	    = it.btinfo->aux_data.at (insn->aux_data_index).c_str ();
>	  uiout->field_fmt ("aux-data", "%s", data);

No need to construct a std::string; you may just use a const char * or
a reference to the std::string that at () returns.

All of the above are acceptable, I think.

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 10/10] btrace: Extend ptwrite event decoding.
  2022-06-22 11:43 ` [PATCH v5 10/10] btrace: Extend ptwrite event decoding Felix Willgerodt
@ 2022-06-29 13:35   ` Metzger, Markus T
  2022-09-19  8:59     ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-06-29 13:35 UTC (permalink / raw)
  To: Willgerodt, Felix; +Cc: gdb-patches

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


^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-06-28 13:59   ` Metzger, Markus T
@ 2022-07-11 12:48     ` Willgerodt, Felix
  2022-07-12 12:23       ` Metzger, Markus T
  0 siblings, 1 reply; 32+ messages in thread
From: Willgerodt, Felix @ 2022-07-11 12:48 UTC (permalink / raw)
  To: Metzger, Markus T, gdb-patches

Hi Markus,

Thanks for your feedback. I added some responses below,
that we probably should agree on before the next revision.

Thanks,
Felix

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 15:59
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> Hello Felix,
> 
> >With this patch a default ptwrite filter is registered upon start of GDB.
> >It prints the plain ptwrite payload as hex.  The default filter can be
> >overwritten by registering a custom filter in python or by registering
> >"None", for no output at all.  Registering a filter function creates per
> >thread copies to allow unique internal states per thread.
> >---
> > gdb/btrace.c                   |   3 +
> > gdb/btrace.h                   |   9 +++
> > gdb/data-directory/Makefile.in |   1 +
> > gdb/extension-priv.h           |   5 ++
> > gdb/extension.c                |  13 ++++
> > gdb/extension.h                |   3 +
> > gdb/guile/guile.c              |   1 +
> > gdb/python/lib/gdb/ptwrite.py  |  86 +++++++++++++++++++++++++
> > gdb/python/py-record-btrace.c  | 111
> +++++++++++++++++++++++++++++++++
> > gdb/python/py-record-btrace.h  |   8 +++
> > gdb/python/python-internal.h   |   3 +
> > gdb/python/python.c            |   2 +
> > 12 files changed, 245 insertions(+)
> > create mode 100644 gdb/python/lib/gdb/ptwrite.py
> 
> 
> > /* For maintenance commands.  */
> >@@ -1317,6 +1318,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
> >   uint64_t offset;
> >   int status;
> >
> >+  apply_ext_lang_ptwrite_filter (btinfo);
> 
> A comment would be nice, here, to explain what this does.

I added one in the next version.

> 
> >+  /* Function pointer to the ptwrite callback.  Returns the string returned
> >+     by the ptwrite filter function or nullptr if no string is supposed to
> >+     be printed.  */
> >+  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
> >+				   const void *ptw_filter);
> 
> The comment doesn't match the code.  It cannot return nullptr.

Will be fixed in the next revision.

> Why to we call the void * parameter ptw_filter instead of the usual context?
> We probably want to call the callback itself ptw_filter and the void *
> argument
> context.
> 
> We also seem to mix the terms ptwrite callback and ptwrite filter.

I think the problem here is that both are callbacks. The first one (ptw_callback_fun)
is used in btrace.c to call python/py-record-btrace.c:recpy_call_filter, or another
extension language that would provide this functionality,
see extension.c:apply_ext_lang_ptwrite_filter.
The second one (ptw_filter) is what recpy_call_filter will use to do the python call via
PyObject_CallFunctionObjArgs().

So we call a callback with another callback as the argument (which is the actual ptw_filter).
Therefore the current naming seems correct to me. As the ptw_filter to me clearly is
the void *. I am open to suggestions, but calling the actual filter function context and the
gdb internal callback the filter seems wrong to me.

I could add to the comment, that the callback_fun can be provided by any extension
language. Or rename it somehow.

> >+
> >+  /* Function pointer to the ptwrite filter function.  */
> >+  void *ptw_filter = nullptr;
> 
> Is this the function or some context for the function?
> 

See above. This is the actual python filter object GDB calls in the end.

> 
> >diff --git a/gdb/guile/guile.c b/gdb/guile/guile.c
> >index 14b191ded62..86f92a476af 100644
> >--- a/gdb/guile/guile.c
> >+++ b/gdb/guile/guile.c
> >@@ -124,6 +124,7 @@ static const struct extension_language_ops
> >guile_extension_ops =
> >   gdbscm_apply_val_pretty_printer,
> >
> >   NULL, /* gdbscm_apply_frame_filter, */
> >+  NULL, /* gdbscm_load_ptwrite_listener, */
> 
> This should probably be called ptwrite_filter.

Fixed locally.

> 
> >+def default_filter(payload, ip):
> >+    """Default filter that is active upon starting GDB."""
> >+    return "{:x}".format(payload)
> >+
> >+# This dict contains the per thread copies of the filter function and the
> >+# global template filter, from which the copies are created.
> >+_ptwrite_filter = {"global" : default_filter}
> 
> Why those leading underscores?

GDB follows the PEP8 coding standards:
https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards
These say that the leading underscore is a "weak internal user indicator":
https://pep8.org/#descriptive-naming-styles
See also:
https://docs.python.org/3/tutorial/classes.html#private-variables

Other files in GDB use the style as well. It is supposed to mean that
the variable shouldn't be used directly.

> 
> >+def _update_filter_dict(thread_list):
> >+    """Helper function to update the filter dict.
> >+
> >+    Discards filter copies of threads that already exited and registers
> >+    copies of the filter for new threads."""
> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
> >+    lwp_list = [i.ptid[1] for i in thread_list]
> >+
> >+    # clean-up old filters
> >+    for key in _ptwrite_filter.keys():
> >+      if key not in lwp_list and key != "global":
> >+        _ptwrite_filter.pop(key)
> >+
> >+    # Register filter for new threads
> >+    for key in lwp_list:
> >+        if key not in _ptwrite_filter.keys():
> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> 
> This function is called two times: once after we cleared all filters, and
> once when looking up the filter for a given thread.  The first time, we
> know that there are no existing filters; the second time, we are really
> only interested in a single filter.
>
> Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
> exist, create a new one?

Yes, we could get rid of the call to _update_filter_dict() in register_filter().
The main reason I added it was to clean the obsolete filters whenever
possible. I don't see a clear performance advantage if we would remove
the call (without having a thread exit notification).
We need to clean up the same amount of filters at some point.

> That leaves removing obsolete filters.  Could this be done with some
> thread notification?

IIRC, you suggested this previously. I replied that there is no python API
that I am aware of that can do this. The python events API doesn't expose
thread exited events.

The notification approach would have the disadvantage that GDB might
make a lot of calls to python to clean up single filters. Of course, the
current approach has the disadvantage that it manually keeps the thread
list and filter dict in sync.

> 
> >+def _clear_traces(thread_list):
> >+    """Helper function to clear the trace of all threads in THREAD_LIST."""
> >+    current_thread = gdb.selected_thread()
> >+
> >+    recording = gdb.current_recording()
> >+
> >+    if (recording is not None):
> 
> Let's remove the empty line to group the statements.

Will be done.

> 
> >+/* Helper function that calls the ptwrite filter PTW_FILTER with
> >+   PAYLOAD and IP as arguments.  Returns a pointer to the string that will
> >+   be printed or nullptr if nothing should be printed.  IP can be nullptr,
> >+   PAYLOAD must point to a valid integer.  */
> >+std::string
> >+recpy_call_filter (const uint64_t payload, const uint64_t ip,
> >+		   const void *ptw_filter)
> 
> The comment doesn't match the code.  It cannot return nullptr.  Also,
> IP and PAYLOAD are integers, not pointers.
> 
> I think we can shorten "calls the ptwrite filter PTW_FILTER" to just
> "calls PTW_FILTER".

Right, I missed that. Will be fixed, thanks.

> 
> >+{
> >+  std::string result;
> >+
> >+  if ((PyObject *) ptw_filter == Py_None)
> >+    return result;
> >+  else if ((PyObject *) ptw_filter == nullptr)
> >+    error (_("No valid ptwrite filter."));
> 
> No need for else.  Should we check nullptr first?

Will be fixed.

> 
> >+/* Helper function returning the current ptwrite filter.  Returns nullptr
> >+   in case of errors.  */
> >+
> >+PyObject *
> >+get_ptwrite_filter ()
> >+{
> >+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
> >+
> >+  if (PyErr_Occurred ())
> >+  {
> >+    gdbpy_print_stack ();
> >+    return nullptr;
> >+  }
> 
> Do we want to print the stack without throwing an error?
> 

I added an error (and unified the errors of the two functions a bit).

> >+/* Used for registering the default ptwrite filter to the current thread.  A
> 
> What does 'default' mean, here?  We're registering the callback that btrace
> calls on PTW.  From btrace's perspective, this is the ptwrite filter.  From
> python's perspective, this is maybe a proxy for the python filter.
> 
> >+   pointer to this function is stored in the python extension interface.  */
> >+
> >+void
> >+gdbpy_load_ptwrite_filter (const struct extension_language_defn
> *extlang,
> >+			   struct btrace_thread_info *btinfo)
> >+{
> >+  if (!gdb_python_initialized || btinfo == nullptr)
> >+    return;
> 
> Isn't this an error or even an internal error case?

gdb_python_initialized is actually checked by gdbpy_enter, so I removed it.
I added an assert for btinfo.

> 
> >+/* Callback function for the ptwrite filter.  */
> >+extern std::string recpy_call_filter (const uint64_t payload,
> >+				      const uint64_t ip,
> >+				      const void *ptw_filter);
> 
> Should the comment say something like 'proxy for the python
> ptwrite filter'?

See discussion above. In the extension language interface in GDB this is
a callback. How about:

-/* Callback function for the ptwrite filter.  */
+/* Helper function to call the ptwrite filter.  */

That makes it more in line with other python/*.h files.
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class.
  2022-06-28  9:11   ` Metzger, Markus T
@ 2022-07-11 12:48     ` Willgerodt, Felix
  0 siblings, 0 replies; 32+ messages in thread
From: Willgerodt, Felix @ 2022-07-11 12:48 UTC (permalink / raw)
  To: Metzger, Markus T, gdb-patches

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 11:11
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> Hello Felix,
> 
> >+  if (insn->iclass == BTRACE_INSN_AUX)
> >+    return recpy_aux_new (
> >+	iter.btinfo->aux_data.at (insn->aux_data_index).c_str (), number);
> 
> The formatting looks off.  It should be broken at the 2nd argument.
> 

It can't be broken there as it would be 83 characters long. I moved the "("
to the line below.

> >@@ -64,6 +70,18 @@ struct recpy_gap_object
> >   Py_ssize_t number;
> > };
> >
> >+/* Python RecordAuxiliary object.  */
> >+typedef struct
> >+{
> >+  PyObject_HEAD
> >+
> >+  /* Auxiliary data.  */
> >+  const char *data;
> 
> Should this be a std::string instead of referencing the C string in the
> AUX_DATA?

I don't think this can be a std::string. recpy_aux_object needs to be
passed to the python C-API to create a new PyObject.
I tried it and GDB segfaults when trying to assign anything to data.

Regards,
Felix
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-07-11 12:48     ` Willgerodt, Felix
@ 2022-07-12 12:23       ` Metzger, Markus T
  2022-07-13  8:49         ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-07-12 12:23 UTC (permalink / raw)
  To: Willgerodt, Felix; +Cc: gdb-patches

Hello Felix,

>> Why to we call the void * parameter ptw_filter instead of the usual context?
>> We probably want to call the callback itself ptw_filter and the void *
>> argument
>> context.
>>
>> We also seem to mix the terms ptwrite callback and ptwrite filter.
>
>I think the problem here is that both are callbacks. The first one
>(ptw_callback_fun)
>is used in btrace.c to call python/py-record-btrace.c:recpy_call_filter, or another
>extension language that would provide this functionality,
>see extension.c:apply_ext_lang_ptwrite_filter.
>The second one (ptw_filter) is what recpy_call_filter will use to do the python call
>via
>PyObject_CallFunctionObjArgs().
>
>So we call a callback with another callback as the argument (which is the actual
>ptw_filter).
>Therefore the current naming seems correct to me. As the ptw_filter to me
>clearly is
>the void *. 

That's from python's perspective.  The fact that we pass the python object that
implements the python filter to the btrace callback is a detail of the python
implementation, though.

From btrace's perspective, this is the ptwrite callback/filter and the void * is
whatever context that callback needs passed as argument.  I'm fine to call the
function ptw_callback but the context shouldn't be called ptwrite_filter.

In proper C++, we'd probably have a single member for a callable object that
may store its context inside.  Would this even work for python?


>I am open to suggestions, but calling the actual filter function context
>and the
>gdb internal callback the filter seems wrong to me.

Not from btrace's perspective and the code is in btrace.  Python just uses
it in a particular way.


>> >+def default_filter(payload, ip):
>> >+    """Default filter that is active upon starting GDB."""
>> >+    return "{:x}".format(payload)
>> >+
>> >+# This dict contains the per thread copies of the filter function and the
>> >+# global template filter, from which the copies are created.
>> >+_ptwrite_filter = {"global" : default_filter}
>>
>> Why those leading underscores?
>
>GDB follows the PEP8 coding standards:
>https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards
>These say that the leading underscore is a "weak internal user indicator":
>https://pep8.org/#descriptive-naming-styles
>See also:
>https://docs.python.org/3/tutorial/classes.html#private-variables
>
>Other files in GDB use the style as well. It is supposed to mean that
>the variable shouldn't be used directly.

OK.  Thanks.


>> >+def _update_filter_dict(thread_list):
>> >+    """Helper function to update the filter dict.
>> >+
>> >+    Discards filter copies of threads that already exited and registers
>> >+    copies of the filter for new threads."""
>> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
>> >+    lwp_list = [i.ptid[1] for i in thread_list]
>> >+
>> >+    # clean-up old filters
>> >+    for key in _ptwrite_filter.keys():
>> >+      if key not in lwp_list and key != "global":
>> >+        _ptwrite_filter.pop(key)
>> >+
>> >+    # Register filter for new threads
>> >+    for key in lwp_list:
>> >+        if key not in _ptwrite_filter.keys():
>> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>>
>> This function is called two times: once after we cleared all filters, and
>> once when looking up the filter for a given thread.  The first time, we
>> know that there are no existing filters; the second time, we are really
>> only interested in a single filter.
>>
>> Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
>> exist, create a new one?
>
>Yes, we could get rid of the call to _update_filter_dict() in register_filter().
>The main reason I added it was to clean the obsolete filters whenever
>possible. I don't see a clear performance advantage if we would remove
>the call (without having a thread exit notification).
>We need to clean up the same amount of filters at some point.
>
>> That leaves removing obsolete filters.  Could this be done with some
>> thread notification?
>
>IIRC, you suggested this previously. I replied that there is no python API
>that I am aware of that can do this. The python events API doesn't expose
>thread exited events.

I keep stumbling over this.

When looking up a filter, we are clearly only interested in one thread.
Just looking up that one and creating it when it is missing seems a lot
more straight forward.

Lacking a thread exit notification, we could still add a _prune_filters
function that we call every now and then that just removes filters for
exited threads.

Does that sound reasonable?  We'd need to find good places to call
it from.

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-07-12 12:23       ` Metzger, Markus T
@ 2022-07-13  8:49         ` Willgerodt, Felix
  2022-07-13 15:20           ` Metzger, Markus T
  0 siblings, 1 reply; 32+ messages in thread
From: Willgerodt, Felix @ 2022-07-13  8:49 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 12. Juli 2022 14:24
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> Hello Felix,
> 
> >> Why to we call the void * parameter ptw_filter instead of the usual
> context?
> >> We probably want to call the callback itself ptw_filter and the void *
> >> argument
> >> context.
> >>
> >> We also seem to mix the terms ptwrite callback and ptwrite filter.
> >
> >I think the problem here is that both are callbacks. The first one
> >(ptw_callback_fun)
> >is used in btrace.c to call python/py-record-btrace.c:recpy_call_filter, or
> another
> >extension language that would provide this functionality,
> >see extension.c:apply_ext_lang_ptwrite_filter.
> >The second one (ptw_filter) is what recpy_call_filter will use to do the
> python call
> >via
> >PyObject_CallFunctionObjArgs().
> >
> >So we call a callback with another callback as the argument (which is the
> actual
> >ptw_filter).
> >Therefore the current naming seems correct to me. As the ptw_filter to me
> >clearly is
> >the void *.
> 
> That's from python's perspective.  The fact that we pass the python object
> that
> implements the python filter to the btrace callback is a detail of the python
> implementation, though.
> 
> From btrace's perspective, this is the ptwrite callback/filter and the void * is
> whatever context that callback needs passed as argument.  I'm fine to call
> the
> function ptw_callback but the context shouldn't be called ptwrite_filter.
> 
> In proper C++, we'd probably have a single member for a callable object that
> may store its context inside.  Would this even work for python?
>
> >I am open to suggestions, but calling the actual filter function context
> >and the
> >gdb internal callback the filter seems wrong to me.
> 
> Not from btrace's perspective and the code is in btrace.  Python just uses
> it in a particular way.

I get your argumentation. I still think ptw_filter is perfectly fine. As that is
what it will always be, even for other extension languages. I see it more
from a "global GDB" perspective, rather than "btrace vs python" or from a
"callback concept" POV.
Regardless, I will call it to ptw_context in the next revision. Is that ok?


> >> >+def _update_filter_dict(thread_list):
> >> >+    """Helper function to update the filter dict.
> >> >+
> >> >+    Discards filter copies of threads that already exited and registers
> >> >+    copies of the filter for new threads."""
> >> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
> >> >+    lwp_list = [i.ptid[1] for i in thread_list]
> >> >+
> >> >+    # clean-up old filters
> >> >+    for key in _ptwrite_filter.keys():
> >> >+      if key not in lwp_list and key != "global":
> >> >+        _ptwrite_filter.pop(key)
> >> >+
> >> >+    # Register filter for new threads
> >> >+    for key in lwp_list:
> >> >+        if key not in _ptwrite_filter.keys():
> >> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> >>
> >> This function is called two times: once after we cleared all filters, and
> >> once when looking up the filter for a given thread.  The first time, we
> >> know that there are no existing filters; the second time, we are really
> >> only interested in a single filter.
> >>
> >> Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
> >> exist, create a new one?
> >
> >Yes, we could get rid of the call to _update_filter_dict() in register_filter().
> >The main reason I added it was to clean the obsolete filters whenever
> >possible. I don't see a clear performance advantage if we would remove
> >the call (without having a thread exit notification).
> >We need to clean up the same amount of filters at some point.
> >
> >> That leaves removing obsolete filters.  Could this be done with some
> >> thread notification?
> >
> >IIRC, you suggested this previously. I replied that there is no python API
> >that I am aware of that can do this. The python events API doesn't expose
> >thread exited events.
> 
> I keep stumbling over this.
> 
> When looking up a filter, we are clearly only interested in one thread.
> Just looking up that one and creating it when it is missing seems a lot
> more straight forward.
> 
> Lacking a thread exit notification, we could still add a _prune_filters
> function that we call every now and then that just removes filters for
> exited threads.
> 
> Does that sound reasonable?  We'd need to find good places to call
> it from.

To me that is kind of what I have implemented now. Just not with a
separate _prune_filters() and doing it in "two good places".

But I just realized that having it in get_filter() would only improve
performance if someone would call get_filter() from python directly.
Which probably isn't a scenario worth optimizing for.

Are you okay with changing get_filter and inlining _update_filter_dict?

--- a/gdb/python/lib/gdb/ptwrite.py
+++ b/gdb/python/lib/gdb/ptwrite.py
@@ -29,25 +29,6 @@ def default_filter(payload, ip):
 _ptwrite_filter = {"global" : default_filter}
 
 
-def _update_filter_dict(thread_list):
-    """Helper function to update the filter dict.
-
-    Discards filter copies of threads that already exited and registers
-    copies of the filter for new threads."""
-    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
-    lwp_list = [i.ptid[1] for i in thread_list]
-
-    # Clean-up old filters
-    for key in _ptwrite_filter.keys():
-      if key not in lwp_list and key != "global":
-        _ptwrite_filter.pop(key)
-
-    # Register filter for new threads
-    for key in lwp_list:
-        if key not in _ptwrite_filter.keys():
-            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
-
-
 def _clear_traces(thread_list):
     """Helper function to clear the trace of all threads in THREAD_LIST."""
     current_thread = gdb.selected_thread()
@@ -74,12 +55,26 @@ def register_filter(filter):
     _ptwrite_filter.clear()
     _ptwrite_filter["global"] = filter
 
-    _update_filter_dict(thread_list)
+    # thread_list[x].ptid returns the tuple (pid, lwp, tid).
+    lwp_list = [i.ptid[1] for i in thread_list]
+
+    # Clean-up old filters.
+    for key in _ptwrite_filter.keys():
+      if key not in lwp_list and key != "global":
+        _ptwrite_filter.pop(key)
+
+    # Register filter for new threads.
+    for key in lwp_list:
+        if key not in _ptwrite_filter.keys():
+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
 
 
 def get_filter():
     """Returns the filters of the current thread."""
-    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
-    _update_filter_dict(thread_list)
+    key = gdb.selected_thread().ptid[1]
+
+    # This could be a new thread.
+    if key not in _ptwrite_filter.keys():
+        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
 
-    return _ptwrite_filter[gdb.selected_thread().ptid[1]]
+    return _ptwrite_filter[key]

Thanks,
Felix
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-07-13  8:49         ` Willgerodt, Felix
@ 2022-07-13 15:20           ` Metzger, Markus T
  2022-07-26 14:08             ` Willgerodt, Felix
  0 siblings, 1 reply; 32+ messages in thread
From: Metzger, Markus T @ 2022-07-13 15:20 UTC (permalink / raw)
  To: Willgerodt, Felix; +Cc: gdb-patches

Hello Felix,

>I get your argumentation. I still think ptw_filter is perfectly fine. As that is
>what it will always be, even for other extension languages. I see it more
>from a "global GDB" perspective, rather than "btrace vs python" or from a
>"callback concept" POV.
>Regardless, I will call it to ptw_context in the next revision. Is that ok?

That sounds OK.  How about avoiding the problem by doing it with a C++
callable?


>> >> >+def _update_filter_dict(thread_list):
>> >> >+    """Helper function to update the filter dict.
>> >> >+
>> >> >+    Discards filter copies of threads that already exited and registers
>> >> >+    copies of the filter for new threads."""
>> >> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
>> >> >+    lwp_list = [i.ptid[1] for i in thread_list]
>> >> >+
>> >> >+    # clean-up old filters
>> >> >+    for key in _ptwrite_filter.keys():
>> >> >+      if key not in lwp_list and key != "global":
>> >> >+        _ptwrite_filter.pop(key)
>> >> >+
>> >> >+    # Register filter for new threads
>> >> >+    for key in lwp_list:
>> >> >+        if key not in _ptwrite_filter.keys():
>> >> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>> >>
>> >> This function is called two times: once after we cleared all filters, and
>> >> once when looking up the filter for a given thread.  The first time, we
>> >> know that there are no existing filters; the second time, we are really
>> >> only interested in a single filter.
>> >>
>> >> Wouldn't it suffice to lookup the filter in get_filter() and, if it doesn't
>> >> exist, create a new one?
>> >
>> >Yes, we could get rid of the call to _update_filter_dict() in register_filter().
>> >The main reason I added it was to clean the obsolete filters whenever
>> >possible. I don't see a clear performance advantage if we would remove
>> >the call (without having a thread exit notification).
>> >We need to clean up the same amount of filters at some point.
>> >
>> >> That leaves removing obsolete filters.  Could this be done with some
>> >> thread notification?
>> >
>> >IIRC, you suggested this previously. I replied that there is no python API
>> >that I am aware of that can do this. The python events API doesn't expose
>> >thread exited events.
>>
>> I keep stumbling over this.
>>
>> When looking up a filter, we are clearly only interested in one thread.
>> Just looking up that one and creating it when it is missing seems a lot
>> more straight forward.
>>
>> Lacking a thread exit notification, we could still add a _prune_filters
>> function that we call every now and then that just removes filters for
>> exited threads.
>>
>> Does that sound reasonable?  We'd need to find good places to call
>> it from.
>
>To me that is kind of what I have implemented now. Just not with a
>separate _prune_filters() and doing it in "two good places".
>
>But I just realized that having it in get_filter() would only improve
>performance if someone would call get_filter() from python directly.
>Which probably isn't a scenario worth optimizing for.
>
>Are you okay with changing get_filter and inlining _update_filter_dict?

I can't say whether performance is an issue.  I stumbled over it while
trying to understand the code and wondering why we are doing all
this.

I think it would be cleaner to have a prune function and otherwise
just do what's necessary.


> def _clear_traces(thread_list):
>     """Helper function to clear the trace of all threads in THREAD_LIST."""
>     current_thread = gdb.selected_thread()
>@@ -74,12 +55,26 @@ def register_filter(filter):
>     _ptwrite_filter.clear()
>     _ptwrite_filter["global"] = filter
>
>-    _update_filter_dict(thread_list)
>+    # thread_list[x].ptid returns the tuple (pid, lwp, tid).
>+    lwp_list = [i.ptid[1] for i in thread_list]
>+
>+    # Clean-up old filters.
>+    for key in _ptwrite_filter.keys():
>+      if key not in lwp_list and key != "global":
>+        _ptwrite_filter.pop(key)

We just cleared the filters.  There are no existing filters.

>+
>+    # Register filter for new threads.
>+    for key in lwp_list:
>+        if key not in _ptwrite_filter.keys():
>+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])

New filters are added on-demand below.  I don't think this is necessary.

>
>
> def get_filter():
>     """Returns the filters of the current thread."""
>-    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
>-    _update_filter_dict(thread_list)
>+    key = gdb.selected_thread().ptid[1]
>+
>+    # This could be a new thread.
>+    if key not in _ptwrite_filter.keys():
>+        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>
>-    return _ptwrite_filter[gdb.selected_thread().ptid[1]]
>+    return _ptwrite_filter[key]

That looks good.

We're no longer pruning filters for exited threads.  We could do so on
inferior exit.  Would that suffice?

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-07-13 15:20           ` Metzger, Markus T
@ 2022-07-26 14:08             ` Willgerodt, Felix
  2022-09-14  8:37               ` Metzger, Markus T
  0 siblings, 1 reply; 32+ messages in thread
From: Willgerodt, Felix @ 2022-07-26 14:08 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 13. Juli 2022 17:21
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> Hello Felix,
> 
> >I get your argumentation. I still think ptw_filter is perfectly fine. As that is
> >what it will always be, even for other extension languages. I see it more
> >from a "global GDB" perspective, rather than "btrace vs python" or from a
> >"callback concept" POV.
> >Regardless, I will call it to ptw_context in the next revision. Is that ok?
> 
> That sounds OK.  How about avoiding the problem by doing it with a C++
> callable?

I don't think I completely follow what you are proposing or what we would
gain. I guess I can make the callback_fun e.g. a std::function or a
callable class/struct. We will still need to save a pointer for the
ptw_context/filter in btinfo (or the callable class/struct member), as we pass
that to the Python C API. To me this doesn't avoid the naming problem.

>+    # Clean-up old filters.
> >+    for key in _ptwrite_filter.keys():
> >+      if key not in lwp_list and key != "global":
> >+        _ptwrite_filter.pop(key)
> 
> We just cleared the filters.  There are no existing filters.

Right.

> >+
> >+    # Register filter for new threads.
> >+    for key in lwp_list:
> >+        if key not in _ptwrite_filter.keys():
> >+            _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> 
> New filters are added on-demand below.  I don't think this is necessary.

Right again. Thanks for the help, these two things make it much simpler.

> >
> >
> > def get_filter():
> >     """Returns the filters of the current thread."""
> >-    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
> >-    _update_filter_dict(thread_list)
> >+    key = gdb.selected_thread().ptid[1]
> >+
> >+    # This could be a new thread.
> >+    if key not in _ptwrite_filter.keys():
> >+        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> >
> >-    return _ptwrite_filter[gdb.selected_thread().ptid[1]]
> >+    return _ptwrite_filter[key]
> 
> That looks good.
> 
> We're no longer pruning filters for exited threads.  We could do so on
> inferior exit.  Would that suffice?


Pruning on inferior exit only helps in certain cases.
We can't get a complete "all-past-threads" list from GDB for an exited
inferior though. And we can't just remove every dict entry except for
"global", as we could be debugging multiple inferiors.

What I could do is add e.g. the inferior id to the dict. And prune only these on
inferior exit:

+def ptw_exit_handler(event):
+    """Exit handler to prune _ptwrite_filter on inferior exit."""
+    for key in _ptwrite_filter.keys():
+        if key.startswith(f"{event.inferior.pid}."):
+            del _ptwrite_filter[key]
+
+
 def _clear_traces(thread_list):
     """Helper function to clear the trace of all threads in THREAD_LIST."""
     current_thread = gdb.selected_thread()
@@ -59,10 +66,13 @@ def register_filter(filter):
 
 def get_filter():
     """Returns the filters of the current thread."""
-    key = gdb.selected_thread().ptid[1]
+    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
 
     # This could be a new thread.
     if key not in _ptwrite_filter.keys():
         _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
 
     return _ptwrite_filter[key]
+
+
+gdb.events.exited.connect(ptw_exit_handler)

Regards,
Felix
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration.
  2022-07-26 14:08             ` Willgerodt, Felix
@ 2022-09-14  8:37               ` Metzger, Markus T
  0 siblings, 0 replies; 32+ messages in thread
From: Metzger, Markus T @ 2022-09-14  8:37 UTC (permalink / raw)
  To: Willgerodt, Felix; +Cc: gdb-patches

Hello Felix,

Sorry for the late reply.  I just found this in my inbox.

>> >I get your argumentation. I still think ptw_filter is perfectly fine. As that is
>> >what it will always be, even for other extension languages. I see it more
>> >from a "global GDB" perspective, rather than "btrace vs python" or from a
>> >"callback concept" POV.
>> >Regardless, I will call it to ptw_context in the next revision. Is that ok?
>>
>> That sounds OK.  How about avoiding the problem by doing it with a C++
>> callable?
>
>I don't think I completely follow what you are proposing or what we would
>gain. I guess I can make the callback_fun e.g. a std::function or a
>callable class/struct. We will still need to save a pointer for the
>ptw_context/filter in btinfo (or the callable class/struct member), as we pass
>that to the Python C API. To me this doesn't avoid the naming problem.

We'd hide the context inside the object so we don't need to find a name for it.

I'm also fine with the callback/context terminology.


>Pruning on inferior exit only helps in certain cases.
>We can't get a complete "all-past-threads" list from GDB for an exited
>inferior though. And we can't just remove every dict entry except for
>"global", as we could be debugging multiple inferiors.
>
>What I could do is add e.g. the inferior id to the dict. And prune only these on
>inferior exit:

That sounds good.

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 10/10] btrace: Extend ptwrite event decoding.
  2022-06-29 13:35   ` Metzger, Markus T
@ 2022-09-19  8:59     ` Willgerodt, Felix
  0 siblings, 0 replies; 32+ messages in thread
From: Willgerodt, Felix @ 2022-09-19  8:59 UTC (permalink / raw)
  To: Metzger, Markus T; +Cc: gdb-patches

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history.
  2022-06-28  9:10   ` Metzger, Markus T
@ 2022-09-19  8:59     ` Willgerodt, Felix
  0 siblings, 0 replies; 32+ messages in thread
From: Willgerodt, Felix @ 2022-09-19  8:59 UTC (permalink / raw)
  To: Metzger, Markus T, gdb-patches

Sorry, I forgot to send these out on Friday.

Felix 

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 28. Juni 2022 11:11
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: RE: [PATCH v5 03/10] btrace: Enable auxiliary instructions in record
> function-call-history.
> 
> Hello Felix,
> 
> >Print the auxiliary data when a btrace_insn of type BTRACE_INSN_AUX
> >is encountered in the function-call-history.  Printing is
> >active by default, it can be silenced with the /a modifier.
> 
> I find it a bit strange that /a disables some output instead of enabling it.
> Should we change the default?  I'd probably always use it, but then, I
> also always supply /cli.
>

See the same comment/discussion in patch 2. We agreed that we leave it
as is, as there are already other flags that disable stuff.

> 
> >This patch is in preparation for the new ptwrite feature, which is based on
> >auxiliary instructions.
> >---
> > gdb/btrace.h        |  6 +++++-
> > gdb/doc/gdb.texinfo |  5 +++--
> > gdb/record-btrace.c | 21 +++++++++++++++++++++
> > gdb/record.c        |  5 +++++
> > gdb/record.h        |  5 ++++-
> > 5 files changed, 38 insertions(+), 4 deletions(-)
> 
> 
> >   /* The 'up' link points to a tail call.  This obviously only makes sense
> >      if bfun_up_links_to_ret is clear.  */
> >-  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1)
> >+  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1),
> >+
> >+  /* Indicates that at least one auxiliary instruction is in the current
> >+     function segment.  */
> >+  BFUN_AUX_DECODED = (1 << 2)
> 
> Should this maybe be called BFUN_CONTAINS_AUX?

Fine with me, I renamed it.

> 
> >+static void
> >+btrace_print_aux_insn (struct ui_out *uiout,
> >+		       const struct btrace_function *bfun,
> >+		       const struct btrace_thread_info *btinfo)
> >+{
> >+  for (const btrace_insn &insn : bfun->insn)
> >+    {
> >+      if (insn.iclass == BTRACE_INSN_AUX)
> >+	{
> >+	  uiout->text ("\t\t[");
> 
> This should probably do
> 
>     uiout->field_skip ("index");
>     uiout->text ("\t[");
> 

I tried it locally. This actually isn't equivalent and only one /t will be
printed. I don't think we want that.

Regards,
Felix
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

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2022-09-19  8:59 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-22 11:43 [PATCH v5 00/10] Extensions for PTWRITE Felix Willgerodt
2022-06-22 11:43 ` [PATCH v5 01/10] btrace: Introduce auxiliary instructions Felix Willgerodt
2022-06-28  9:10   ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 02/10] btrace: Enable auxiliary instructions in record instruction-history Felix Willgerodt
2022-06-28  9:10   ` Metzger, Markus T
2022-06-28 11:28     ` Willgerodt, Felix
2022-06-29 10:43       ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 03/10] btrace: Enable auxiliary instructions in record function-call-history Felix Willgerodt
2022-06-28  9:10   ` Metzger, Markus T
2022-09-19  8:59     ` Willgerodt, Felix
2022-06-22 11:43 ` [PATCH v5 04/10] btrace: Handle stepping and goto for auxiliary instructions Felix Willgerodt
2022-06-28  9:11   ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 05/10] python: Introduce gdb.RecordAuxiliary class Felix Willgerodt
2022-06-28  9:11   ` Metzger, Markus T
2022-07-11 12:48     ` Willgerodt, Felix
2022-06-22 11:43 ` [PATCH v5 06/10] python: Add clear() to gdb.Record Felix Willgerodt
2022-06-28  9:11   ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt Felix Willgerodt
2022-06-28  9:11   ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 08/10] btrace, linux: Enable ptwrite packets Felix Willgerodt
2022-06-28  9:12   ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 09/10] btrace, python: Enable ptwrite filter registration Felix Willgerodt
2022-06-28 13:59   ` Metzger, Markus T
2022-07-11 12:48     ` Willgerodt, Felix
2022-07-12 12:23       ` Metzger, Markus T
2022-07-13  8:49         ` Willgerodt, Felix
2022-07-13 15:20           ` Metzger, Markus T
2022-07-26 14:08             ` Willgerodt, Felix
2022-09-14  8:37               ` Metzger, Markus T
2022-06-22 11:43 ` [PATCH v5 10/10] btrace: Extend ptwrite event decoding Felix Willgerodt
2022-06-29 13:35   ` Metzger, Markus T
2022-09-19  8:59     ` Willgerodt, Felix

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).