[PATCH v8 00/10] Extensions for PTWRITE

[PATCH v8 00/10] Extensions for PTWRITE

[Felix Willgerodt]

Eli already approved the documentation parts.
Markus reviewed every patch and approved the btrace bits.
The only thing that is missing is a maintainers approval for the Python
bits.

v7 can be found here:
https://sourceware.org/pipermail/gdb-patches/2022-October/192830.html

I pinged this version 12 times, but didn't get any response.
Since the last time I posted this was half a year ago, I decided to re-post a
rebased version again.  I only had to solve some minor conflicts.  The
biggest change was using the new require logic for the testing.

Thanks,
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                                      |   7 +
 gdb/btrace.c                                  |  68 ++-
 gdb/btrace.h                                  |  38 +-
 gdb/config.in                                 |   3 +
 gdb/configure                                 |  11 +
 gdb/data-directory/Makefile.in                |   1 +
 gdb/disasm-flags.h                            |   1 +
 gdb/doc/gdb.texinfo                           |  32 +-
 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                 |  80 +++
 gdb/python/py-record-btrace.c                 | 136 ++++-
 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                           | 107 +++-
 gdb/record.c                                  |  10 +
 gdb/record.h                                  |   5 +-
 gdb/remote.c                                  |  33 ++
 gdb/testsuite/gdb.btrace/i386-ptwrite.S       | 550 ++++++++++++++++++
 gdb/testsuite/gdb.btrace/ptwrite.c            |  39 ++
 gdb/testsuite/gdb.btrace/ptwrite.exp          | 200 +++++++
 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S     | 544 +++++++++++++++++
 gdb/testsuite/gdb.python/py-record-btrace.exp |   6 +-
 gdb/testsuite/lib/gdb.exp                     |  72 +++
 gdbserver/linux-low.cc                        |   3 +
 gdbserver/server.cc                           |  18 +
 gdbsupport/btrace-common.h                    |   6 +
 gdbsupport/common.m4                          |   2 +
 gdbsupport/config.in                          |   3 +
 gdbsupport/configure                          |  11 +
 38 files changed, 2279 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.39.2

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

[PATCH v8 01/10] btrace: Introduce auxiliary instructions.

[Felix Willgerodt]

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 |  3 +++
 3 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 38d3882c154..b0afc1735f8 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 0ec84113217..dd80d3e6806 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 6c811b8be2e..d1690c07a99 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7639,6 +7639,9 @@ 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 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.39.2

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

[PATCH v8 02/10] btrace: Enable auxiliary instructions in record instruction-history.

[Felix Willgerodt]

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 e2e8a7a82e5..08b2d506493 100644
--- a/gdb/disasm-flags.h
+++ b/gdb/disasm-flags.h
@@ -34,6 +34,7 @@ enum gdb_disassembly_flag : unsigned
     DISASSEMBLY_SOURCE = (0x1 << 5),
     DISASSEMBLY_SPECULATIVE = (0x1 << 6),
     DISASSEMBLY_RAW_BYTES = (0x1 << 7),
+    DISASSEMBLY_OMIT_AUX_INSN = (0x1 << 8),
   };
 DEF_ENUM_FLAGS_TYPE (enum gdb_disassembly_flag, gdb_disassembly_flags);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index d1690c07a99..a3ca25577c4 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8057,6 +8057,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 2d88e4d20bf..e69004b35b5 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -826,6 +826,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 f7c95153537..c7397858717 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;
@@ -856,6 +859,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.39.2

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

[PATCH v8 03/10] btrace: Enable auxiliary instructions in record function-call-history.

[Felix Willgerodt]

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 dd80d3e6806..f6a8274bb16 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_CONTAINS_AUX = (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 a3ca25577c4..2d8cf143262 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -8113,8 +8113,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 e69004b35b5..9ec7eb1b415 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1158,6 +1158,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
@@ -1233,6 +1250,10 @@ btrace_call_history (struct ui_out *uiout,
 	}
 
       uiout->text ("\n");
+
+      if (((flags & RECORD_DONT_PRINT_AUX) == 0)
+	  && ((bfun->flags & BFUN_CONTAINS_AUX) != 0))
+	btrace_print_aux_insn(uiout, bfun, btinfo);
     }
 }
 
diff --git a/gdb/record.c b/gdb/record.c
index c7397858717..2c25dd3dfe1 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -639,6 +639,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);
 	    }
@@ -883,6 +886,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 166b5349988..ec06aaa7909 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.39.2

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

[PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary instructions.

[Felix Willgerodt]

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 | 67 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 54 insertions(+), 13 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 9ec7eb1b415..22687e889ec 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2378,9 +2378,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;
 
@@ -2388,12 +2392,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);
@@ -2424,9 +2443,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;
 
@@ -2436,8 +2458,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.
 
@@ -2859,25 +2895,30 @@ 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.39.2

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

Re: [PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary instructions.

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> 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.

Hi Felix,

Just a few minor comments.  No need to repost just for that.

> @@ -2388,12 +2392,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 ();
> +        }

These lines just (wrongfully) replaces the indentation with all spaces,
when in reality these lines don't need to be touched.

> +
> +      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 ());

I was a bit thrown off by the use of std::vector::at :P (I think it was
used in the previous patch too).  Did you use it specifically for the
behavior when the access is out of bounds?  If not, using operator[]
would probably be better (and more usual).

> @@ -2859,25 +2895,30 @@ 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."));

Just wondering, would it be useful for the user to print a more specific
error message ("Can't go to an auxiliary instruction") if they try to
goto an aux instruction?

Simon

RE: [PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary instructions.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 15:10
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 04/10] btrace: Handle stepping and goto for auxiliary
> instructions.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > 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.
> 
> Hi Felix,
> 
> Just a few minor comments.  No need to repost just for that.
> 
> > @@ -2388,12 +2392,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 ();
> > +        }
> 
> These lines just (wrongfully) replaces the indentation with all spaces,
> when in reality these lines don't need to be touched.

Will fix that! Thanks.

> > +
> > +      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 ());
> 
> I was a bit thrown off by the use of std::vector::at :P (I think it was
> used in the previous patch too).  Did you use it specifically for the
> behavior when the access is out of bounds?  If not, using operator[]
> would probably be better (and more usual).

Yes this was intentionally. I think I added it based on a previous review.

> > @@ -2859,25 +2895,30 @@ 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."));
> 
> Just wondering, would it be useful for the user to print a more specific
> error message ("Can't go to an auxiliary instruction") if they try to
> goto an aux instruction?
> 

I like the suggestion. I will split it up to keep the current message for gaps:

  const struct btrace_insn *insn = btrace_insn_get (&it);
  if (insn == NULL)
    error (_("No such instruction."));
  if (insn->iclass == BTRACE_INSN_AUX)
    error (_("Can't go to an auxiliary instruction."));

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

[PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Felix Willgerodt]

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 | 35 +++++++++++------
 gdb/python/py-record.c        | 73 ++++++++++++++++++++++++++++++++++-
 gdb/python/py-record.h        |  3 ++
 4 files changed, 111 insertions(+), 13 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 54d5660543a..31243b8ec47 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3914,6 +3914,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 4af86672d26..16925eadd7a 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,14 @@ 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 +451,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 +468,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 +658,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 +680,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 +702,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 1e40f2cded0..1afa0f25275 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 6eec71e06e7..75bf6fae935 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.39.2

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

Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> @@ -163,6 +165,14 @@ 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);

Not really important, but recpy_aux_new could take a
`const std::string &`, it would simplify callers a bit.

> +
> +

Remove one newline.

> +/* 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);
> +}

Nothing new with this patch, since this is the same pattern used for
other object, but just wondering: obj->data seems to be borrowed from
the btrace backend.  Are there some lifetime issues if you do:

1. Create a gdb.RecordAuxiliary object "b"
2. Issue a gdb command to clear the btrace data
3. Access "b.data"

?  It seems to me like obj->data might point to freed data.

Simon

RE: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 15:28
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > @@ -163,6 +165,14 @@ 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);
> 
> Not really important, but recpy_aux_new could take a
> `const std::string &`, it would simplify callers a bit.

Thanks, I will do that.

> > +
> > +
> 
> Remove one newline.

Done.

> > +/* 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);
> > +}
> 
> Nothing new with this patch, since this is the same pattern used for
> other object, but just wondering: obj->data seems to be borrowed from
> the btrace backend.  Are there some lifetime issues if you do:
> 
> 1. Create a gdb.RecordAuxiliary object "b"
> 2. Issue a gdb command to clear the btrace data
> 3. Access "b.data"
> 
> ?  It seems to me like obj->data might point to freed data.

About the pattern:
There is already "maint btrace clear" so I could test and debug this with
master. On master GDB prints an error before accessing any of these objects,
as we check if the trace is empty before accessing anything.

With my patch I could however see a nullptr access. But not at this part of the code.
I think I need to rethink the modelling of Auxiliary objects as new objects.
We model them as a class of instruction in btrace.c.
And the python lists we put them in are designed to be a list of a specific type of object.
So I can either change the way lists are for btrace recordings or model Auxiliaries
as an instruction. I will think about it and post a new revision that addresses this.

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

Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Simon Marchi]

On 3/31/23 06:58, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Freitag, 24. März 2023 15:28
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.
>>
>> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
>>> @@ -163,6 +165,14 @@ 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);
>>
>> Not really important, but recpy_aux_new could take a
>> `const std::string &`, it would simplify callers a bit.
> 
> Thanks, I will do that.
> 
>>> +
>>> +
>>
>> Remove one newline.
> 
> Done.
> 
>>> +/* 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);
>>> +}
>>
>> Nothing new with this patch, since this is the same pattern used for
>> other object, but just wondering: obj->data seems to be borrowed from
>> the btrace backend.  Are there some lifetime issues if you do:
>>
>> 1. Create a gdb.RecordAuxiliary object "b"
>> 2. Issue a gdb command to clear the btrace data
>> 3. Access "b.data"
>>
>> ?  It seems to me like obj->data might point to freed data.
> 
> About the pattern:
> There is already "maint btrace clear" so I could test and debug this with
> master. On master GDB prints an error before accessing any of these objects,
> as we check if the trace is empty before accessing anything.

Then, maybe if you record something after clearing the data?  Something
like:

 1. Create a gdb.RecordAuxiliary object "b"
 2. Issue "maint btrace clear"
 3. Do one step (to make the trace non-empty again)
 4. Access "b.data"

> With my patch I could however see a nullptr access. But not at this part of the code.
> I think I need to rethink the modelling of Auxiliary objects as new objects.
> We model them as a class of instruction in btrace.c.
> And the python lists we put them in are designed to be a list of a specific type of object.

Which list are you referring to?

Simon

RE: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Metzger, Markus T]

>>>> +/* 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);
>>>> +}
>>>
>>> Nothing new with this patch, since this is the same pattern used for
>>> other object, but just wondering: obj->data seems to be borrowed from
>>> the btrace backend.  Are there some lifetime issues if you do:
>>>
>>> 1. Create a gdb.RecordAuxiliary object "b"
>>> 2. Issue a gdb command to clear the btrace data
>>> 3. Access "b.data"
>>>
>>> ?  It seems to me like obj->data might point to freed data.
>>
>> About the pattern:
>> There is already "maint btrace clear" so I could test and debug this with
>> master. On master GDB prints an error before accessing any of these objects,
>> as we check if the trace is empty before accessing anything.
>
>Then, maybe if you record something after clearing the data?  Something
>like:
>
> 1. Create a gdb.RecordAuxiliary object "b"
> 2. Issue "maint btrace clear"
> 3. Do one step (to make the trace non-empty again)
> 4. Access "b.data"

I added the 'maint btrace clear' command to force re-decoding of the trace
for debugging purposes.  Any CLI command that uses btrace will trigger a
trace fetch and decode.

For ptwrite filters, the use-case is to replace the filter and re-decode with
the new filter in place (and the old filter removed).  I'm not against chaining
filters and decorating their aux contribution with the filter name.  IIRC this
was considered more complicated and we couldn't really find a good use-case.

The filter needs to know about the source or binary instrumentation that
added those ptwrite instructions.  It's job is to aggregate the raw data and
the IP of the ptwrite instruction that emitted that data and turn it into
something more useful than the raw hex values.

The use case for filter chaining would be independent instrumentations
active at the same time and interpreted by their respective ptwrite filter.

To the topic at hand, I'd say that record instruction or aux objects should
behave like, say, frame objects.  You cannot clear the frames to force a
re-unwind AFAIK but you can step or finish to turn a frame invalid.

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

Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Simon Marchi]

On 4/4/23 02:57, Metzger, Markus T wrote:
> 
>>>>> +/* 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);
>>>>> +}
>>>>
>>>> Nothing new with this patch, since this is the same pattern used for
>>>> other object, but just wondering: obj->data seems to be borrowed from
>>>> the btrace backend.  Are there some lifetime issues if you do:
>>>>
>>>> 1. Create a gdb.RecordAuxiliary object "b"
>>>> 2. Issue a gdb command to clear the btrace data
>>>> 3. Access "b.data"
>>>>
>>>> ?  It seems to me like obj->data might point to freed data.
>>>
>>> About the pattern:
>>> There is already "maint btrace clear" so I could test and debug this with
>>> master. On master GDB prints an error before accessing any of these objects,
>>> as we check if the trace is empty before accessing anything.
>>
>> Then, maybe if you record something after clearing the data?  Something
>> like:
>>
>> 1. Create a gdb.RecordAuxiliary object "b"
>> 2. Issue "maint btrace clear"
>> 3. Do one step (to make the trace non-empty again)
>> 4. Access "b.data"
> 
> I added the 'maint btrace clear' command to force re-decoding of the trace
> for debugging purposes.  Any CLI command that uses btrace will trigger a
> trace fetch and decode.

Ack.

> For ptwrite filters, the use-case is to replace the filter and re-decode with
> the new filter in place (and the old filter removed).

Yes, that makes sense.

> I'm not against chaining
> filters and decorating their aux contribution with the filter name.  IIRC this
> was considered more complicated and we couldn't really find a good use-case.

Just to be clear, I didn't suggest to add that.

> The filter needs to know about the source or binary instrumentation that
> added those ptwrite instructions.  It's job is to aggregate the raw data and
> the IP of the ptwrite instruction that emitted that data and turn it into
> something more useful than the raw hex values.
> 
> The use case for filter chaining would be independent instrumentations
> active at the same time and interpreted by their respective ptwrite filter.

Ack.  Or some kind of top-level filter that delegates.

> To the topic at hand, I'd say that record instruction or aux objects should
> behave like, say, frame objects.  You cannot clear the frames to force a
> re-unwind AFAIK but you can step or finish to turn a frame invalid.

Yes, that is my worry.  It seems possible for the btrace Python objects
to outlive the data they are wrapping, and I don't see any measures
taken to avoid accessing the stale data.  Usually that can be done with
an observer that will mark the Python objects invalid.

Another case: if you "continue" and a thread exits, is the btrace record
data for that thread deleted?

Simon

RE: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Dienstag, 4. April 2023 16:17
> To: Metzger, Markus T <markus.t.metzger@intel.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: Re: [PATCH v8 05/10] python: Introduce gdb.RecordAuxiliary class.
> 
> On 4/4/23 02:57, Metzger, Markus T wrote:
> >
> >>>>> +/* 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);
> >>>>> +}
> >>>>
> >>>> Nothing new with this patch, since this is the same pattern used for
> >>>> other object, but just wondering: obj->data seems to be borrowed
> from
> >>>> the btrace backend.  Are there some lifetime issues if you do:
> >>>>
> >>>> 1. Create a gdb.RecordAuxiliary object "b"
> >>>> 2. Issue a gdb command to clear the btrace data
> >>>> 3. Access "b.data"
> >>>>
> >>>> ?  It seems to me like obj->data might point to freed data.
> >>>
> >>> About the pattern:
> >>> There is already "maint btrace clear" so I could test and debug this with
> >>> master. On master GDB prints an error before accessing any of these
> objects,
> >>> as we check if the trace is empty before accessing anything.
> >>
> >> Then, maybe if you record something after clearing the data?  Something
> >> like:
> >>
> >> 1. Create a gdb.RecordAuxiliary object "b"
> >> 2. Issue "maint btrace clear"
> >> 3. Do one step (to make the trace non-empty again)
> >> 4. Access "b.data"
> >
> > I added the 'maint btrace clear' command to force re-decoding of the trace
> > for debugging purposes.  Any CLI command that uses btrace will trigger a
> > trace fetch and decode.
> 
> Ack.
> 
> > For ptwrite filters, the use-case is to replace the filter and re-decode with
> > the new filter in place (and the old filter removed).
> 
> Yes, that makes sense.
> 
> > I'm not against chaining
> > filters and decorating their aux contribution with the filter name.  IIRC this
> > was considered more complicated and we couldn't really find a good use-
> case.
> 
> Just to be clear, I didn't suggest to add that.
> 
> > The filter needs to know about the source or binary instrumentation that
> > added those ptwrite instructions.  It's job is to aggregate the raw data and
> > the IP of the ptwrite instruction that emitted that data and turn it into
> > something more useful than the raw hex values.
> >
> > The use case for filter chaining would be independent instrumentations
> > active at the same time and interpreted by their respective ptwrite filter.
> 
> Ack.  Or some kind of top-level filter that delegates.
> 
> > To the topic at hand, I'd say that record instruction or aux objects should
> > behave like, say, frame objects.  You cannot clear the frames to force a
> > re-unwind AFAIK but you can step or finish to turn a frame invalid.
> 
> Yes, that is my worry.  It seems possible for the btrace Python objects
> to outlive the data they are wrapping, and I don't see any measures
> taken to avoid accessing the stale data.  Usually that can be done with
> an observer that will mark the Python objects invalid.
> 
> Another case: if you "continue" and a thread exits, is the btrace record
> data for that thread deleted?
> 
> Simon

I looked at it a bit more yesterday.

The gap objects get a string literal (const char *). Which have a guaranteed
lifetime over the full program afaik and therefore pointers are never dangling.
I didn't check if we wouldn't error out first, as I don't know how to reliably
record gaps.

The insn and func objects cannot be accessed after clear.
Any commands will show:

"gdb.error: No such function segment."
or
"gdb.error: No such instruction."

Even if you "save" it in a separate python variable, e.g. like "b" in Simon's list.

For auxiliaries in my patch however, Simon is right. We just pass a pointer
to an element in a vector of strings (aux_data) that is allocated in btrace.c.
We clear that vector with clear. I debugged GDB, in that case we do still point
to the same address and just read a string from it. No matter what is at that
address now:

>>> r = gdb.current_recording()
>>> i = r.instruction_history
>>> a = i[9]
>>> a.data

Thread 1 "gdb-up" hit Breakpoint 1, recpy_aux_data (
    self=<gdb.RecordAuxiliary at remote 0x7fffe8152710>, closure=0x0)
    at gdb/gdb/python/py-record.c:547
547       const recpy_aux_object * const obj = (const recpy_aux_object *) self;
(gdb) n
549       return PyUnicode_FromString (obj->data);
(gdb) p obj.data
$1 = 0x2d1ade0 "42"
(gdb) c
Continuing.
'42'
>>> r.clear()
>>> a.data

Thread 1 "gdb-up" hit Breakpoint 1, recpy_aux_data (
    self=<gdb.RecordAuxiliary at remote 0x7fffe8152710>, closure=0x0)
    at gdb/gdb/python/py-record.c:547
547       const recpy_aux_object * const obj = (const recpy_aux_object *) self;
(gdb) n
549       return PyUnicode_FromString (obj->data);
(gdb) p obj.data
$2 = 0x2d1ade0 "42"
(gdb) c
Continuing.
'42'

So I should really create a copy or error out if the trace
was cleared instead. Probably the latter to be consistent.

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

[PATCH v8 06/10] python: Add clear() to gdb.Record.

[Felix Willgerodt]

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 31243b8ec47..e1ab3b20eda 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3858,6 +3858,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 16925eadd7a..4922154fc1b 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -802,6 +802,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 0af8153b715..f297772f946 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 1afa0f25275..d5bc1c7b2aa 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 bd397d3c974..1f0539466af 100644
--- a/gdb/testsuite/gdb.python/py-record-btrace.exp
+++ b/gdb/testsuite/gdb.python/py-record-btrace.exp
@@ -143,7 +143,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.39.2

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

Re: [PATCH v8 06/10] python: Add clear() to gdb.Record.

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> This function allows to clear the trace data from python, forcing to
> re-decode the trace for successive commands.

Can you describe the use case for this?  When would the data associated
to a recording be stale and need to be re-decoded?

Simon

RE: [PATCH v8 06/10] python: Add clear() to gdb.Record.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 15:37
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 06/10] python: Add clear() to gdb.Record.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > This function allows to clear the trace data from python, forcing to
> > re-decode the trace for successive commands.
> 
> Can you describe the use case for this?  When would the data associated
> to a recording be stale and need to be re-decoded?
> 

We only need it right now when the ptwrite filter function is changing.
In the future there might be other "auxiliary functionality" that could use this.
But with the commands right now there isn't really any other scenario I think.

Note that there is also "maint btrace clear" in CLI, which is equivalent, but
a maintenance command of course.

I will add a sentence to the commit-msg, that this will be used by the future
ptwrite patches.

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

[PATCH v8 07/10] btrace, gdbserver: Add ptwrite to btrace_config_pt.

[Felix Willgerodt]

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                 |  8 +++++++-
 gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
 gdb/features/btrace-conf.dtd |  1 +
 gdb/remote.c                 | 33 +++++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc       |  3 +++
 gdbserver/server.cc          | 18 ++++++++++++++++++
 gdbsupport/btrace-common.h   |  6 ++++++
 7 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index b0afc1735f8..f2fc4786e21 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,16 @@ 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_enum,
+    gdb_xml_enums_boolean },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 2d8cf143262..cd2313cdb6a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -43779,6 +43779,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{-}
@@ -44090,6 +44095,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.
@@ -44597,6 +44605,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{(yes|no)}
+Indicate support for @code{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
@@ -47245,6 +47265,7 @@ The formal DTD for the branch trace configuration format is given below:
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	(yes | no)	#IMPLIED>
 @end smallexample
 
 @include agentexpr.texi
diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
index 3fbe3a4dc32..7aeaa4695e1 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	(yes | no) #IMPLIED>
diff --git a/gdb/remote.c b/gdb/remote.c
index 526df313ea7..d0b96f2f2f6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -286,6 +286,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,
 
@@ -5599,6 +5602,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 },
@@ -14213,6 +14218,31 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
 
       rs->btrace_config.pt.size = conf->pt.size;
     }
+
+  if ((m_features.packet_support (PACKET_Qbtrace_conf_pt_ptwrite)
+       == PACKET_ENABLE)
+	&& conf->pt.ptwrite != rs->btrace_config.pt.ptwrite)
+    {
+      pos = buf;
+      const char *ptw = conf->pt.ptwrite ? "yes" : "no";
+      const char *name
+	= packets_descriptions[PACKET_Qbtrace_conf_pt_ptwrite].name;
+      pos += xsnprintf (pos, endbuf - pos, "%s=\"%s\"", name, ptw);
+
+      putpkt (buf);
+      getpkt (&rs->buf, 0);
+
+      if (m_features.packet_ok (buf, PACKET_Qbtrace_conf_pt_ptwrite)
+	  == 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.  */
@@ -15392,6 +15422,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (PACKET_Qbtrace_conf_pt_size, "Qbtrace-conf:pt:size",
 			 "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (PACKET_Qbtrace_conf_pt_ptwrite, "Qbtrace-conf:pt:ptwrite",
+			 "btrace-conf-pt-ptwrite", 0);
+
   add_packet_config_cmd (PACKET_vContSupported, "vContSupported",
 			 "verbose-resume-supported", 0);
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index e6a39202a98..12ba2314308 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -6876,6 +6876,9 @@ linux_process_target::read_btrace_conf (const btrace_target_info *tinfo,
 	  string_xml_appendf (*buffer, "<pt");
 	  string_xml_appendf (*buffer, " size=\"0x%x\"", conf->pt.size);
 	  string_xml_appendf (*buffer, "/>\n");
+	  string_xml_appendf (*buffer, " ptwrite=\"%s\"",
+			     conf->pt.ptwrite ? "yes" : "no");
+	  string_xml_appendf (*buffer, "/>\n");
 	  break;
 	}
     }
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 949849b63a2..4e62c1a4ced 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -547,6 +547,23 @@ 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)
+    {
+      bool ptwrite;
+
+      op += strlen ("pt:ptwrite=");
+      if (strncmp (op, "\"yes\"", strlen ("\"yes\"")) == 0)
+	ptwrite = true;
+      else if (strncmp (op, "\"no\"", strlen ("\"no\"")) == 0)
+	ptwrite = false;
+      else
+	{
+	  strcpy (own_buf, "E.Bad ptwrite value.");
+	  return -1;
+	}
+
+      current_btrace_conf.pt.ptwrite = ptwrite;
+    }
   else
     {
       strcpy (own_buf, "E.Bad Qbtrace configuration option.");
@@ -2146,6 +2163,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 e287c93a6c1..03cc60648e9 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 both gdb and gdbserver support this, gdb will try to enable ptwrite
+     packets when tracing is started.  */
+  bool ptwrite;
 };
 
 /* A branch tracing configuration.
-- 
2.39.2

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

[PATCH v8 08/10] btrace, linux: Enable ptwrite packets.

[Felix Willgerodt]

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 c5b3f1c93cf..e320eaa12ce 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -417,6 +417,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.  */
 
@@ -626,6 +649,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 22687e889ec..0b36ce5fdf6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -3291,4 +3291,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.39.2

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

[PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Felix Willgerodt]

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                   |  4 ++
 gdb/btrace.h                   |  8 ++++
 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  | 80 +++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.c  | 88 ++++++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.h  |  8 ++++
 gdb/python/python-internal.h   |  3 ++
 gdb/python/python.c            |  2 +
 12 files changed, 216 insertions(+)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py

diff --git a/gdb/btrace.c b/gdb/btrace.c
index f2fc4786e21..37dd0b666d8 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,9 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
   uint64_t offset;
   int status;
 
+  /* Register the ptwrite filter.  */
+  apply_ext_lang_ptwrite_filter (btinfo);
+
   for (;;)
     {
       struct pt_insn insn;
diff --git a/gdb/btrace.h b/gdb/btrace.h
index f6a8274bb16..912cb16056a 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -352,6 +352,14 @@ 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.  */
+  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
+				   const void *ptw_context) = nullptr;
+
+  /* Context for the ptw_callback_fun.  */
+  void *ptw_context = 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 ff1340c44c0..6ba880c3b6b 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 23a9f646d12..75112afd3ab 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 4ac6e0b6732..8b32c7e1f13 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -551,6 +551,19 @@ apply_ext_lang_frame_filter (frame_info_ptr 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 ab83f9c6a28..639093945e4 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 887b7fa5dc8..e9b0d4127d5 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_filter, */
 
   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..0f5b0473023
--- /dev/null
+++ b/gdb/python/lib/gdb/ptwrite.py
@@ -0,0 +1,80 @@
+# 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."""
+
+from copy import deepcopy
+import gdb
+
+
+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 ptwrite_exit_handler(event):
+    """Exit handler to prune _ptwrite_filter on inferior exit."""
+    for key in list(_ptwrite_filter.keys()):
+        if key.startswith(f"{event.inferior.pid}."):
+            del _ptwrite_filter[key]
+
+
+gdb.events.exited.connect(ptwrite_exit_handler)
+
+
+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("The filter must be callable or 'None'.")
+
+    # Clear the traces of all threads to force re-decoding with
+    # the new filter.
+    thread_list = gdb.selected_inferior().threads()
+    _clear_traces(thread_list)
+
+    _ptwrite_filter.clear()
+    _ptwrite_filter["global"] = filter_
+
+
+def get_filter():
+    """Returns the filters of the current thread."""
+    # The key is of this format to enable an per-inferior cleanup when an
+    # inferior exits.
+    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
+
+    # Create a new filter for new threads.
+    if key not in _ptwrite_filter.keys():
+        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
+
+    return _ptwrite_filter[key]
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 4922154fc1b..100a9ee8578 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -763,6 +763,94 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
   return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
 }
 
+/* Helper function that calls PTW_FILTER with PAYLOAD and IP as arguments.
+   Returns the string that will be printed.  */
+std::string
+recpy_call_filter (const uint64_t payload, const uint64_t ip,
+		   const void *ptw_filter)
+{
+  std::string result;
+
+  if ((PyObject *) ptw_filter == nullptr)
+    error (_("No valid ptwrite filter."));
+  if ((PyObject *) ptw_filter == Py_None)
+    return result;
+
+  gdbpy_enter enter_py;
+
+  gdbpy_ref<> py_payload (PyLong_FromUnsignedLongLong (payload));
+  gdbpy_ref<> py_ip (PyLong_FromUnsignedLongLong (ip));
+
+  if (ip == 0)
+    py_ip = gdbpy_ref<>::new_reference (Py_None);
+
+  gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *) ptw_filter,
+							py_payload.get (),
+							py_ip.get (),
+							nullptr));
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      gdbpy_error (_("Couldn't call the ptwrite filter."));
+    }
+
+  /* Py_None is valid and results in no output.  */
+  if (py_result == Py_None)
+    return result;
+
+  result = gdbpy_obj_to_string (py_result.get ()).get ();
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      gdbpy_error (_("The ptwrite filter didn't return a string."));
+    }
+
+  return result;
+}
+
+/* Helper function returning the current ptwrite filter.  */
+
+PyObject *
+get_ptwrite_filter ()
+{
+  gdbpy_ref<> module (PyImport_ImportModule ("gdb.ptwrite"));
+
+  if (PyErr_Occurred ())
+  {
+    gdbpy_print_stack ();
+    return nullptr;
+  }
+
+  /* We need to keep the reference count.  */
+  gdbpy_ref<> ptw_filter (PyObject_CallMethod (module.get (), "get_filter",
+					       nullptr));
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      gdbpy_error (_("Couldn't get the ptwrite filter."));
+    }
+
+  return ptw_filter.get();
+}
+
+/* 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)
+{
+  gdb_assert (btinfo != nullptr);
+
+  gdbpy_enter enter_py;
+
+  btinfo->ptw_callback_fun = &recpy_call_filter;
+  btinfo->ptw_context= 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 f297772f946..12750b3b1c3 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 ();
+
+/* Helper function to call 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 258f5c42537..c21232c07dc 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -376,6 +376,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 *,
    frame_info_ptr frame, frame_filter_flags flags,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index b295ff88743..9a167ae9026 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.39.2

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

Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> 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                   |  4 ++
>  gdb/btrace.h                   |  8 ++++
>  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  | 80 +++++++++++++++++++++++++++++++
>  gdb/python/py-record-btrace.c  | 88 ++++++++++++++++++++++++++++++++++
>  gdb/python/py-record-btrace.h  |  8 ++++
>  gdb/python/python-internal.h   |  3 ++
>  gdb/python/python.c            |  2 +
>  12 files changed, 216 insertions(+)
>  create mode 100644 gdb/python/lib/gdb/ptwrite.py
> 
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index f2fc4786e21..37dd0b666d8 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,9 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
>    uint64_t offset;
>    int status;
>  
> +  /* Register the ptwrite filter.  */
> +  apply_ext_lang_ptwrite_filter (btinfo);
> +
>    for (;;)
>      {
>        struct pt_insn insn;
> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index f6a8274bb16..912cb16056a 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -352,6 +352,14 @@ 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.  */
> +  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t ip,
> +				   const void *ptw_context) = nullptr;
> +
> +  /* Context for the ptw_callback_fun.  */
> +  void *ptw_context = 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 ff1340c44c0..6ba880c3b6b 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 23a9f646d12..75112afd3ab 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 4ac6e0b6732..8b32c7e1f13 100644
> --- a/gdb/extension.c
> +++ b/gdb/extension.c
> @@ -551,6 +551,19 @@ apply_ext_lang_frame_filter (frame_info_ptr 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 ab83f9c6a28..639093945e4 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 887b7fa5dc8..e9b0d4127d5 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_filter, */
>  
>    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..0f5b0473023
> --- /dev/null
> +++ b/gdb/python/lib/gdb/ptwrite.py
> @@ -0,0 +1,80 @@
> +# 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."""
> +
> +from copy import deepcopy
> +import gdb
> +
> +
> +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 ptwrite_exit_handler(event):
> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> +    for key in list(_ptwrite_filter.keys()):

I don't think the list() is needed.

> +        if key.startswith(f"{event.inferior.pid}."):

Instead of using a string as a key, you could use a tuple, (pid, lwp).
So here you would check key[0].

> +            del _ptwrite_filter[key]
> +
> +
> +gdb.events.exited.connect(ptwrite_exit_handler)
> +
> +
> +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()

I guess that answers my question on the previous patch about the use
case of that clear method.  If that's the only goal of the clear method,
and if there are no use case for the user to call it, I think it would
be better to have an internal function instead.

Can you explain why its needed to switch threads and call clear on the
same recording object, but once with each thread selected?  I thought
the recording would be per-inferior, and one call to clear would clear
the data for all threads of that inferior.

> +
> +        current_thread.switch()
> +
> +
> +def register_filter(filter_):
> +    """Register the ptwrite filter function."""
> +    if filter_ is not None and not callable(filter_):
> +        raise TypeError("The filter must be callable or 'None'.")
> +
> +    # Clear the traces of all threads to force re-decoding with
> +    # the new filter.
> +    thread_list = gdb.selected_inferior().threads()
> +    _clear_traces(thread_list)

Could we avoid relying on the selected inferior?  For instance, it could
be a method in Inferior:

  inf.register_ptwrite_filter(f)

or pass an inferior to this function:

  ptwrite.register_filter(inf, f)

I don't like having to switch inferior all the time in GDB to do
per-inferior stuff, I don't want to impose that to our Python users :).

> +
> +    _ptwrite_filter.clear()
> +    _ptwrite_filter["global"] = filter_

Hmm, above you (seem to) clear the data for all threads of the selected
inferior, implying that setting a filter affects only the current
inferior.  But here you clear the whole _ptwrite_filter, which could
contain filters for other inferiors.  I don't understand.

> +
> +
> +def get_filter():
> +    """Returns the filters of the current thread."""
> +    # The key is of this format to enable an per-inferior cleanup when an
> +    # inferior exits.
> +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"

I don't think we can use f-strings, we claim to support back to Python
3.4.

You could perhaps also pass the key or the pid + lwp from the C++ code
(taken from inferior_ptid).  Calling selected_inferior twice is just
more churn to get the same info.

> +
> +    # Create a new filter for new threads.
> +    if key not in _ptwrite_filter.keys():

.keys() is not needed

> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])

Why is the deepcopy needed.  Because users could register a complex
object with a __call__ method, in which case a simple copy would not do
the same as deepcopy?  Just wondering if that's what users would expect.

> +
> +    return _ptwrite_filter[key]
> diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
> index 4922154fc1b..100a9ee8578 100644
> --- a/gdb/python/py-record-btrace.c
> +++ b/gdb/python/py-record-btrace.c
> @@ -763,6 +763,94 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
>    return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
>  }
>  
> +/* Helper function that calls PTW_FILTER with PAYLOAD and IP as arguments.
> +   Returns the string that will be printed.  */
> +std::string
> +recpy_call_filter (const uint64_t payload, const uint64_t ip,
> +		   const void *ptw_filter)

Should be static.

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

I think this can only happen if get_ptwrite_filter has failed, in
gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
written as:

    btinfo->ptw_context = get_ptwrite_filter ();
    if (btinfo->ptw_context != nullptr)
      btinfo->ptw_callback_fun = &recpy_call_filter;

... such that recpy_call_filter is only installed when there is
something in ptw_context.  recpy_call_filter could then do:

  gdb_assert (ptw_filter != nullptr);

> +  if ((PyObject *) ptw_filter == Py_None)
> +    return result;
> +
> +  gdbpy_enter enter_py;
> +
> +  gdbpy_ref<> py_payload (PyLong_FromUnsignedLongLong (payload));
> +  gdbpy_ref<> py_ip (PyLong_FromUnsignedLongLong (ip));
> +
> +  if (ip == 0)
> +    py_ip = gdbpy_ref<>::new_reference (Py_None);
> +
> +  gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *) ptw_filter,
> +							py_payload.get (),
> +							py_ip.get (),
> +							nullptr));
> +
> +  if (PyErr_Occurred ())
> +    {
> +      gdbpy_print_stack ();
> +      gdbpy_error (_("Couldn't call the ptwrite filter."));
> +    }
> +
> +  /* Py_None is valid and results in no output.  */
> +  if (py_result == Py_None)
> +    return result;
> +
> +  result = gdbpy_obj_to_string (py_result.get ()).get ();
> +
> +  if (PyErr_Occurred ())
> +    {
> +      gdbpy_print_stack ();
> +      gdbpy_error (_("The ptwrite filter didn't return a string."));
> +    }
> +
> +  return result;
> +}
> +
> +/* Helper function returning the current ptwrite filter.  */
> +
> +PyObject *
> +get_ptwrite_filter ()

Should be static.

> +/* 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)
> +{
> +  gdb_assert (btinfo != nullptr);
> +
> +  gdbpy_enter enter_py;
> +
> +  btinfo->ptw_callback_fun = &recpy_call_filter;
> +  btinfo->ptw_context= get_ptwrite_filter ();

Missing space.

> diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-btrace.h
> index f297772f946..12750b3b1c3 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 ();
> +
> +/* Helper function to call 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 258f5c42537..c21232c07dc 100644
> --- a/gdb/python/python-internal.h
> +++ b/gdb/python/python-internal.h
> @@ -376,6 +376,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);

Can this declaration be in py-record-btrace.h?  I really don't see why
all the declarations are stuffed into python.h.  Also, please move the
comment to the declaration, and put the appropriate /* See ... */
comment on the definition.

Simon

RE: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 16:23
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > 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                   |  4 ++
> >  gdb/btrace.h                   |  8 ++++
> >  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  | 80
> +++++++++++++++++++++++++++++++
> >  gdb/python/py-record-btrace.c  | 88
> ++++++++++++++++++++++++++++++++++
> >  gdb/python/py-record-btrace.h  |  8 ++++
> >  gdb/python/python-internal.h   |  3 ++
> >  gdb/python/python.c            |  2 +
> >  12 files changed, 216 insertions(+)
> >  create mode 100644 gdb/python/lib/gdb/ptwrite.py
> >
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index f2fc4786e21..37dd0b666d8 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,9 @@ ftrace_add_pt (struct btrace_thread_info
> *btinfo,
> >    uint64_t offset;
> >    int status;
> >
> > +  /* Register the ptwrite filter.  */
> > +  apply_ext_lang_ptwrite_filter (btinfo);
> > +
> >    for (;;)
> >      {
> >        struct pt_insn insn;
> > diff --git a/gdb/btrace.h b/gdb/btrace.h
> > index f6a8274bb16..912cb16056a 100644
> > --- a/gdb/btrace.h
> > +++ b/gdb/btrace.h
> > @@ -352,6 +352,14 @@ 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.  */
> > +  std::string (*ptw_callback_fun) (const uint64_t payload, const uint64_t
> ip,
> > +				   const void *ptw_context) = nullptr;
> > +
> > +  /* Context for the ptw_callback_fun.  */
> > +  void *ptw_context = 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 ff1340c44c0..6ba880c3b6b 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 23a9f646d12..75112afd3ab 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 4ac6e0b6732..8b32c7e1f13 100644
> > --- a/gdb/extension.c
> > +++ b/gdb/extension.c
> > @@ -551,6 +551,19 @@ apply_ext_lang_frame_filter (frame_info_ptr
> 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 ab83f9c6a28..639093945e4 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 887b7fa5dc8..e9b0d4127d5 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_filter, */
> >
> >    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..0f5b0473023
> > --- /dev/null
> > +++ b/gdb/python/lib/gdb/ptwrite.py
> > @@ -0,0 +1,80 @@
> > +# 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."""
> > +
> > +from copy import deepcopy
> > +import gdb
> > +
> > +
> > +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 ptwrite_exit_handler(event):
> > +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> > +    for key in list(_ptwrite_filter.keys()):
> 
> I don't think the list() is needed.

It actually is needed, as I re-size the array:

>>> mydict = {1: "one", 2: "two"}
>>> for k in mydict.keys():
...     if k == 2:
...         del mydict[k]
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: dictionary changed size during iteration

> > +        if key.startswith(f"{event.inferior.pid}."):
> 
> Instead of using a string as a key, you could use a tuple, (pid, lwp).
> So here you would check key[0].

Done. I will use (-1, -1) as the key for the template filter.

> > +            del _ptwrite_filter[key]
> > +
> > +
> > +gdb.events.exited.connect(ptwrite_exit_handler)
> > +
> > +
> > +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()
> 
> I guess that answers my question on the previous patch about the use
> case of that clear method.  If that's the only goal of the clear method,
> and if there are no use case for the user to call it, I think it would
> be better to have an internal function instead.
> 
> Can you explain why its needed to switch threads and call clear on the
> same recording object, but once with each thread selected?  I thought
> the recording would be per-inferior, and one call to clear would clear
> the data for all threads of that inferior.

The recordings are per thread not per Inferior. Though I don't think you
can actually record only one thread of an inferior, as GDB will always
start a recording for all threads, or start recording a new thread if the
existing thread is already recording.

We want to allow internal state of a filter function to be maintained
per thread. Hence the copy and clearing for all threads of the inferior.
I implemented the clear() patch based on feedback from Markus.
Note that there is already "maintenance btrace clear" in CLI.

That said, you are right, I shouldn't clear the same object. In the first
version of this series I still had it right, but changed it for V2.
I can't exactly remember why.

This still leaves the problem of multiple inferiors, that you also
commented on below.
I think our actual idea was to have this globally, for all inferiors.
But that means I would need to clear all traces for all inferiors above.
Not just all threads of the current inferior.
That can easily be done, unless our other discussions go into a different
direction, and we make this per-inferior.

So I would change this to something like this:

def _clear_traces():
    """Helper function to clear the trace of all threads."""
    current_thread = gdb.selected_thread()

    for inferior in gdb.inferiors():
        for thread in inferior.threads():
            thread.switch()
            recording = gdb.current_recording()
            if recording is not None:
                recording.clear()

    current_thread.switch()

Not sure if it is worth exiting early per inferior. I couldn't find
a scenario where you can selectively record (or not record)
one thread of a multi-threaded inferior.

> > +
> > +        current_thread.switch()
> > +
> > +
> > +def register_filter(filter_):
> > +    """Register the ptwrite filter function."""
> > +    if filter_ is not None and not callable(filter_):
> > +        raise TypeError("The filter must be callable or 'None'.")
> > +
> > +    # Clear the traces of all threads to force re-decoding with
> > +    # the new filter.
> > +    thread_list = gdb.selected_inferior().threads()
> > +    _clear_traces(thread_list)
> 
> Could we avoid relying on the selected inferior?  For instance, it could
> be a method in Inferior:
> 
>   inf.register_ptwrite_filter(f)
> 
> or pass an inferior to this function:
> 
>   ptwrite.register_filter(inf, f)
> 
> I don't like having to switch inferior all the time in GDB to do
> per-inferior stuff, I don't want to impose that to our Python users :).

I get where you are coming from. Though one could argue that this is already
the case today. Starting recordings is already per inferior.
If we have two inferiors and start recording one we don't automatically
record the second one. And if we have one inferior that is being recorded,
and add a second, the second also won't be recorded. You also don't have
any option, apart from switching inferiors and starting the recording manually.

For threads that is different. Spawning threads will also be recorded
automatically. And if you have two threads and start recording, both
will be recorded.

What you suggest is definitely possible, years ago I had a version that
would allow registering different filters per thread, but we decided against
that.

> > +
> > +    _ptwrite_filter.clear()
> > +    _ptwrite_filter["global"] = filter_
> 
> Hmm, above you (seem to) clear the data for all threads of the selected
> inferior, implying that setting a filter affects only the current
> inferior.  But here you clear the whole _ptwrite_filter, which could
> contain filters for other inferiors.  I don't understand.

You are right, That seems confusing. See the comment two above for an
answer.

> > +
> > +
> > +def get_filter():
> > +    """Returns the filters of the current thread."""
> > +    # The key is of this format to enable an per-inferior cleanup when an
> > +    # inferior exits.
> > +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
> 
> I don't think we can use f-strings, we claim to support back to Python
> 3.4.

Did we write down what we support clearly? The gdb/README actually says 3.2.

And I think it might be good to mention this here:
https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

It seems like there are already some f strings in the master branch:

./lib/gdb/dap/io.py:63:            header = f"Content-Length: {len(body_bytes)}\r\n\r\n"
./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")

I just did a quick grep and didn't look at all occasions.

That said, I will remove f strings.

> You could perhaps also pass the key or the pid + lwp from the C++ code
> (taken from inferior_ptid).  Calling selected_inferior twice is just
> more churn to get the same info.

This isn't supposed to be limited to be called from C++ code.
And I think having it without any arguments is a bit easier to use from python.
I changed it according to your prior suggestions and made it a tuple.
But I guess this also depends on our per-inferior discussion above.

> > +
> > +    # Create a new filter for new threads.
> > +    if key not in _ptwrite_filter.keys():
> 
> .keys() is not needed

Right, I removed it.

> > +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> 
> Why is the deepcopy needed.  Because users could register a complex
> object with a __call__ method, in which case a simple copy would not do
> the same as deepcopy?  Just wondering if that's what users would expect.

We wanted to maintain internal state per thread in an easy fashion.
As you already saw in Patch 10.

> > +
> > +    return _ptwrite_filter[key]
> > diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-
> btrace.c
> > index 4922154fc1b..100a9ee8578 100644
> > --- a/gdb/python/py-record-btrace.c
> > +++ b/gdb/python/py-record-btrace.c
> > @@ -763,6 +763,94 @@ recpy_bt_function_call_history (PyObject *self,
> void *closure)
> >    return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
> >  }
> >
> > +/* Helper function that calls PTW_FILTER with PAYLOAD and IP as
> arguments.
> > +   Returns the string that will be printed.  */
> > +std::string
> > +recpy_call_filter (const uint64_t payload, const uint64_t ip,
> > +		   const void *ptw_filter)
> 
> Should be static.

Done.

> > +{
> > +  std::string result;
> > +
> > +  if ((PyObject *) ptw_filter == nullptr)
> > +    error (_("No valid ptwrite filter."));
> 
> I think this can only happen if get_ptwrite_filter has failed, in
> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
> written as:
> 
>     btinfo->ptw_context = get_ptwrite_filter ();
>     if (btinfo->ptw_context != nullptr)
>       btinfo->ptw_callback_fun = &recpy_call_filter;
> 
> ... such that recpy_call_filter is only installed when there is
> something in ptw_context.  recpy_call_filter could then do:
> 
>   gdb_assert (ptw_filter != nullptr);

I guess I could do that. Yet we check ptw_callback_fun before calling it
in btrace.c. See patch 10. So that alone would actually hide problems to the user.
And just not print any ptwrite string silently. Even if the user gave a custom
listener function. So we would also need to remove that check.

I am also wondering what the advantage you see is.
That we have an assert instead of an error? I guess I could do that even
without the change in gdbpy_load_ptwrite_filter.

> > +  if ((PyObject *) ptw_filter == Py_None)
> > +    return result;
> > +
> > +  gdbpy_enter enter_py;
> > +
> > +  gdbpy_ref<> py_payload (PyLong_FromUnsignedLongLong (payload));
> > +  gdbpy_ref<> py_ip (PyLong_FromUnsignedLongLong (ip));
> > +
> > +  if (ip == 0)
> > +    py_ip = gdbpy_ref<>::new_reference (Py_None);
> > +
> > +  gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *)
> ptw_filter,
> > +							py_payload.get (),
> > +							py_ip.get (),
> > +							nullptr));
> > +
> > +  if (PyErr_Occurred ())
> > +    {
> > +      gdbpy_print_stack ();
> > +      gdbpy_error (_("Couldn't call the ptwrite filter."));
> > +    }
> > +
> > +  /* Py_None is valid and results in no output.  */
> > +  if (py_result == Py_None)
> > +    return result;
> > +
> > +  result = gdbpy_obj_to_string (py_result.get ()).get ();
> > +
> > +  if (PyErr_Occurred ())
> > +    {
> > +      gdbpy_print_stack ();
> > +      gdbpy_error (_("The ptwrite filter didn't return a string."));
> > +    }
> > +
> > +  return result;
> > +}
> > +
> > +/* Helper function returning the current ptwrite filter.  */
> > +
> > +PyObject *
> > +get_ptwrite_filter ()
> 
> Should be static.

Will be done in the next version.

> > +/* 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)
> > +{
> > +  gdb_assert (btinfo != nullptr);
> > +
> > +  gdbpy_enter enter_py;
> > +
> > +  btinfo->ptw_callback_fun = &recpy_call_filter;
> > +  btinfo->ptw_context= get_ptwrite_filter ();
> 
> Missing space.

Fixed in the next version.

> > diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-
> btrace.h
> > index f297772f946..12750b3b1c3 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 ();
> > +
> > +/* Helper function to call 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 258f5c42537..c21232c07dc 100644
> > --- a/gdb/python/python-internal.h
> > +++ b/gdb/python/python-internal.h
> > @@ -376,6 +376,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);
> 
> Can this declaration be in py-record-btrace.h?  I really don't see why
> all the declarations are stuffed into python.h.  Also, please move the
> comment to the declaration, and put the appropriate /* See ... */
> comment on the definition.

I don't think so. I modelled this after the other extension language interfaces.
These are also all in here, without any comment. This function is being called
via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
As far as I understand it, that is how you interact with the python layer.
To also allow the same things to be done in guile, if someone feels like it.
Please tell me if I misunderstood the architecture.

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

Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Simon Marchi]

>>> +def ptwrite_exit_handler(event):
>>> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
>>> +    for key in list(_ptwrite_filter.keys()):
>>
>> I don't think the list() is needed.
> 
> It actually is needed, as I re-size the array:
> 
>>>> mydict = {1: "one", 2: "two"}
>>>> for k in mydict.keys():
> ...     if k == 2:
> ...         del mydict[k]
> ... 
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> RuntimeError: dictionary changed size during iteration
Ah ok, it's to take a "snapshot" of the keys.

>> I guess that answers my question on the previous patch about the use
>> case of that clear method.  If that's the only goal of the clear method,
>> and if there are no use case for the user to call it, I think it would
>> be better to have an internal function instead.
>>
>> Can you explain why its needed to switch threads and call clear on the
>> same recording object, but once with each thread selected?  I thought
>> the recording would be per-inferior, and one call to clear would clear
>> the data for all threads of that inferior.
> 
> The recordings are per thread not per Inferior. Though I don't think you
> can actually record only one thread of an inferior, as GDB will always
> start a recording for all threads, or start recording a new thread if the
> existing thread is already recording.
> 
> We want to allow internal state of a filter function to be maintained
> per thread. Hence the copy and clearing for all threads of the inferior.
> I implemented the clear() patch based on feedback from Markus.
> Note that there is already "maintenance btrace clear" in CLI.
> 
> That said, you are right, I shouldn't clear the same object. In the first
> version of this series I still had it right, but changed it for V2.
> I can't exactly remember why.
> 
> This still leaves the problem of multiple inferiors, that you also
> commented on below.
> I think our actual idea was to have this globally, for all inferiors.
> But that means I would need to clear all traces for all inferiors above.
> Not just all threads of the current inferior.
> That can easily be done, unless our other discussions go into a different
> direction, and we make this per-inferior.
> 
> So I would change this to something like this:
> 
> def _clear_traces():
>     """Helper function to clear the trace of all threads."""
>     current_thread = gdb.selected_thread()
> 
>     for inferior in gdb.inferiors():
>         for thread in inferior.threads():
>             thread.switch()
>             recording = gdb.current_recording()
>             if recording is not None:
>                 recording.clear()
> 
>     current_thread.switch()
> 
> Not sure if it is worth exiting early per inferior. I couldn't find
> a scenario where you can selectively record (or not record)
> one thread of a multi-threaded inferior.

For our internal Python helper code, it's fine to do things based on
some internal knowledge of how GDB is implemented.  So, if we know what
doing "clear" on one thread clears the recording for the whole inferior,
then we can do just one thread per inferior.  If GDB changes, we can
adjust that code at the same time.

But seeing this, I think that from a Python user point of view, the
clear method you are proposing is a bit confusing.  It is called on a
Record object, which is per-thread, but it clears the recording for all
threads of the inferior.  This would be unexpected, unless documented as
such in the doc (but even then, it feels a bit awkward).

The doc talks about "the current recording" here and there, but it's
unclear what the scope of the recording is.

>>> +
>>> +        current_thread.switch()
>>> +
>>> +
>>> +def register_filter(filter_):
>>> +    """Register the ptwrite filter function."""
>>> +    if filter_ is not None and not callable(filter_):
>>> +        raise TypeError("The filter must be callable or 'None'.")
>>> +
>>> +    # Clear the traces of all threads to force re-decoding with
>>> +    # the new filter.
>>> +    thread_list = gdb.selected_inferior().threads()
>>> +    _clear_traces(thread_list)
>>
>> Could we avoid relying on the selected inferior?  For instance, it could
>> be a method in Inferior:
>>
>>   inf.register_ptwrite_filter(f)
>>
>> or pass an inferior to this function:
>>
>>   ptwrite.register_filter(inf, f)
>>
>> I don't like having to switch inferior all the time in GDB to do
>> per-inferior stuff, I don't want to impose that to our Python users :).
> 
> I get where you are coming from. Though one could argue that this is already
> the case today. Starting recordings is already per inferior.
> If we have two inferiors and start recording one we don't automatically
> record the second one. And if we have one inferior that is being recorded,
> and add a second, the second also won't be recorded. You also don't have
> any option, apart from switching inferiors and starting the recording manually.

I think it's a flaw in the current API that you need to switch current
inferior to start recording on a specific inferior.  And also, it's a
flaw in the documentation that it doesn't state that the recording is
started for the current inferior.  The user is left to guess (or must
experiment, or read the GDB source code, or have bad surprises) about
what happens when you have multiple inferiors.

But if I understood you clearly, register_filter is really meant to
install a global filter (valid for all inferiors, current and future).
And the call to _clear_traces is meant to clear all traces of all
inferiors.  In that case, it's fine for register_ptwrite_filter to be
global.

> For threads that is different. Spawning threads will also be recorded
> automatically. And if you have two threads and start recording, both
> will be recorded.
> 
> What you suggest is definitely possible, years ago I had a version that
> would allow registering different filters per thread, but we decided against
> that.

I don't think that is what I suggested.  If your experience suggests
that having just one global filter is enough, I can't argue with that.
Plus, it would probably be possible to implement per-inferior or
per-thread filters in the future if needed, that would take precedence
on the global filter.

My point is just that an API that requires the user to switch context,
then make an API call, then restore the original context, is bad.  It's
unclear and error prone.  I know we have some of that already, but I
would like to avoid adding more.

>>> +
>>> +
>>> +def get_filter():
>>> +    """Returns the filters of the current thread."""
>>> +    # The key is of this format to enable an per-inferior cleanup when an
>>> +    # inferior exits.
>>> +    key = f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
>>
>> I don't think we can use f-strings, we claim to support back to Python
>> 3.4.
> 
> Did we write down what we support clearly? The gdb/README actually says 3.2.

Sorry, I said 3.4 because I remembered sending a patch that raised the
minimum level to 3.4.  But that patch was rejected, so it's plausible we
still support 3.2:

  https://inbox.sourceware.org/gdb-patches/20220107152921.2858909-3-simon.marchi@polymtl.ca/#t

> And I think it might be good to mention this here:
> https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

Feel free to edit that page.  However, the risk of having it documented
at multiple places is that we can easily forget to update one spot.
Maybe put a link to the README, and say "make sure your changes are
compatible with the minimum Python version mentioned in the README"?

> It seems like there are already some f strings in the master branch:
> 
> ./lib/gdb/dap/io.py:63:            header = f"Content-Length: {len(body_bytes)}\r\n\r\n"
> ./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
> ./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")
> 
> I just did a quick grep and didn't look at all occasions.
> 
> That said, I will remove f strings.

Probably erroneous then.  Since this is specifically in DAP code, would
someone using Python 3.4 hit an error when _not_ using DAP?  I would
guess not, probably why this wasn't noticed.  Or, nobody tests with
older Python versions...

>> You could perhaps also pass the key or the pid + lwp from the C++ code
>> (taken from inferior_ptid).  Calling selected_inferior twice is just
>> more churn to get the same info.
> 
> This isn't supposed to be limited to be called from C++ code.

Ok, I didn't consider that.

One thing to note is that there isn't always a selected thread, so you
might want to handle that appropriately.

> And I think having it without any arguments is a bit easier to use from python.
> I changed it according to your prior suggestions and made it a tuple.
> But I guess this also depends on our per-inferior discussion above.

Indeed.  I had not noticed previously, but what I said earlier is true
for get_filter as well.  I would much rather see get_filter take a
thread as argument, than relying on the selected thread.

> 
>>> +
>>> +    # Create a new filter for new threads.
>>> +    if key not in _ptwrite_filter.keys():
>>> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
>>
>> Why is the deepcopy needed.  Because users could register a complex
>> object with a __call__ method, in which case a simple copy would not do
>> the same as deepcopy?  Just wondering if that's what users would expect.
> 
> We wanted to maintain internal state per thread in an easy fashion.
> As you already saw in Patch 10.

I understand that you want to have one copy of the filter per thread.

But doing a copy seems like a strange instantiation strategy.  The
"global" filter isn't ever used, right?  It only serves to make copies?
I think a more common pattern would be for the user to register a
factory function (really, ny callable that fits the bill) which would be
called whenever we need to create an instance.

>>> +{
>>> +  std::string result;
>>> +
>>> +  if ((PyObject *) ptw_filter == nullptr)
>>> +    error (_("No valid ptwrite filter."));
>>
>> I think this can only happen if get_ptwrite_filter has failed, in
>> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
>> written as:
>>
>>     btinfo->ptw_context = get_ptwrite_filter ();
>>     if (btinfo->ptw_context != nullptr)
>>       btinfo->ptw_callback_fun = &recpy_call_filter;
>>
>> ... such that recpy_call_filter is only installed when there is
>> something in ptw_context.  recpy_call_filter could then do:
>>
>>   gdb_assert (ptw_filter != nullptr);
> 
> I guess I could do that. Yet we check ptw_callback_fun before calling it
> in btrace.c. See patch 10. So that alone would actually hide problems to the user.
> And just not print any ptwrite string silently. Even if the user gave a custom
> listener function. So we would also need to remove that check.
> 
> I am also wondering what the advantage you see is.
> That we have an assert instead of an error? I guess I could do that even
> without the change in gdbpy_load_ptwrite_filter.

Just trying to choose the right tool for the job, which in turns helps
build the right mental model.  error() calls are for errors that aren't
GDB's fault, errors that are not the result of a bug in GDB.  gdb_assert
is to catch things programming errors in GDB.

If I see:

  gdb_assert (foo == nullptr);

I conclude that there should be not situation where foo is nullptr.  If
I see:

  if (foo -- nullptr)
    error (...)

Then I conclude there are some valid situations where foo is nullptr
(and then we have to worry about how they are handled).

>>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
>>> index 258f5c42537..c21232c07dc 100644
>>> --- a/gdb/python/python-internal.h
>>> +++ b/gdb/python/python-internal.h
>>> @@ -376,6 +376,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);
>>
>> Can this declaration be in py-record-btrace.h?  I really don't see why
>> all the declarations are stuffed into python.h.  Also, please move the
>> comment to the declaration, and put the appropriate /* See ... */
>> comment on the definition.
> 
> I don't think so. I modelled this after the other extension language interfaces.
> These are also all in here, without any comment. This function is being called
> via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
> As far as I understand it, that is how you interact with the python layer.
> To also allow the same things to be done in guile, if someone feels like it.
> Please tell me if I misunderstood the architecture.

I think you're right that everything goes through python/python.h.  I
don't know why though, we we can't have functions declared in various .h
files.  Perhaps it's easier to manage for when building with Python
disabled, I don't know.  In any case, not really important right now,
sorry for bringing that up.

Simon

RE: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Montag, 3. April 2023 22:45
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> >>> +def ptwrite_exit_handler(event):
> >>> +    """Exit handler to prune _ptwrite_filter on inferior exit."""
> >>> +    for key in list(_ptwrite_filter.keys()):
> >>
> >> I don't think the list() is needed.
> >
> > It actually is needed, as I re-size the array:
> >
> >>>> mydict = {1: "one", 2: "two"}
> >>>> for k in mydict.keys():
> > ...     if k == 2:
> > ...         del mydict[k]
> > ...
> > Traceback (most recent call last):
> >   File "<stdin>", line 1, in <module>
> > RuntimeError: dictionary changed size during iteration
> Ah ok, it's to take a "snapshot" of the keys.
> 
> >> I guess that answers my question on the previous patch about the use
> >> case of that clear method.  If that's the only goal of the clear method,
> >> and if there are no use case for the user to call it, I think it would
> >> be better to have an internal function instead.
> >>
> >> Can you explain why its needed to switch threads and call clear on the
> >> same recording object, but once with each thread selected?  I thought
> >> the recording would be per-inferior, and one call to clear would clear
> >> the data for all threads of that inferior.
> >
> > The recordings are per thread not per Inferior. Though I don't think you
> > can actually record only one thread of an inferior, as GDB will always
> > start a recording for all threads, or start recording a new thread if the
> > existing thread is already recording.
> >
> > We want to allow internal state of a filter function to be maintained
> > per thread. Hence the copy and clearing for all threads of the inferior.
> > I implemented the clear() patch based on feedback from Markus.
> > Note that there is already "maintenance btrace clear" in CLI.
> >
> > That said, you are right, I shouldn't clear the same object. In the first
> > version of this series I still had it right, but changed it for V2.
> > I can't exactly remember why.
> >
> > This still leaves the problem of multiple inferiors, that you also
> > commented on below.
> > I think our actual idea was to have this globally, for all inferiors.
> > But that means I would need to clear all traces for all inferiors above.
> > Not just all threads of the current inferior.
> > That can easily be done, unless our other discussions go into a different
> > direction, and we make this per-inferior.
> >
> > So I would change this to something like this:
> >
> > def _clear_traces():
> >     """Helper function to clear the trace of all threads."""
> >     current_thread = gdb.selected_thread()
> >
> >     for inferior in gdb.inferiors():
> >         for thread in inferior.threads():
> >             thread.switch()
> >             recording = gdb.current_recording()
> >             if recording is not None:
> >                 recording.clear()
> >
> >     current_thread.switch()
> >
> > Not sure if it is worth exiting early per inferior. I couldn't find
> > a scenario where you can selectively record (or not record)
> > one thread of a multi-threaded inferior.
> 
> For our internal Python helper code, it's fine to do things based on
> some internal knowledge of how GDB is implemented.  So, if we know what
> doing "clear" on one thread clears the recording for the whole inferior,
> then we can do just one thread per inferior.  If GDB changes, we can
> adjust that code at the same time.
> 
> But seeing this, I think that from a Python user point of view, the
> clear method you are proposing is a bit confusing.  It is called on a
> Record object, which is per-thread, but it clears the recording for all
> threads of the inferior.  This would be unexpected, unless documented as
> such in the doc (but even then, it feels a bit awkward).
> 
> The doc talks about "the current recording" here and there, but it's
> unclear what the scope of the recording is.

I agree on the doc part. It would indeed be nice to write this down.

Though, I think you misunderstood some things (or maybe I did).
Gdb.current_recording().clear(), as added in patch 5, does clear on a 
per-thread basis. Not on a per inferior basis as you write.
But maybe you meant _clear_traces() here? This is supposed to be
an internal function, as the leading underscore suggests. It indeed
does clear all recordings for all threads. As that is needed for ptwrite,
which is the only intended use case of this internal function.

So I don't quite see what would be unexpected.

> >>> +
> >>> +        current_thread.switch()
> >>> +
> >>> +
> >>> +def register_filter(filter_):
> >>> +    """Register the ptwrite filter function."""
> >>> +    if filter_ is not None and not callable(filter_):
> >>> +        raise TypeError("The filter must be callable or 'None'.")
> >>> +
> >>> +    # Clear the traces of all threads to force re-decoding with
> >>> +    # the new filter.
> >>> +    thread_list = gdb.selected_inferior().threads()
> >>> +    _clear_traces(thread_list)
> >>
> >> Could we avoid relying on the selected inferior?  For instance, it could
> >> be a method in Inferior:
> >>
> >>   inf.register_ptwrite_filter(f)
> >>
> >> or pass an inferior to this function:
> >>
> >>   ptwrite.register_filter(inf, f)
> >>
> >> I don't like having to switch inferior all the time in GDB to do
> >> per-inferior stuff, I don't want to impose that to our Python users :).
> >
> > I get where you are coming from. Though one could argue that this is
> already
> > the case today. Starting recordings is already per inferior.
> > If we have two inferiors and start recording one we don't automatically
> > record the second one. And if we have one inferior that is being recorded,
> > and add a second, the second also won't be recorded. You also don't have
> > any option, apart from switching inferiors and starting the recording
> manually.
> 
> I think it's a flaw in the current API that you need to switch current
> inferior to start recording on a specific inferior.  And also, it's a
> flaw in the documentation that it doesn't state that the recording is
> started for the current inferior.  The user is left to guess (or must
> experiment, or read the GDB source code, or have bad surprises) about
> what happens when you have multiple inferiors.

I agree.

> But if I understood you clearly, register_filter is really meant to
> install a global filter (valid for all inferiors, current and future).
> And the call to _clear_traces is meant to clear all traces of all
> inferiors.  In that case, it's fine for register_ptwrite_filter to be
> global.

That is right.

> > For threads that is different. Spawning threads will also be recorded
> > automatically. And if you have two threads and start recording, both
> > will be recorded.
> >
> > What you suggest is definitely possible, years ago I had a version that
> > would allow registering different filters per thread, but we decided against
> > that.
> 
> I don't think that is what I suggested.  If your experience suggests
> that having just one global filter is enough, I can't argue with that.
> Plus, it would probably be possible to implement per-inferior or
> per-thread filters in the future if needed, that would take precedence
> on the global filter.
> 
> My point is just that an API that requires the user to switch context,
> then make an API call, then restore the original context, is bad.  It's
> unclear and error prone.  I know we have some of that already, but I
> would like to avoid adding more.

I again see your points. Above you wrote:

Simon> In that case, it's fine for register_ptwrite_filter to be
Simon> global.

So I won't change anything here now. Extending it in the future should be
simple, I agree.

> >>> +
> >>> +
> >>> +def get_filter():
> >>> +    """Returns the filters of the current thread."""
> >>> +    # The key is of this format to enable an per-inferior cleanup when an
> >>> +    # inferior exits.
> >>> +    key =
> f"{gdb.selected_inferior().pid}.{gdb.selected_thread().ptid[1]}"
> >>
> >> I don't think we can use f-strings, we claim to support back to Python
> >> 3.4.
> >
> > Did we write down what we support clearly? The gdb/README actually
> says 3.2.
> 
> Sorry, I said 3.4 because I remembered sending a patch that raised the
> minimum level to 3.4.  But that patch was rejected, so it's plausible we
> still support 3.2:
> 
>   https://inbox.sourceware.org/gdb-patches/20220107152921.2858909-3-
> simon.marchi@polymtl.ca/#t
> 
> > And I think it might be good to mention this here:
> > https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-
> Standards
> 
> Feel free to edit that page.  However, the risk of having it documented
> at multiple places is that we can easily forget to update one spot.
> Maybe put a link to the README, and say "make sure your changes are
> compatible with the minimum Python version mentioned in the README"?

Good point, yes a single source of truth is much better.
I am no longer thinking it is a must and I don't think I can edit it anyway.

> > It seems like there are already some f strings in the master branch:
> >
> > ./lib/gdb/dap/io.py:63:            header = f"Content-Length:
> {len(body_bytes)}\r\n\r\n"
> > ./lib/gdb/dap/launch.py:29:        send_gdb(f"file {_program}")
> > ./lib/gdb/dap/state.py:25:        exec_and_log(f"thread {thread_id}")
> >
> > I just did a quick grep and didn't look at all occasions.
> >
> > That said, I will remove f strings.
> 
> Probably erroneous then.  Since this is specifically in DAP code, would
> someone using Python 3.4 hit an error when _not_ using DAP?  I would
> guess not, probably why this wasn't noticed.  Or, nobody tests with
> older Python versions...
> 
> >> You could perhaps also pass the key or the pid + lwp from the C++ code
> >> (taken from inferior_ptid).  Calling selected_inferior twice is just
> >> more churn to get the same info.
> >
> > This isn't supposed to be limited to be called from C++ code.
> 
> Ok, I didn't consider that.
> 
> One thing to note is that there isn't always a selected thread, so you
> might want to handle that appropriately.
> 
> > And I think having it without any arguments is a bit easier to use from
> python.
> > I changed it according to your prior suggestions and made it a tuple.
> > But I guess this also depends on our per-inferior discussion above.
> 
> Indeed.  I had not noticed previously, but what I said earlier is true
> for get_filter as well.  I would much rather see get_filter take a
> thread as argument, than relying on the selected thread.

Okay, I will add this. Probably as an optional argument, as I think
that makes it much easier for single threaded users.

> >
> >>> +
> >>> +    # Create a new filter for new threads.
> >>> +    if key not in _ptwrite_filter.keys():
> >>> +        _ptwrite_filter[key] = deepcopy(_ptwrite_filter["global"])
> >>
> >> Why is the deepcopy needed.  Because users could register a complex
> >> object with a __call__ method, in which case a simple copy would not do
> >> the same as deepcopy?  Just wondering if that's what users would expect.
> >
> > We wanted to maintain internal state per thread in an easy fashion.
> > As you already saw in Patch 10.
> 
> I understand that you want to have one copy of the filter per thread.
> 
> But doing a copy seems like a strange instantiation strategy.  The
> "global" filter isn't ever used, right?  It only serves to make copies?
> I think a more common pattern would be for the user to register a
> factory function (really, ny callable that fits the bill) which would be
> called whenever we need to create an instance.

Mhm, I think that would make this a bit too complex for users.
I don't think they want to care about providing a generator as well
that returns a callable. I don't see any new feature that
is being unlocked with that, just more tasks for the user.

And if it does at some point, the code should be straightforward to
extend in the future.

> >>> +{
> >>> +  std::string result;
> >>> +
> >>> +  if ((PyObject *) ptw_filter == nullptr)
> >>> +    error (_("No valid ptwrite filter."));
> >>
> >> I think this can only happen if get_ptwrite_filter has failed, in
> >> gdbpy_load_ptwrite_filter.  So perhaps gdbpy_load_ptwrite_filter can be
> >> written as:
> >>
> >>     btinfo->ptw_context = get_ptwrite_filter ();
> >>     if (btinfo->ptw_context != nullptr)
> >>       btinfo->ptw_callback_fun = &recpy_call_filter;
> >>
> >> ... such that recpy_call_filter is only installed when there is
> >> something in ptw_context.  recpy_call_filter could then do:
> >>
> >>   gdb_assert (ptw_filter != nullptr);
> >
> > I guess I could do that. Yet we check ptw_callback_fun before calling it
> > in btrace.c. See patch 10. So that alone would actually hide problems to the
> user.
> > And just not print any ptwrite string silently. Even if the user gave a custom
> > listener function. So we would also need to remove that check.
> >
> > I am also wondering what the advantage you see is.
> > That we have an assert instead of an error? I guess I could do that even
> > without the change in gdbpy_load_ptwrite_filter.
> 
> Just trying to choose the right tool for the job, which in turns helps
> build the right mental model.  error() calls are for errors that aren't
> GDB's fault, errors that are not the result of a bug in GDB.  gdb_assert
> is to catch things programming errors in GDB.
> 
> If I see:
> 
>   gdb_assert (foo == nullptr);
> 
> I conclude that there should be not situation where foo is nullptr.  If
> I see:
> 
>   if (foo -- nullptr)
>     error (...)
> 
> Then I conclude there are some valid situations where foo is nullptr
> (and then we have to worry about how they are handled).

I will change the error in recpy_call_filter to an assert.
I still don't see the point of not registering ptw_callback_fun().
I fear that will just hide things.
Anyway, I will look at this again when implementing the printing in C++
that you suggested for builds without Python. I think the picture
might be clearer once I have that.

> >>> diff --git a/gdb/python/python-internal.h b/gdb/python/python-
> internal.h
> >>> index 258f5c42537..c21232c07dc 100644
> >>> --- a/gdb/python/python-internal.h
> >>> +++ b/gdb/python/python-internal.h
> >>> @@ -376,6 +376,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);
> >>
> >> Can this declaration be in py-record-btrace.h?  I really don't see why
> >> all the declarations are stuffed into python.h.  Also, please move the
> >> comment to the declaration, and put the appropriate /* See ... */
> >> comment on the definition.
> >
> > I don't think so. I modelled this after the other extension language
> interfaces.
> > These are also all in here, without any comment. This function is being
> called
> > via apply_ext_lang_ptwrite_filter. Which is called from btrace.c.
> > As far as I understand it, that is how you interact with the python layer.
> > To also allow the same things to be done in guile, if someone feels like it.
> > Please tell me if I misunderstood the architecture.
> 
> I think you're right that everything goes through python/python.h.  I
> don't know why though, we we can't have functions declared in various .h
> files.  Perhaps it's easier to manage for when building with Python
> disabled, I don't know.  In any case, not really important right now,
> sorry for bringing that up.

No worries, I appreciate the review!

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

Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Simon Marchi]

> I agree on the doc part. It would indeed be nice to write this down.
> 
> Though, I think you misunderstood some things (or maybe I did).
> Gdb.current_recording().clear(), as added in patch 5, does clear on a 
> per-thread basis. Not on a per inferior basis as you write.
> But maybe you meant _clear_traces() here? This is supposed to be
> an internal function, as the leading underscore suggests. It indeed
> does clear all recordings for all threads. As that is needed for ptwrite,
> which is the only intended use case of this internal function.
> 
> So I don't quite see what would be unexpected.
My misunderstanding comes from where you said:


> Not sure if it is worth exiting early per inferior. I couldn't find
> a scenario where you can selectively record (or not record)
> one thread of a multi-threaded inferior.

By "exiting early", I thought you meant that it would be enough to clear
the data for one thread in each inferior.

>> I understand that you want to have one copy of the filter per thread.
>>
>> But doing a copy seems like a strange instantiation strategy.  The
>> "global" filter isn't ever used, right?  It only serves to make copies?
>> I think a more common pattern would be for the user to register a
>> factory function (really, ny callable that fits the bill) which would be
>> called whenever we need to create an instance.
> 
> Mhm, I think that would make this a bit too complex for users.
> I don't think they want to care about providing a generator as well
> that returns a callable. I don't see any new feature that
> is being unlocked with that, just more tasks for the user.

I don't think it adds significant complexity, this is pretty typical
Python.  It is similar to what we have for the pretty printers.  You
register a function that gets called to instantiate the right pretty
printer.

At the simplest, it would be:

  class MyFilter:
    ...

  def make_one_filter():
    return MyFilter()

  register_filter(make_one_filter)

or even:

  class MyFilter:
    ...

  register_filter(lambda: MyFilter())

An advantage is that it makes it easy to return a different filter per
thread, if needed.  Since filters are on a per-thread basis, I think it
would even make sense to pass the thread to the factory function, in
case the user wants to select a filter based on that.

Not that the fact that filters are callable themselves is just a design
choice you made, you could very well decide to call something else than
__call__ on those objects.  If you want to make things more explicit,
you can make it so filters have to implement a "get_aux_data" method,
instead of __call__.  The advantage of using __call__ is just that it
allows freely passing functions as well as objects implementing
__call__, so that gives the user a bit more flexibility.

Simon

RE: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Dienstag, 4. April 2023 17:06
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> > I agree on the doc part. It would indeed be nice to write this down.
> >
> > Though, I think you misunderstood some things (or maybe I did).
> > Gdb.current_recording().clear(), as added in patch 5, does clear on a
> > per-thread basis. Not on a per inferior basis as you write.
> > But maybe you meant _clear_traces() here? This is supposed to be
> > an internal function, as the leading underscore suggests. It indeed
> > does clear all recordings for all threads. As that is needed for ptwrite,
> > which is the only intended use case of this internal function.
> >
> > So I don't quite see what would be unexpected.
> My misunderstanding comes from where you said:
> 
> 
> > Not sure if it is worth exiting early per inferior. I couldn't find
> > a scenario where you can selectively record (or not record)
> > one thread of a multi-threaded inferior.
> 
> By "exiting early", I thought you meant that it would be enough to clear
> the data for one thread in each inferior.
> 
> >> I understand that you want to have one copy of the filter per thread.
> >>
> >> But doing a copy seems like a strange instantiation strategy.  The
> >> "global" filter isn't ever used, right?  It only serves to make copies?
> >> I think a more common pattern would be for the user to register a
> >> factory function (really, ny callable that fits the bill) which would be
> >> called whenever we need to create an instance.
> >
> > Mhm, I think that would make this a bit too complex for users.
> > I don't think they want to care about providing a generator as well
> > that returns a callable. I don't see any new feature that
> > is being unlocked with that, just more tasks for the user.
> 
> I don't think it adds significant complexity, this is pretty typical
> Python.  It is similar to what we have for the pretty printers.  You
> register a function that gets called to instantiate the right pretty
> printer.
> 
> At the simplest, it would be:
> 
>   class MyFilter:
>     ...
> 
>   def make_one_filter():
>     return MyFilter()
> 
>   register_filter(make_one_filter)
> 
> or even:
> 
>   class MyFilter:
>     ...
> 
>   register_filter(lambda: MyFilter())
> 
> An advantage is that it makes it easy to return a different filter per
> thread, if needed.  Since filters are on a per-thread basis, I think it
> would even make sense to pass the thread to the factory function, in
> case the user wants to select a filter based on that.
> 
> Not that the fact that filters are callable themselves is just a design
> choice you made, you could very well decide to call something else than
> __call__ on those objects.  If you want to make things more explicit,
> you can make it so filters have to implement a "get_aux_data" method,
> instead of __call__.  The advantage of using __call__ is just that it
> allows freely passing functions as well as objects implementing
> __call__, so that gives the user a bit more flexibility.
> 
> Simon

Sorry, I still don't understand this or see the advantage of the
factory pattern here.
And I only want to use a pattern if there is a clear advantage to it.

I assume you want to move the "housekeeping" to the user side, right?
Why do we need a factory pattern for that?
Can't you achieve the same if we just have one callable filter object
instead of one per thread? And have the user do it in that object, if
he needs to? (Per thread filters would still allow the same, though not
as clean, as they would have to call the singular object.)
Passing the thread can also be done with the filter function directly.
And is just a convenience, as the GDB API is available to the user.
(I am not saying, it is a bad convenience feature.)

AFAIK, you use a factory pattern when you want to abstract what
type of exact object is returned. I don't see that we need that.
And we already have a bit of freedom anyway, with allowing
callables.

If you don't like the per thread copy, we could still just not do that.
And move the per-thread or per-inferior housekeeping to the user.
I had this at some point. The documentation I wrote still gives a short
example for how to do something for one thread but not for another.

As far as I remember, Markus wanted per-thread state in an easier way
though.

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

Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Simon Marchi]

On 4/5/23 06:20, Willgerodt, Felix wrote:
> 
> 
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Dienstag, 4. April 2023 17:06
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
>> registration.
>>
>>> I agree on the doc part. It would indeed be nice to write this down.
>>>
>>> Though, I think you misunderstood some things (or maybe I did).
>>> Gdb.current_recording().clear(), as added in patch 5, does clear on a
>>> per-thread basis. Not on a per inferior basis as you write.
>>> But maybe you meant _clear_traces() here? This is supposed to be
>>> an internal function, as the leading underscore suggests. It indeed
>>> does clear all recordings for all threads. As that is needed for ptwrite,
>>> which is the only intended use case of this internal function.
>>>
>>> So I don't quite see what would be unexpected.
>> My misunderstanding comes from where you said:
>>
>>
>>> Not sure if it is worth exiting early per inferior. I couldn't find
>>> a scenario where you can selectively record (or not record)
>>> one thread of a multi-threaded inferior.
>>
>> By "exiting early", I thought you meant that it would be enough to clear
>> the data for one thread in each inferior.
>>
>>>> I understand that you want to have one copy of the filter per thread.
>>>>
>>>> But doing a copy seems like a strange instantiation strategy.  The
>>>> "global" filter isn't ever used, right?  It only serves to make copies?
>>>> I think a more common pattern would be for the user to register a
>>>> factory function (really, ny callable that fits the bill) which would be
>>>> called whenever we need to create an instance.
>>>
>>> Mhm, I think that would make this a bit too complex for users.
>>> I don't think they want to care about providing a generator as well
>>> that returns a callable. I don't see any new feature that
>>> is being unlocked with that, just more tasks for the user.
>>
>> I don't think it adds significant complexity, this is pretty typical
>> Python.  It is similar to what we have for the pretty printers.  You
>> register a function that gets called to instantiate the right pretty
>> printer.
>>
>> At the simplest, it would be:
>>
>>   class MyFilter:
>>     ...
>>
>>   def make_one_filter():
>>     return MyFilter()
>>
>>   register_filter(make_one_filter)
>>
>> or even:
>>
>>   class MyFilter:
>>     ...
>>
>>   register_filter(lambda: MyFilter())
>>
>> An advantage is that it makes it easy to return a different filter per
>> thread, if needed.  Since filters are on a per-thread basis, I think it
>> would even make sense to pass the thread to the factory function, in
>> case the user wants to select a filter based on that.
>>
>> Not that the fact that filters are callable themselves is just a design
>> choice you made, you could very well decide to call something else than
>> __call__ on those objects.  If you want to make things more explicit,
>> you can make it so filters have to implement a "get_aux_data" method,
>> instead of __call__.  The advantage of using __call__ is just that it
>> allows freely passing functions as well as objects implementing
>> __call__, so that gives the user a bit more flexibility.
>>
>> Simon
> 
> Sorry, I still don't understand this or see the advantage of the
> factory pattern here.
> And I only want to use a pattern if there is a clear advantage to it.
> 
> I assume you want to move the "housekeeping" to the user side, right?

Not sure what you mean by "housekeeping".  In the end, with what I
propose, we still end up with one filter object per thread, it just
changes how they are instantiated.  Instead of copying a "template"
object, we call the "get me a new instance" function.

> Why do we need a factory pattern for that?
> Can't you achieve the same if we just have one callable filter object
> instead of one per thread? And have the user do it in that object, if
> he needs to? (Per thread filters would still allow the same, though not
> as clean, as they would have to call the singular object.)
> Passing the thread can also be done with the filter function directly.
> And is just a convenience, as the GDB API is available to the user.
> (I am not saying, it is a bad convenience feature.)
> 
> AFAIK, you use a factory pattern when you want to abstract what
> type of exact object is returned. I don't see that we need that.
> And we already have a bit of freedom anyway, with allowing
> callables.
>
> If you don't like the per thread copy, we could still just not do that.

With what I propose, there would still be one copy (instance) of the
filter per thread.  So maybe we're not on the same page.

> And move the per-thread or per-inferior housekeeping to the user.
> I had this at some point. The documentation I wrote still gives a short
> example for how to do something for one thread but not for another.
> 
> As far as I remember, Markus wanted per-thread state in an easier way
> though.

Simon

RE: [PATCH v8 09/10] btrace, python: Enable ptwrite filter registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Mittwoch, 5. April 2023 22:27
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>
> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> registration.
> 
> 
> 
> On 4/5/23 06:20, Willgerodt, Felix wrote:
> >
> >
> >> -----Original Message-----
> >> From: Simon Marchi <simark@simark.ca>
> >> Sent: Dienstag, 4. April 2023 17:06
> >> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> >> patches@sourceware.org
> >> Subject: Re: [PATCH v8 09/10] btrace, python: Enable ptwrite filter
> >> registration.
> >>
> >>> I agree on the doc part. It would indeed be nice to write this down.
> >>>
> >>> Though, I think you misunderstood some things (or maybe I did).
> >>> Gdb.current_recording().clear(), as added in patch 5, does clear on a
> >>> per-thread basis. Not on a per inferior basis as you write.
> >>> But maybe you meant _clear_traces() here? This is supposed to be
> >>> an internal function, as the leading underscore suggests. It indeed
> >>> does clear all recordings for all threads. As that is needed for ptwrite,
> >>> which is the only intended use case of this internal function.
> >>>
> >>> So I don't quite see what would be unexpected.
> >> My misunderstanding comes from where you said:
> >>
> >>
> >>> Not sure if it is worth exiting early per inferior. I couldn't find
> >>> a scenario where you can selectively record (or not record)
> >>> one thread of a multi-threaded inferior.
> >>
> >> By "exiting early", I thought you meant that it would be enough to clear
> >> the data for one thread in each inferior.
> >>
> >>>> I understand that you want to have one copy of the filter per thread.
> >>>>
> >>>> But doing a copy seems like a strange instantiation strategy.  The
> >>>> "global" filter isn't ever used, right?  It only serves to make copies?
> >>>> I think a more common pattern would be for the user to register a
> >>>> factory function (really, ny callable that fits the bill) which would be
> >>>> called whenever we need to create an instance.
> >>>
> >>> Mhm, I think that would make this a bit too complex for users.
> >>> I don't think they want to care about providing a generator as well
> >>> that returns a callable. I don't see any new feature that
> >>> is being unlocked with that, just more tasks for the user.
> >>
> >> I don't think it adds significant complexity, this is pretty typical
> >> Python.  It is similar to what we have for the pretty printers.  You
> >> register a function that gets called to instantiate the right pretty
> >> printer.
> >>
> >> At the simplest, it would be:
> >>
> >>   class MyFilter:
> >>     ...
> >>
> >>   def make_one_filter():
> >>     return MyFilter()
> >>
> >>   register_filter(make_one_filter)
> >>
> >> or even:
> >>
> >>   class MyFilter:
> >>     ...
> >>
> >>   register_filter(lambda: MyFilter())
> >>
> >> An advantage is that it makes it easy to return a different filter per
> >> thread, if needed.  Since filters are on a per-thread basis, I think it
> >> would even make sense to pass the thread to the factory function, in
> >> case the user wants to select a filter based on that.
> >>
> >> Not that the fact that filters are callable themselves is just a design
> >> choice you made, you could very well decide to call something else than
> >> __call__ on those objects.  If you want to make things more explicit,
> >> you can make it so filters have to implement a "get_aux_data" method,
> >> instead of __call__.  The advantage of using __call__ is just that it
> >> allows freely passing functions as well as objects implementing
> >> __call__, so that gives the user a bit more flexibility.
> >>
> >> Simon
> >
> > Sorry, I still don't understand this or see the advantage of the
> > factory pattern here.
> > And I only want to use a pattern if there is a clear advantage to it.
> >
> > I assume you want to move the "housekeeping" to the user side, right?
> 
> Not sure what you mean by "housekeeping".  In the end, with what I
> propose, we still end up with one filter object per thread, it just
> changes how they are instantiated.  Instead of copying a "template"
> object, we call the "get me a new instance" function.
> 
> > Why do we need a factory pattern for that?
> > Can't you achieve the same if we just have one callable filter object
> > instead of one per thread? And have the user do it in that object, if
> > he needs to? (Per thread filters would still allow the same, though not
> > as clean, as they would have to call the singular object.)
> > Passing the thread can also be done with the filter function directly.
> > And is just a convenience, as the GDB API is available to the user.
> > (I am not saying, it is a bad convenience feature.)
> >
> > AFAIK, you use a factory pattern when you want to abstract what
> > type of exact object is returned. I don't see that we need that.
> > And we already have a bit of freedom anyway, with allowing
> > callables.
> >
> > If you don't like the per thread copy, we could still just not do that.
> 
> With what I propose, there would still be one copy (instance) of the
> filter per thread.  So maybe we're not on the same page.
> 
> > And move the per-thread or per-inferior housekeeping to the user.
> > I had this at some point. The documentation I wrote still gives a short
> > example for how to do something for one thread but not for another.
> >
> > As far as I remember, Markus wanted per-thread state in an easier way
> > though.
> 
> Simon

Ah okay, we were indeed not on the same page. Sorry for that.
So it is just about getting rid of the deepcopy mechanism and
the extra copy.

Okay, I will look into this a bit more for the next version.

Thanks a lot!
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

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

[Felix Willgerodt]

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                                  |   7 +
 gdb/btrace.c                              |  54 +++
 gdb/config.in                             |   3 +
 gdb/configure                             |  11 +
 gdb/doc/python.texi                       | 150 ++++++
 gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 550 ++++++++++++++++++++++
 gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
 gdb/testsuite/gdb.btrace/ptwrite.exp      | 200 ++++++++
 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 544 +++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                 |  72 +++
 gdbsupport/common.m4                      |   2 +
 gdbsupport/config.in                      |   3 +
 gdbsupport/configure                      |  11 +
 13 files changed, 1646 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 cc262f1f8a6..5dd05867f2a 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -106,6 +106,13 @@ show always-read-ctf
 
 *** Changes in GDB 13
 
+* GDB now supports printing of ptwrite payloads from the Intel Processor
+  Trace during 'record instruction-history', 'record function-call-history'
+  and all stepping commands.  The payload is also accessible in Python as a
+  RecordAuxiliary object.  Printing is customizable via a ptwrite filter
+  function in Python.  By default, the raw ptwrite payload is printed for
+  each ptwrite that is encountered.
+
 * MI version 1 is deprecated, and will be removed in GDB 14.
 
 * GDB now supports dumping memory tag data for AArch64 MTE.  It also supports
diff --git a/gdb/btrace.c b/gdb/btrace.c
index 37dd0b666d8..db0d0e291d9 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1253,6 +1253,54 @@ 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_context);
+
+	    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;
+	    ptw_insn.aux_data_index = btinfo->aux_data.size () - 1;
+	    ptw_insn.size = 0;
+	    ptw_insn.iclass = BTRACE_INSN_AUX;
+	    ptw_insn.flags = flags;
+
+	    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_CONTAINS_AUX;
+	    ftrace_update_insns (bfun, ptw_insn);
+
+	    break;
+	  }
+#endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
 	}
     }
 #endif /* defined (HAVE_PT_INSN_EVENT) */
@@ -2981,6 +3029,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 a7da88b92d7..9e6e59d73c3 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 5bb2a0795e5..778e69f74b9 100755
--- a/gdb/configure
+++ b/gdb/configure
@@ -25770,6 +25770,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 e1ab3b20eda..e0772a7242e 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -7145,6 +7145,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
@@ -7335,3 +7336,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
+@code{PTWRITE} auxiliary information.  A custom Python function can be
+registered via @code{gdb.ptwrite.register_filter} as the @code{PTWRITE}
+filter function.  This function will be called with the @code{PTWRITE}
+payload and PC as arguments during trace decoding.
+
+@findex gdb.ptwrite.register_filter
+@defun register_filter (@var{filter})
+Used to register the @code{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 ()
+Return the currently active @code{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..ae6372ab991
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/i386-ptwrite.S
@@ -0,0 +1,550 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.
+
+
+   This file has been generated using gcc version 10.3.1 20210422
+   (Red Hat 10.3.1-1):
+   gcc -S -dA -g -m32 -mptwrite ptwrite.c -o i386-ptwrite.S.  */
+
+
+	.file	"ptwrite.c"
+	.text
+.Ltext0:
+	.globl	ptwrite1
+	.type	ptwrite1, @function
+ptwrite1:
+.LFB4021:
+	.file 1 "ptwrite.c"
+	# ptwrite.c:22:1
+	.loc 1 22 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
+	subl	$16, %esp
+	# ptwrite.c:23:3
+	.loc 1 23 3
+	movl	8(%ebp), %eax
+	movl	%eax, -4(%ebp)
+.LBB6:
+.LBB7:
+	.file 2 "/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h"
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:307:3
+	.loc 2 307 3
+	movl	-4(%ebp), %eax
+	ptwrite	%eax
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:308:1
+	.loc 2 308 1
+	nop
+.LBE7:
+.LBE6:
+	# ptwrite.c:24:1
+	.loc 1 24 1
+	nop
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE4021:
+	.size	ptwrite1, .-ptwrite1
+	.globl	ptwrite2
+	.type	ptwrite2, @function
+ptwrite2:
+.LFB4022:
+	# ptwrite.c:28:1
+	.loc 1 28 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
+	subl	$16, %esp
+	# ptwrite.c:29:3
+	.loc 1 29 3
+	movl	8(%ebp), %eax
+	movl	%eax, -4(%ebp)
+.LBB8:
+.LBB9:
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:307:3
+	.loc 2 307 3
+	movl	-4(%ebp), %eax
+	ptwrite	%eax
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:308:1
+	.loc 2 308 1
+	nop
+.LBE9:
+.LBE8:
+	# ptwrite.c:30:1
+	.loc 1 30 1
+	nop
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE4022:
+	.size	ptwrite2, .-ptwrite2
+	.globl	main
+	.type	main, @function
+main:
+.LFB4023:
+	# ptwrite.c:34:1
+	.loc 1 34 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:35:3
+	.loc 1 35 3
+	pushl	$66
+	call	ptwrite1
+	addl	$4, %esp
+	# ptwrite.c:36:3
+	.loc 1 36 3
+	pushl	$67
+	call	ptwrite2
+	addl	$4, %esp
+	# ptwrite.c:38:10
+	.loc 1 38 10
+	movl	$0, %eax
+	# ptwrite.c:39:1
+	.loc 1 39 1
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+# SUCC: EXIT [always]
+	ret
+	.cfi_endproc
+.LFE4023:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x129	# 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	.LASF15	# DW_AT_producer: "GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -m32 -mptwrite -mtune=generic -march=i686 -g"
+	.byte	0xc	# DW_AT_language
+	.long	.LASF16	# DW_AT_name: "ptwrite.c"
+	.long	.LASF17	# DW_AT_comp_dir: "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_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x3	# (DIE (0x2c) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF0	# DW_AT_name: "short int"
+	.uleb128 0x3	# (DIE (0x33) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF1	# DW_AT_name: "char"
+	.uleb128 0x3	# (DIE (0x3a) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF2	# DW_AT_name: "long long int"
+	.uleb128 0x3	# (DIE (0x41) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x4	# DW_AT_encoding
+	.long	.LASF3	# DW_AT_name: "float"
+	.uleb128 0x3	# (DIE (0x48) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF4	# DW_AT_name: "unsigned int"
+	.uleb128 0x3	# (DIE (0x4f) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF5	# DW_AT_name: "long int"
+	.uleb128 0x3	# (DIE (0x56) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x8	# DW_AT_encoding
+	.long	.LASF6	# DW_AT_name: "unsigned char"
+	.uleb128 0x3	# (DIE (0x5d) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF7	# DW_AT_name: "short unsigned int"
+	.uleb128 0x3	# (DIE (0x64) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF8	# DW_AT_name: "long unsigned int"
+	.uleb128 0x3	# (DIE (0x6b) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF9	# DW_AT_name: "signed char"
+	.uleb128 0x3	# (DIE (0x72) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF10	# DW_AT_name: "long long unsigned int"
+	.uleb128 0x3	# (DIE (0x79) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x4	# DW_AT_encoding
+	.long	.LASF11	# DW_AT_name: "double"
+	.uleb128 0x4	# (DIE (0x80) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF18	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x21	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	0x25	# DW_AT_type
+	.long	.LFB4023	# DW_AT_low_pc
+	.long	.LFE4023-.LFB4023	# 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 0x5	# (DIE (0x96) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF12	# DW_AT_name: "ptwrite2"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	.LFB4022	# DW_AT_low_pc
+	.long	.LFE4022-.LFB4022	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0xd5	# DW_AT_sibling
+	.uleb128 0x6	# (DIE (0xac) DW_TAG_formal_parameter)
+	.long	.LASF14	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0xf	# DW_AT_decl_column
+	.long	0x25	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x7	# (DIE (0xbb) DW_TAG_inlined_subroutine)
+	.long	0x114	# DW_AT_abstract_origin
+	.long	.LBB8	# DW_AT_low_pc
+	.long	.LBE8-.LBB8	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_call_file (ptwrite.c)
+	.byte	0x1d	# DW_AT_call_line
+	.byte	0x3	# DW_AT_call_column
+	.uleb128 0x8	# (DIE (0xcb) DW_TAG_formal_parameter)
+	.long	0x11e	# DW_AT_abstract_origin
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -12
+	.byte	0	# end of children of DIE 0xbb
+	.byte	0	# end of children of DIE 0x96
+	.uleb128 0x5	# (DIE (0xd5) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF13	# DW_AT_name: "ptwrite1"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	.LFB4021	# DW_AT_low_pc
+	.long	.LFE4021-.LFB4021	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0x114	# DW_AT_sibling
+	.uleb128 0x6	# (DIE (0xeb) DW_TAG_formal_parameter)
+	.long	.LASF14	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0xf	# DW_AT_decl_column
+	.long	0x25	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x7	# (DIE (0xfa) DW_TAG_inlined_subroutine)
+	.long	0x114	# DW_AT_abstract_origin
+	.long	.LBB6	# DW_AT_low_pc
+	.long	.LBE6-.LBB6	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_call_file (ptwrite.c)
+	.byte	0x17	# DW_AT_call_line
+	.byte	0x3	# DW_AT_call_column
+	.uleb128 0x8	# (DIE (0x10a) DW_TAG_formal_parameter)
+	.long	0x11e	# DW_AT_abstract_origin
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -12
+	.byte	0	# end of children of DIE 0xfa
+	.byte	0	# end of children of DIE 0xd5
+	.uleb128 0x9	# (DIE (0x114) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF19	# DW_AT_name: "_ptwrite32"
+	.byte	0x2	# DW_AT_decl_file (/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h)
+	.value	0x131	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.byte	0x3	# DW_AT_inline
+			# DW_AT_artificial
+	.uleb128 0xa	# (DIE (0x11e) DW_TAG_formal_parameter)
+	.ascii "__B\0"	# DW_AT_name
+	.byte	0x2	# DW_AT_decl_file (/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h)
+	.value	0x131	# DW_AT_decl_line
+	.byte	0x16	# DW_AT_decl_column
+	.long	0x48	# DW_AT_type
+	.byte	0	# end of children of DIE 0x114
+	.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 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 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 0xe	# (DW_FORM_strp)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (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 0x5	# (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 0x6	# (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 0x7	# (abbrev code)
+	.uleb128 0x1d	# (TAG: DW_TAG_inlined_subroutine)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x31	# (DW_AT_abstract_origin)
+	.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 0x58	# (DW_AT_call_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x59	# (DW_AT_call_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x57	# (DW_AT_call_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.byte	0
+	.byte	0
+	.uleb128 0x8	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x31	# (DW_AT_abstract_origin)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.byte	0
+	.byte	0
+	.uleb128 0x9	# (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 0x5	# (DW_FORM_data2)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x20	# (DW_AT_inline)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x34	# (DW_AT_artificial)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0xa	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0x5	# (DW_FORM_data2)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.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
+.LASF2:
+	.string	"long long int"
+.LASF14:
+	.string	"value"
+.LASF19:
+	.string	"_ptwrite32"
+.LASF4:
+	.string	"unsigned int"
+.LASF17:
+	.string	"gdb/gdb/testsuite/gdb.btrace"
+.LASF18:
+	.string	"main"
+.LASF16:
+	.string	"ptwrite.c"
+.LASF8:
+	.string	"long unsigned int"
+.LASF10:
+	.string	"long long unsigned int"
+.LASF13:
+	.string	"ptwrite1"
+.LASF12:
+	.string	"ptwrite2"
+.LASF6:
+	.string	"unsigned char"
+.LASF1:
+	.string	"char"
+.LASF5:
+	.string	"long int"
+.LASF15:
+	.string	"GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -m32 -mptwrite -mtune=generic -march=i686 -g"
+.LASF11:
+	.string	"double"
+.LASF7:
+	.string	"short unsigned int"
+.LASF9:
+	.string	"signed char"
+.LASF3:
+	.string	"float"
+.LASF0:
+	.string	"short int"
+	.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..c85b656919b
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/ptwrite.c
@@ -0,0 +1,39 @@
+/* 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/>.  */
+
+#include <immintrin.h>
+
+void
+ptwrite1 (int value)
+{
+  _ptwrite32 (value);
+}
+
+void
+ptwrite2 (int value)
+{
+  _ptwrite32 (value);
+}
+
+int
+main (void)
+{
+  ptwrite1 (0x42);
+  ptwrite2 (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..734b99ea0ea
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/ptwrite.exp
@@ -0,0 +1,200 @@
+# 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
+
+require allow_btrace_ptw_tests allow_python_tests
+
+set opts {}
+
+if [info exists COMPILE] {
+    # make check RUNTESTFLAGS="gdb.btrace/ptwrite.exp COMPILE=1"
+    standard_testfile ptwrite.c
+    lappend opts debug additional_flags=-mptwrite
+} 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
+}
+
+### 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" ".*" "next"
+gdb_test "next" ".*" "next 2"
+
+with_test_prefix "Default" {
+    # Test record instruction-history
+    gdb_test "record instruction-history 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[42\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite2\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[43\\\].*" \
+	]
+
+    gdb_test "record instruction-history /a 1" [multi_line \
+	".*\[0-9\]+\t   $hex <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	".*\[0-9\]+\t   $hex <ptwrite2\\+\[0-9\]+>:\tptwrite %\[a-z\]+.*" \
+	]
+
+    # Test function call history
+    gdb_test "record function-call-history 1,4" [multi_line \
+	"1\tmain" \
+	"2\tptwrite1" \
+	"\t\t\\\[42\\\]" \
+	"3\tmain" \
+	"4\tptwrite2" \
+	"\t\t\\\[43\\\]" \
+	]
+
+    gdb_test "record function-call-history /a 1,4" [multi_line \
+	"1\tmain" \
+	"2\tptwrite1" \
+	"3\tmain" \
+	"4\tptwrite2" \
+	]
+}
+
+# Test payload printing during stepping
+with_test_prefix "Stepping" {
+    gdb_test "record goto 10" "No such instruction\."
+    gdb_test "record goto 9" ".*ptwrite.* at .*"
+    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    -0x4\\\(%(e|r)bp\\\),%(e|r)ax" \
+	"ptwrite %eax" \
+	"42" \
+	"nop.*" \
+	"mov    -0x4\\\(%(e|r)bp\\\),%(e|r)ax" \
+	"ptwrite %eax" \
+	"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 <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[payload: 66, ip: $hex\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite2\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   $hex <ptwrite2\\+\[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 <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   $hex <ptwrite1\\+\[0-9\]+>:.*" \
+	"\[0-9\]+\t   $hex <ptwrite2\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   $hex <ptwrite2\\+\[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 <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[68\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite2\\+\[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 <ptwrite1\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+	"\[0-9\]+\t   \\\[1, 66\\\]" \
+	".*\[0-9\]+\t   $hex <ptwrite2\\+\[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..6fe64013399
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
@@ -0,0 +1,544 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 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/>.
+
+
+   This file has been generated using gcc version 10.3.1 20210422
+   (Red Hat 10.3.1-1):
+   gcc -S -dA -g -mptwrite ptwrite.c -o x86_64-ptwrite.S.  */
+
+	.file	"ptwrite.c"
+	.text
+.Ltext0:
+	.globl	ptwrite1
+	.type	ptwrite1, @function
+ptwrite1:
+.LFB4096:
+	.file 1 "ptwrite.c"
+	# ptwrite.c:22:1
+	.loc 1 22 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
+	movl	%edi, -20(%rbp)
+	# ptwrite.c:23:3
+	.loc 1 23 3
+	movl	-20(%rbp), %eax
+	movl	%eax, -4(%rbp)
+.LBB6:
+.LBB7:
+	.file 2 "/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h"
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:307:3
+	.loc 2 307 3
+	movl	-4(%rbp), %eax
+	ptwrite	%eax
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:308:1
+	.loc 2 308 1
+	nop
+.LBE7:
+.LBE6:
+	# ptwrite.c:24:1
+	.loc 1 24 1
+	nop
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE4096:
+	.size	ptwrite1, .-ptwrite1
+	.globl	ptwrite2
+	.type	ptwrite2, @function
+ptwrite2:
+.LFB4097:
+	# ptwrite.c:28:1
+	.loc 1 28 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
+	movl	%edi, -20(%rbp)
+	# ptwrite.c:29:3
+	.loc 1 29 3
+	movl	-20(%rbp), %eax
+	movl	%eax, -4(%rbp)
+.LBB8:
+.LBB9:
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:307:3
+	.loc 2 307 3
+	movl	-4(%rbp), %eax
+	ptwrite	%eax
+	# /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:308:1
+	.loc 2 308 1
+	nop
+.LBE9:
+.LBE8:
+	# ptwrite.c:30:1
+	.loc 1 30 1
+	nop
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE4097:
+	.size	ptwrite2, .-ptwrite2
+	.globl	main
+	.type	main, @function
+main:
+.LFB4098:
+	# ptwrite.c:34:1
+	.loc 1 34 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:35:3
+	.loc 1 35 3
+	movl	$66, %edi
+	call	ptwrite1
+	# ptwrite.c:36:3
+	.loc 1 36 3
+	movl	$67, %edi
+	call	ptwrite2
+	# ptwrite.c:38:10
+	.loc 1 38 10
+	movl	$0, %eax
+	# ptwrite.c:39:1
+	.loc 1 39 1
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE4098:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x159	# 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	.LASF15	# DW_AT_producer: "GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -mptwrite -mtune=generic -march=x86-64 -g"
+	.byte	0xc	# DW_AT_language
+	.long	.LASF16	# DW_AT_name: "ptwrite.c"
+	.long	.LASF17	# DW_AT_comp_dir: "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_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x3	# (DIE (0x34) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF0	# DW_AT_name: "short int"
+	.uleb128 0x3	# (DIE (0x3b) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF1	# DW_AT_name: "char"
+	.uleb128 0x3	# (DIE (0x42) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF2	# DW_AT_name: "long long int"
+	.uleb128 0x3	# (DIE (0x49) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x4	# DW_AT_encoding
+	.long	.LASF3	# DW_AT_name: "float"
+	.uleb128 0x3	# (DIE (0x50) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF4	# DW_AT_name: "long unsigned int"
+	.uleb128 0x3	# (DIE (0x57) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF5	# DW_AT_name: "long int"
+	.uleb128 0x3	# (DIE (0x5e) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x8	# DW_AT_encoding
+	.long	.LASF6	# DW_AT_name: "unsigned char"
+	.uleb128 0x3	# (DIE (0x65) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF7	# DW_AT_name: "short unsigned int"
+	.uleb128 0x3	# (DIE (0x6c) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF8	# DW_AT_name: "unsigned int"
+	.uleb128 0x3	# (DIE (0x73) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF9	# DW_AT_name: "signed char"
+	.uleb128 0x3	# (DIE (0x7a) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF10	# DW_AT_name: "long long unsigned int"
+	.uleb128 0x3	# (DIE (0x81) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x4	# DW_AT_encoding
+	.long	.LASF11	# DW_AT_name: "double"
+	.uleb128 0x4	# (DIE (0x88) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF18	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x21	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.long	0x2d	# DW_AT_type
+	.quad	.LFB4098	# DW_AT_low_pc
+	.quad	.LFE4098-.LFB4098	# 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 0x5	# (DIE (0xa6) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF12	# DW_AT_name: "ptwrite2"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB4097	# DW_AT_low_pc
+	.quad	.LFE4097-.LFB4097	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0xf5	# DW_AT_sibling
+	.uleb128 0x6	# (DIE (0xc4) DW_TAG_formal_parameter)
+	.long	.LASF14	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0xf	# DW_AT_decl_column
+	.long	0x2d	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -36
+	.uleb128 0x7	# (DIE (0xd3) DW_TAG_inlined_subroutine)
+	.long	0x144	# DW_AT_abstract_origin
+	.quad	.LBB8	# DW_AT_low_pc
+	.quad	.LBE8-.LBB8	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_call_file (ptwrite.c)
+	.byte	0x1d	# DW_AT_call_line
+	.byte	0x3	# DW_AT_call_column
+	.uleb128 0x8	# (DIE (0xeb) DW_TAG_formal_parameter)
+	.long	0x14e	# DW_AT_abstract_origin
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -20
+	.byte	0	# end of children of DIE 0xd3
+	.byte	0	# end of children of DIE 0xa6
+	.uleb128 0x5	# (DIE (0xf5) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF13	# DW_AT_name: "ptwrite1"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB4096	# DW_AT_low_pc
+	.quad	.LFE4096-.LFB4096	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0x144	# DW_AT_sibling
+	.uleb128 0x6	# (DIE (0x113) DW_TAG_formal_parameter)
+	.long	.LASF14	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0xf	# DW_AT_decl_column
+	.long	0x2d	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -36
+	.uleb128 0x7	# (DIE (0x122) DW_TAG_inlined_subroutine)
+	.long	0x144	# DW_AT_abstract_origin
+	.quad	.LBB6	# DW_AT_low_pc
+	.quad	.LBE6-.LBB6	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_call_file (ptwrite.c)
+	.byte	0x17	# DW_AT_call_line
+	.byte	0x3	# DW_AT_call_column
+	.uleb128 0x8	# (DIE (0x13a) DW_TAG_formal_parameter)
+	.long	0x14e	# DW_AT_abstract_origin
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -20
+	.byte	0	# end of children of DIE 0x122
+	.byte	0	# end of children of DIE 0xf5
+	.uleb128 0x9	# (DIE (0x144) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF19	# DW_AT_name: "_ptwrite32"
+	.byte	0x2	# DW_AT_decl_file (/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h)
+	.value	0x131	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.byte	0x3	# DW_AT_inline
+			# DW_AT_artificial
+	.uleb128 0xa	# (DIE (0x14e) DW_TAG_formal_parameter)
+	.ascii "__B\0"	# DW_AT_name
+	.byte	0x2	# DW_AT_decl_file (/usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h)
+	.value	0x131	# DW_AT_decl_line
+	.byte	0x16	# DW_AT_decl_column
+	.long	0x6c	# DW_AT_type
+	.byte	0	# end of children of DIE 0x144
+	.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 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 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 0xe	# (DW_FORM_strp)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (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 0x5	# (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 0x6	# (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 0x7	# (abbrev code)
+	.uleb128 0x1d	# (TAG: DW_TAG_inlined_subroutine)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x31	# (DW_AT_abstract_origin)
+	.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 0x58	# (DW_AT_call_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x59	# (DW_AT_call_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x57	# (DW_AT_call_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.byte	0
+	.byte	0
+	.uleb128 0x8	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x31	# (DW_AT_abstract_origin)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.byte	0
+	.byte	0
+	.uleb128 0x9	# (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 0x5	# (DW_FORM_data2)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x20	# (DW_AT_inline)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x34	# (DW_AT_artificial)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.uleb128 0xa	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0x5	# (DW_FORM_data2)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.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
+.LASF2:
+	.string	"long long int"
+.LASF14:
+	.string	"value"
+.LASF19:
+	.string	"_ptwrite32"
+.LASF8:
+	.string	"unsigned int"
+.LASF17:
+	.string	"gdb/gdb/testsuite/gdb.btrace"
+.LASF18:
+	.string	"main"
+.LASF16:
+	.string	"ptwrite.c"
+.LASF4:
+	.string	"long unsigned int"
+.LASF10:
+	.string	"long long unsigned int"
+.LASF15:
+	.string	"GNU C17 10.3.1 20210422 (Red Hat 10.3.1-1) -mptwrite -mtune=generic -march=x86-64 -g"
+.LASF13:
+	.string	"ptwrite1"
+.LASF12:
+	.string	"ptwrite2"
+.LASF6:
+	.string	"unsigned char"
+.LASF1:
+	.string	"char"
+.LASF5:
+	.string	"long int"
+.LASF11:
+	.string	"double"
+.LASF7:
+	.string	"short unsigned int"
+.LASF9:
+	.string	"signed char"
+.LASF3:
+	.string	"float"
+.LASF0:
+	.string	"short int"
+	.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 0a0ae697d1e..bced47eda99 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3939,6 +3939,78 @@ gdb_caching_proc allow_btrace_pt_tests {} {
     return $allow_btrace_pt_tests
 }
 
+# Run a test on the target to see if it supports ptwrite instructions and
+# if GDB can decode ptwrite events.  Return 1 if so, 0 if it does not.
+
+gdb_caching_proc allow_btrace_ptw_tests {} {
+    global srcdir subdir gdb_prompt inferior_exited_re decimal
+
+    require allow_btrace_pt_tests
+    set me "allow_btrace_ptw_tests"
+
+    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 allow_btrace_ptw_tests 2
+    gdb_test_multiple "next" "$me: next" {
+	-re -wrap ".*Illegal instruction.*" {
+	    verbose -log "$me:  ptwrite instruction support not detected."
+	    set allow_btrace_ptw_tests 0
+	}
+	-re -wrap ".*$inferior_exited_re normally.*" {
+	    verbose -log "$me:  ptwrite support not detected."
+	    set allow_btrace_ptw_tests 0
+	}
+	-re -wrap "$decimal.*(at|in|return 0).*" {
+	    set allow_btrace_ptw_tests 1
+	}
+    }
+
+    if { $allow_btrace_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 allow_btrace_ptw_tests 1
+	    }
+	    -re ".*${gdb_prompt} $" {
+		verbose -log "$me:  ptwrite decoding support not detected."
+		set allow_btrace_ptw_tests 0
+	    }
+	}
+    }
+
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me:  returning $allow_btrace_ptw_tests" 2
+    return $allow_btrace_ptw_tests
+}
+
+
 # Run a test on the target to see if it supports Aarch64 SVE hardware.
 # Return 1 if so, 0 if it does not.  Note this causes a restart of GDB.
 
diff --git a/gdbsupport/common.m4 b/gdbsupport/common.m4
index 3909ec81ccb..1ad2395e80f 100644
--- a/gdbsupport/common.m4
+++ b/gdbsupport/common.m4
@@ -190,6 +190,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 2cdc1fbde61..8ce8e23bc24 100644
--- a/gdbsupport/config.in
+++ b/gdbsupport/config.in
@@ -238,6 +238,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 9433ac41468..7a10c96bd75 100755
--- a/gdbsupport/configure
+++ b/gdbsupport/configure
@@ -9625,6 +9625,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.39.2

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

Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> 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                                  |   7 +
>  gdb/btrace.c                              |  54 +++
>  gdb/config.in                             |   3 +
>  gdb/configure                             |  11 +
>  gdb/doc/python.texi                       | 150 ++++++
>  gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 550 ++++++++++++++++++++++
>  gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
>  gdb/testsuite/gdb.btrace/ptwrite.exp      | 200 ++++++++
>  gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 544 +++++++++++++++++++++
>  gdb/testsuite/lib/gdb.exp                 |  72 +++
>  gdbsupport/common.m4                      |   2 +
>  gdbsupport/config.in                      |   3 +
>  gdbsupport/configure                      |  11 +
>  13 files changed, 1646 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 cc262f1f8a6..5dd05867f2a 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -106,6 +106,13 @@ show always-read-ctf
>  
>  *** Changes in GDB 13
>  
> +* GDB now supports printing of ptwrite payloads from the Intel Processor
> +  Trace during 'record instruction-history', 'record function-call-history'
> +  and all stepping commands.  The payload is also accessible in Python as a
> +  RecordAuxiliary object.  Printing is customizable via a ptwrite filter
> +  function in Python.  By default, the raw ptwrite payload is printed for
> +  each ptwrite that is encountered.
> +
>  * MI version 1 is deprecated, and will be removed in GDB 14.
>  
>  * GDB now supports dumping memory tag data for AArch64 MTE.  It also supports
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 37dd0b666d8..db0d0e291d9 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1253,6 +1253,54 @@ 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_context);

If ptw_callback_fun is nullptr, the string will always be empty, and the
data not shown?  This means that in a build without Python, the data
will never be shown?

I think a better approach would be to handle the default case (print as
hex) here, instead of in a Python function.  I think that
ptw_callback_fun should return a gdb::optional<std::string>.  If the
optional is instantiated (including containing an empty string), use
that value.  If ptw_callback_fun is nullptr (perhaps because GDB was
built with no Python support), or if the returned optional is not
instantiated (perhaps because the user did not register any filter),
generate the default (print as hex) here.


> +
> +	    if (ptw_string.empty ())
> +	      break;
> +
> +	    btinfo->aux_data.emplace_back (ptw_string);

Since you don't need ptw_string below, you can std::move it in the
vector.


> +@end defun
> +
> +@findex gdb.ptwrite.get_filter
> +@defun get_filter ()
> +Return the currently active @code{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

Ok, I guess that answers my question about deepcopy.  I think it would
be good to say that the copy is done using the deepcopy module/function.

Simon

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

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 16:41
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > 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                                  |   7 +
> >  gdb/btrace.c                              |  54 +++
> >  gdb/config.in                             |   3 +
> >  gdb/configure                             |  11 +
> >  gdb/doc/python.texi                       | 150 ++++++
> >  gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 550
> ++++++++++++++++++++++
> >  gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
> >  gdb/testsuite/gdb.btrace/ptwrite.exp      | 200 ++++++++
> >  gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 544
> +++++++++++++++++++++
> >  gdb/testsuite/lib/gdb.exp                 |  72 +++
> >  gdbsupport/common.m4                      |   2 +
> >  gdbsupport/config.in                      |   3 +
> >  gdbsupport/configure                      |  11 +
> >  13 files changed, 1646 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 cc262f1f8a6..5dd05867f2a 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -106,6 +106,13 @@ show always-read-ctf
> >
> >  *** Changes in GDB 13
> >
> > +* GDB now supports printing of ptwrite payloads from the Intel Processor
> > +  Trace during 'record instruction-history', 'record function-call-history'
> > +  and all stepping commands.  The payload is also accessible in Python as a
> > +  RecordAuxiliary object.  Printing is customizable via a ptwrite filter
> > +  function in Python.  By default, the raw ptwrite payload is printed for
> > +  each ptwrite that is encountered.
> > +
> >  * MI version 1 is deprecated, and will be removed in GDB 14.
> >
> >  * GDB now supports dumping memory tag data for AArch64 MTE.  It also
> supports
> > diff --git a/gdb/btrace.c b/gdb/btrace.c
> > index 37dd0b666d8..db0d0e291d9 100644
> > --- a/gdb/btrace.c
> > +++ b/gdb/btrace.c
> > @@ -1253,6 +1253,54 @@ 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_context);
> 
> If ptw_callback_fun is nullptr, the string will always be empty, and the
> data not shown?  This means that in a build without Python, the data
> will never be shown?
> 
> I think a better approach would be to handle the default case (print as
> hex) here, instead of in a Python function.  I think that
> ptw_callback_fun should return a gdb::optional<std::string>.  If the
> optional is instantiated (including containing an empty string), use
> that value.  If ptw_callback_fun is nullptr (perhaps because GDB was
> built with no Python support), or if the returned optional is not
> instantiated (perhaps because the user did not register any filter),
> generate the default (print as hex) here.
> 

We wanted the user to be able to register None as a listener. But I think
that might still be from a time where we didn't implement
the /a modifiers for the instruction/function-history CLI commands to
enable not printing the strings.

I think your suggestion makes sense. I guess in that case we should try
to get rid of the current default filter in Python. To not have two places
in the code that do the same thing.

> > +
> > +	    if (ptw_string.empty ())
> > +	      break;
> > +
> > +	    btinfo->aux_data.emplace_back (ptw_string);
> 
> Since you don't need ptw_string below, you can std::move it in the
> vector.

Right, I will do that.

> > +@end defun
> > +
> > +@findex gdb.ptwrite.get_filter
> > +@defun get_filter ()
> > +Return the currently active @code{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
> 
> Ok, I guess that answers my question about deepcopy.  I think it would
> be good to say that the copy is done using the deepcopy module/function.
> 

I will add that.

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

Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.

[Simon Marchi]

On 3/31/23 06:58, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: Simon Marchi <simark@simark.ca>
>> Sent: Freitag, 24. März 2023 16:41
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH v8 10/10] btrace: Extend ptwrite event decoding.
>>
>> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
>>> 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                                  |   7 +
>>>  gdb/btrace.c                              |  54 +++
>>>  gdb/config.in                             |   3 +
>>>  gdb/configure                             |  11 +
>>>  gdb/doc/python.texi                       | 150 ++++++
>>>  gdb/testsuite/gdb.btrace/i386-ptwrite.S   | 550
>> ++++++++++++++++++++++
>>>  gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
>>>  gdb/testsuite/gdb.btrace/ptwrite.exp      | 200 ++++++++
>>>  gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 544
>> +++++++++++++++++++++
>>>  gdb/testsuite/lib/gdb.exp                 |  72 +++
>>>  gdbsupport/common.m4                      |   2 +
>>>  gdbsupport/config.in                      |   3 +
>>>  gdbsupport/configure                      |  11 +
>>>  13 files changed, 1646 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 cc262f1f8a6..5dd05867f2a 100644
>>> --- a/gdb/NEWS
>>> +++ b/gdb/NEWS
>>> @@ -106,6 +106,13 @@ show always-read-ctf
>>>
>>>  *** Changes in GDB 13
>>>
>>> +* GDB now supports printing of ptwrite payloads from the Intel Processor
>>> +  Trace during 'record instruction-history', 'record function-call-history'
>>> +  and all stepping commands.  The payload is also accessible in Python as a
>>> +  RecordAuxiliary object.  Printing is customizable via a ptwrite filter
>>> +  function in Python.  By default, the raw ptwrite payload is printed for
>>> +  each ptwrite that is encountered.
>>> +
>>>  * MI version 1 is deprecated, and will be removed in GDB 14.
>>>
>>>  * GDB now supports dumping memory tag data for AArch64 MTE.  It also
>> supports
>>> diff --git a/gdb/btrace.c b/gdb/btrace.c
>>> index 37dd0b666d8..db0d0e291d9 100644
>>> --- a/gdb/btrace.c
>>> +++ b/gdb/btrace.c
>>> @@ -1253,6 +1253,54 @@ 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_context);
>>
>> If ptw_callback_fun is nullptr, the string will always be empty, and the
>> data not shown?  This means that in a build without Python, the data
>> will never be shown?
>>
>> I think a better approach would be to handle the default case (print as
>> hex) here, instead of in a Python function.  I think that
>> ptw_callback_fun should return a gdb::optional<std::string>.  If the
>> optional is instantiated (including containing an empty string), use
>> that value.  If ptw_callback_fun is nullptr (perhaps because GDB was
>> built with no Python support), or if the returned optional is not
>> instantiated (perhaps because the user did not register any filter),
>> generate the default (print as hex) here.
>>
> 
> We wanted the user to be able to register None as a listener. But I think
> that might still be from a time where we didn't implement
> the /a modifiers for the instruction/function-history CLI commands to
> enable not printing the strings.

Passing None could be used to clear the registered filter, to go back to
the default state.

> I think your suggestion makes sense. I guess in that case we should try
> to get rid of the current default filter in Python. To not have two places
> in the code that do the same thing.

Agreed.

>>> +@end defun
>>> +
>>> +@findex gdb.ptwrite.get_filter
>>> +@defun get_filter ()
>>> +Return the currently active @code{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
>>
>> Ok, I guess that answers my question about deepcopy.  I think it would
>> be good to say that the copy is done using the deepcopy module/function.
>>
> 
> I will add that.

Note that with my suggestion of using a factory function instead, that
suggestion would be moot.

Simon

Re: [PATCH v8 00/10] Extensions for PTWRITE

[Simon Marchi]

On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> Eli already approved the documentation parts.
> Markus reviewed every patch and approved the btrace bits.
> The only thing that is missing is a maintainers approval for the Python
> bits.
> 
> v7 can be found here:
> https://sourceware.org/pipermail/gdb-patches/2022-October/192830.html
> 
> I pinged this version 12 times, but didn't get any response.
> Since the last time I posted this was half a year ago, I decided to re-post a
> rebased version again.  I only had to solve some minor conflicts.  The
> biggest change was using the new require logic for the testing.

Hi Felix,

Sorry for that.  With the amount of stuff on the list, it's easy to
overlook things and think "ehhh, I'm not comfortable reviewing this,
I'll let someone else do it".  I will check the Python bits.

git shows these little things to fix when applying the series:

    ➜  binutils-gdb git:(master) ✗ git am ./v8_20230321_felix_willgerodt_extensions_for_ptwrite.mbx
    Applying: btrace: Introduce auxiliary instructions.
    Applying: btrace: Enable auxiliary instructions in record instruction-history.
    Applying: btrace: Enable auxiliary instructions in record function-call-history.
    Applying: btrace: Handle stepping and goto for auxiliary instructions.
    .git/rebase-apply/patch:29: indent with spaces.
            {
    .git/rebase-apply/patch:30: indent with spaces.
              *replay = start;
    .git/rebase-apply/patch:31: indent with spaces.
              return btrace_step_no_history ();
    .git/rebase-apply/patch:32: indent with spaces.
            }
    warning: 4 lines add whitespace errors.
    Applying: python: Introduce gdb.RecordAuxiliary class.
    Applying: python: Add clear() to gdb.Record.
    .git/rebase-apply/patch:106: trailing whitespace.
        gdb_test "python print(i.is_speculative)" "False"
    warning: 1 line adds whitespace errors.
    Applying: btrace, gdbserver: Add ptwrite to btrace_config_pt.
    Applying: btrace, linux: Enable ptwrite packets.
    Applying: btrace, python: Enable ptwrite filter registration.
    Applying: btrace: Extend ptwrite event decoding.
    .git/rebase-apply/patch:285: trailing whitespace.
    (gdb) info threads
    .git/rebase-apply/patch:1187: trailing whitespace.
    # SUCC: EXIT [always]
    .git/rebase-apply/patch:1227: trailing whitespace.
    # SUCC: EXIT [always]
    .git/rebase-apply/patch:1261: trailing whitespace.
    # SUCC: EXIT [always]
    warning: 4 lines add whitespace errors.

Simon

Re: [PATCH v8 00/10] Extensions for PTWRITE

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Sorry for that.  With the amount of stuff on the list, it's easy to
Simon> overlook things and think "ehhh, I'm not comfortable reviewing this,
Simon> I'll let someone else do it".

This is exactly what I did as well.  I see it as a kind of collective
action problem.

It's clear we need a new system here, but I don't know what it is.  I
was hoping patchworks, but that turned out to require too much manual
intervention, IMO.

Maybe we should simply go back to gerrit, despite its issues.  Perhaps
we could do it in conjunction with a robot that auto-assigns reviews.

Another option that would work with the above is to add more reviewers.
However, I don't think that just doing this would be enough -- there is
a structural problem here that we should address.

Tom

Re: [PATCH v8 00/10] Extensions for PTWRITE

[Simon Marchi]

On 3/24/23 14:23, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Sorry for that.  With the amount of stuff on the list, it's easy to
> Simon> overlook things and think "ehhh, I'm not comfortable reviewing this,
> Simon> I'll let someone else do it".
> 
> This is exactly what I did as well.  I see it as a kind of collective
> action problem.
> 
> It's clear we need a new system here, but I don't know what it is.  I
> was hoping patchworks, but that turned out to require too much manual
> intervention, IMO.
> 
> Maybe we should simply go back to gerrit, despite its issues.  Perhaps
> we could do it in conjunction with a robot that auto-assigns reviews.

I don't think just Gerrit would have helped, I would have ignored it on
Gerrit as well.  Perhaps some robot, as you said, would have helped, but
I'm not sure it's good to just randomly assign reviews.  We all have our
areas of knowledge, and I think it's good to respect that.

Perhaps some interactive patch triage / review session like glibc has
would help, but I'm not sure people (including me) want to commit to a
regular meeting for that.

Simon

Re: [PATCH v8 00/10] Extensions for PTWRITE

[Tom Tromey]

Simon> I don't think just Gerrit would have helped, I would have ignored it on
Simon> Gerrit as well.  Perhaps some robot, as you said, would have helped, but
Simon> I'm not sure it's good to just randomly assign reviews.  We all have our
Simon> areas of knowledge, and I think it's good to respect that.

What Rust did is have a robot auto-assign, but then the assignee could
reassign the review to someone else if they thought they wouldn't do it
well.

The idea is to avoid the situation where no one in particular is
responsible for a given patch.

Tom

RE: [PATCH v8 00/10] Extensions for PTWRITE

[Willgerodt, Felix]

> -----Original Message-----
> From: Simon Marchi <simark@simark.ca>
> Sent: Freitag, 24. März 2023 14:57
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
> patches@sourceware.org
> Subject: Re: [PATCH v8 00/10] Extensions for PTWRITE
> 
> On 3/21/23 11:46, Felix Willgerodt via Gdb-patches wrote:
> > Eli already approved the documentation parts.
> > Markus reviewed every patch and approved the btrace bits.
> > The only thing that is missing is a maintainers approval for the Python
> > bits.
> >
> > v7 can be found here:
> > https://sourceware.org/pipermail/gdb-patches/2022-October/192830.html
> >
> > I pinged this version 12 times, but didn't get any response.
> > Since the last time I posted this was half a year ago, I decided to re-post a
> > rebased version again.  I only had to solve some minor conflicts.  The
> > biggest change was using the new require logic for the testing.
> 
> Hi Felix,
> 
> Sorry for that.  With the amount of stuff on the list, it's easy to
> overlook things and think "ehhh, I'm not comfortable reviewing this,
> I'll let someone else do it".  I will check the Python bits.

No worries, thanks for doing that!

> git shows these little things to fix when applying the series:
> 
>     ➜  binutils-gdb git:(master) ✗ git am
> ./v8_20230321_felix_willgerodt_extensions_for_ptwrite.mbx
>     Applying: btrace: Introduce auxiliary instructions.
>     Applying: btrace: Enable auxiliary instructions in record instruction-history.
>     Applying: btrace: Enable auxiliary instructions in record function-call-
> history.
>     Applying: btrace: Handle stepping and goto for auxiliary instructions.
>     .git/rebase-apply/patch:29: indent with spaces.
>             {
>     .git/rebase-apply/patch:30: indent with spaces.
>               *replay = start;
>     .git/rebase-apply/patch:31: indent with spaces.
>               return btrace_step_no_history ();
>     .git/rebase-apply/patch:32: indent with spaces.
>             }
>     warning: 4 lines add whitespace errors.
>     Applying: python: Introduce gdb.RecordAuxiliary class.
>     Applying: python: Add clear() to gdb.Record.
>     .git/rebase-apply/patch:106: trailing whitespace.
>         gdb_test "python print(i.is_speculative)" "False"
>     warning: 1 line adds whitespace errors.
>     Applying: btrace, gdbserver: Add ptwrite to btrace_config_pt.
>     Applying: btrace, linux: Enable ptwrite packets.
>     Applying: btrace, python: Enable ptwrite filter registration.
>     Applying: btrace: Extend ptwrite event decoding.
>     .git/rebase-apply/patch:285: trailing whitespace.
>     (gdb) info threads
>     .git/rebase-apply/patch:1187: trailing whitespace.
>     # SUCC: EXIT [always]
>     .git/rebase-apply/patch:1227: trailing whitespace.
>     # SUCC: EXIT [always]
>     .git/rebase-apply/patch:1261: trailing whitespace.
>     # SUCC: EXIT [always]
>     warning: 4 lines add whitespace errors.
> 
> Simon

Argh, thanks for pointing those out. I will fix them.

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