[PATCH v3 00/12] Extensions for PTWRITE

[PATCH v3 00/12] Extensions for PTWRITE

[Felix Willgerodt]

V1 can be found here:
https://sourceware.org/pipermail/gdb-patches/2019-May/157933.html

V2 can be found here:
https://sourceware.org/pipermail/gdb-patches/2021-June/179908.html

Patch 1 was already approved, and so was all of the documentation
apart from Patch 12.

Changes compared to v2:
* Fixed Lancelot's comments on ptwrite.py in patch 9.
* Addressed Eli's comments on python.texi in patch 12.

Regards,
Felix

Felix Willgerodt (12):
  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 listener registration.
  btrace, python: Enable calling the ptwrite listener.
  gdb, testsuite, lib: Add libipt version check.
  btrace: Extend ptwrite event decoding.

 gdb/NEWS                                      |   6 +
 gdb/btrace.c                                  |  64 ++-
 gdb/btrace.h                                  |  41 +-
 gdb/data-directory/Makefile.in                |   1 +
 gdb/disasm.h                                  |   1 +
 gdb/doc/gdb.texinfo                           |  33 +-
 gdb/doc/python.texi                           | 152 ++++++
 gdb/extension-priv.h                          |   4 +
 gdb/extension.c                               |  21 +
 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                 |  84 +++
 gdb/python/py-record-btrace.c                 | 180 ++++++-
 gdb/python/py-record-btrace.h                 |  14 +
 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                           | 105 +++-
 gdb/record.c                                  |  10 +
 gdb/record.h                                  |   5 +-
 gdb/remote.c                                  |  30 ++
 gdb/testsuite/gdb.btrace/ptwrite.c            |  39 ++
 gdb/testsuite/gdb.btrace/ptwrite.exp          | 194 +++++++
 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S     | 479 ++++++++++++++++++
 gdb/testsuite/gdb.python/py-record-btrace.exp |   6 +-
 gdb/testsuite/lib/gdb.exp                     | 151 +++++-
 gdbserver/linux-low.cc                        |   1 +
 gdbserver/server.cc                           |  16 +
 gdbsupport/btrace-common.h                    |   6 +
 32 files changed, 1733 insertions(+), 41 deletions(-)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py
 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.25.4

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 v3 01/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.c (btrace_clear_history): Clear aux_data.
	* btrace.h (btrace_insn_class): Add BTRACE_INSN_AUX.
	(btrace_insn): New union.
	(btrace_thread_info): New member aux_data.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.texinfo (Process Record and Replay): Document printing of
	auxiliary information.
---
 gdb/btrace.c        |  2 ++
 gdb/btrace.h        | 24 +++++++++++++++++++++---
 gdb/doc/gdb.texinfo |  4 ++++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 5e689c11d4b..9f117a5e90d 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1811,6 +1811,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 8f6ce32103d..670c182505d 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 data 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 d09b86cda95..ff4f41941d3 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7467,6 +7467,10 @@ Moxie, PowerPC, PowerPC64, S/390, and x86 (i386/amd64) running
 GNU/Linux.  Process record and replay can be used both when native
 debugging, and when remote debugging via @code{gdbserver}.
 
+When recording an inferior, @value{GDBN} may print additional auxiliary
+information during stepping commands and commands displaying the execution
+history.
+
 For architecture environments that support process record and replay,
 @value{GDBN} provides the following commands:
 
-- 
2.25.4

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 v3 01/12] btrace: Introduce auxiliary instructions.

[Metzger, Markus T]

Thanks, Felix,

The patch looks good with two small nits.

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

Should we add size to the union, too, or is this needed for AUX instructions?
How about a name for the union?


>   /* 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 data that is printed in all commands displaying or
>+     stepping through the execution history.  */
>+  std::vector<std::string> aux_data;

This is not the data itself but a human-readable representation, correct?
Should we say 'information' instead of 'data'?

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

RE: [PATCH v3 01/12] btrace: Introduce auxiliary instructions.

[Willgerodt, Felix]

Sorry for taking so long again, answers below.

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:07
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 01/12] btrace: Introduce auxiliary instructions.
> 
> Thanks, Felix,
> 
> The patch looks good with two small nits.
> 
> > /* 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;
> >+  };
> 
> Should we add size to the union, too, or is this needed for AUX instructions?
> How about a name for the union?

Size is not needed for AUX right now and I don't think it ever will be.
If I would add it to the union, I would need to come up with another struct that
included pc and size to put them in the union. And touch all users.
I don't see too much benefit, but I can do that if you prefer.

I explicitly didn't name it to prevent touching all users.

> 
> >   /* 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 data that is printed in all commands displaying or
> >+     stepping through the execution history.  */
> >+  std::vector<std::string> aux_data;
> 
> This is not the data itself but a human-readable representation, correct?
> Should we say 'information' instead of 'data'?
>

I changed it to information.

> 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

[PATCH v3 02/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* record-btrace.c (btrace_insn_history): Handle BTRACE_INSN_AUX.
	* record.c (get_insn_history_modifiers): Add /a modifier.
	* disasm.h (gdb_disassembly_flag): Add DISASSEMBLY_OMIT_AUX_INSN.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.texinfo (record instruction history): Document /a modifier.
---
 gdb/disasm.h        |  1 +
 gdb/doc/gdb.texinfo |  3 +++
 gdb/record-btrace.c | 13 +++++++++++++
 gdb/record.c        |  5 +++++
 4 files changed, 22 insertions(+)

diff --git a/gdb/disasm.h b/gdb/disasm.h
index eb82bc3cd01..4db5bd943eb 100644
--- a/gdb/disasm.h
+++ b/gdb/disasm.h
@@ -31,6 +31,7 @@ enum gdb_disassembly_flag
     DISASSEMBLY_OMIT_PC = (0x1 << 4),
     DISASSEMBLY_SOURCE = (0x1 << 5),
     DISASSEMBLY_SPECULATIVE = (0x1 << 6),
+    DISASSEMBLY_OMIT_AUX_INSN = (0x1 << 7),
   };
 DEF_ENUM_FLAGS_TYPE (enum gdb_disassembly_flag, gdb_disassembly_flags);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index ff4f41941d3..8f49a91347f 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7883,6 +7883,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 00affb85d22..96c1b69cb0f 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -821,6 +821,19 @@ 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[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 6968c30d930..084c8a12c16 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;
@@ -854,6 +857,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.25.4

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 v3 02/12] btrace: Enable auxiliary instructions in record instruction-history.

[Metzger, Markus T]

Thanks, Felix,

The patch looks good to me with one small nit.

>+	  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[insn->aux_data_index].c_str ());

I think we should check bounds, here, e.g. by using at() instead of [].

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 v3 02/12] btrace: Enable auxiliary instructions in record instruction-history.

[Willgerodt, Felix]

Done in the next version.

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:07
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 02/12] btrace: Enable auxiliary instructions in record
> instruction-history.
> 
> Thanks, Felix,
> 
> The patch looks good to me with one small nit.
> 
> >+	  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[insn->aux_data_index].c_str
> ());
> 
> I think we should check bounds, here, e.g. by using at() instead of [].
> 
> 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

[PATCH v3 03/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.h (btrace_function_flag): Add BFUN_AUX_DECODED.
	* record-btrace.c (btrace_print_aux_insn): New function.
	(btrace_call_history): Handle BTRACE_INSN_AUX.
	* record.c: (get_call_history_modifiers): Add /a modifier.
	* record.h (record_print_flag): New value RECORD_DONT_PRINT_AUX.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.texinfo (record function-call-history): Document /a modifier.
---
 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 670c182505d..e6694551c70 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -105,7 +105,11 @@ enum btrace_function_flag
 
   /* The 'up' link points to a tail call.  This obviously only makes sense
      if bfun_up_links_to_ret is clear.  */
-  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1)
+  BFUN_UP_LINKS_TO_TAILCALL = (1 << 1),
+
+  /* Indicates that at least one auxiliary instruction is in the current
+     function segment.  */
+  BFUN_AUX_DECODED = (1 << 2)
 };
 DEF_ENUM_FLAGS_TYPE (enum btrace_function_flag, btrace_function_flags);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 8f49a91347f..5e4f8e73b7d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -7940,8 +7940,9 @@ 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 96c1b69cb0f..2fc273ca229 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -1152,6 +1152,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 auto &i : bfun->insn)
+    {
+      if (i.iclass == BTRACE_INSN_AUX)
+	{
+	  uiout->text ("\t\t[");
+	  uiout->field_fmt ("aux-data", "%s",
+			    btinfo->aux_data[i.aux_data_index].c_str ());
+	  uiout->text ("]\n");
+	}
+    }
+}
+
 /* Disassemble a section of the recorded function trace.  */
 
 static void
@@ -1227,6 +1244,10 @@ btrace_call_history (struct ui_out *uiout,
 	}
 
       uiout->text ("\n");
+
+      if (((flags & RECORD_DONT_PRINT_AUX) == 0)
+	  && ((bfun->flags & BFUN_AUX_DECODED) != 0))
+	btrace_print_aux_insn(uiout, bfun, btinfo);
     }
 }
 
diff --git a/gdb/record.c b/gdb/record.c
index 084c8a12c16..17a3585a1a5 100644
--- a/gdb/record.c
+++ b/gdb/record.c
@@ -636,6 +636,9 @@ get_call_history_modifiers (const char **arg)
 	    case 'c':
 	      modifiers |= RECORD_PRINT_INDENT_CALLS;
 	      break;
+	    case 'a':
+	      modifiers |= RECORD_DONT_PRINT_AUX;
+	      break;
 	    default:
 	      error (_("Invalid modifier: %c."), *args);
 	    }
@@ -881,6 +884,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 ac3c84d20ea..3213387920b 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.25.4

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 v3 03/12] btrace: Enable auxiliary instructions in record function-call-history.

[Metzger, Markus T]

Thanks, Felix,

This patch looks good with two small nits.

>+static void
>+btrace_print_aux_insn (struct ui_out *uiout,
>+		       const struct btrace_function *bfun,
>+		       const struct btrace_thread_info *btinfo)
>+{
>+  for (const auto &i : bfun->insn)

Please specify the type instead of 'auto'.  Let's also rename 'i' to 'insn'.

>+    {
>+      if (i.iclass == BTRACE_INSN_AUX)
>+	{
>+	  uiout->text ("\t\t[");
>+	  uiout->field_fmt ("aux-data", "%s",
>+			    btinfo->aux_data[i.aux_data_index].c_str ());

Please check bounds.

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 v3 03/12] btrace: Enable auxiliary instructions in record function-call-history.

[Willgerodt, Felix]

Both are fixed in the next version.

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:14
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 03/12] btrace: Enable auxiliary instructions in record
> function-call-history.
> 
> Thanks, Felix,
> 
> This patch looks good with two small nits.
> 
> >+static void
> >+btrace_print_aux_insn (struct ui_out *uiout,
> >+		       const struct btrace_function *bfun,
> >+		       const struct btrace_thread_info *btinfo)
> >+{
> >+  for (const auto &i : bfun->insn)
> 
> Please specify the type instead of 'auto'.  Let's also rename 'i' to 'insn'.
> 
> >+    {
> >+      if (i.iclass == BTRACE_INSN_AUX)
> >+	{
> >+	  uiout->text ("\t\t[");
> >+	  uiout->field_fmt ("aux-data", "%s",
> >+			    btinfo->aux_data[i.aux_data_index].c_str ());
> 
> Please check bounds.
> 
> 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

[PATCH v3 04/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* record-btrace.c: (record_btrace_single_step_forward): Handle
	BTRACE_INSN_AUX.
	(record_btrace_single_step_backward): Likewise.
	(record_btrace_goto): Likewise.
---
 gdb/record-btrace.c | 66 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 53 insertions(+), 13 deletions(-)

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 2fc273ca229..3d0c344e8e6 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -2377,6 +2377,7 @@ record_btrace_single_step_forward (struct thread_info *tp)
 {
   struct btrace_insn_iterator *replay, end, start;
   struct btrace_thread_info *btinfo;
+  struct btrace_insn next_insn;
 
   btinfo = &tp->btrace;
   replay = btinfo->replay;
@@ -2390,9 +2391,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;
 
@@ -2400,12 +2405,25 @@ 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 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 ();
+	  printf_unfiltered ("[%s]\n",
+			     btinfo->aux_data[insn->aux_data_index].c_str ());
+	  continue;
 	}
+
+      if (insn != NULL)
+	break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Determine the end of the instruction trace.  */
   btrace_insn_end (&end, btinfo);
@@ -2426,6 +2444,7 @@ record_btrace_single_step_backward (struct thread_info *tp)
 {
   struct btrace_insn_iterator *replay, start;
   struct btrace_thread_info *btinfo;
+  struct btrace_insn prev_insn;
 
   btinfo = &tp->btrace;
   replay = btinfo->replay;
@@ -2436,9 +2455,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;
 
@@ -2448,8 +2470,20 @@ 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);
+
+      /* Check if we're stepping a BTRACE_INSN_AUX instruction and skip it.  */
+      if (insn->iclass == BTRACE_INSN_AUX)
+	{
+	  printf_unfiltered ("[%s]\n",
+			     btinfo->aux_data[insn->aux_data_index].c_str ());
+	  continue;
+	}
+
+      if (insn != NULL)
+	break;
     }
-  while (btrace_insn_get (replay) == NULL);
 
   /* Check if we're stepping a breakpoint.
 
@@ -2872,25 +2906,31 @@ record_btrace_target::goto_record_end ()
 /* The goto_record method of target record-btrace.  */
 
 void
-record_btrace_target::goto_record (ULONGEST insn)
+record_btrace_target::goto_record (ULONGEST insn_number)
 {
   struct thread_info *tp;
   struct btrace_insn_iterator it;
   unsigned int number;
   int found;
 
-  number = insn;
+  number = insn_number;
 
   /* Check for wrap-arounds.  */
-  if (number != insn)
+  if (number != insn_number)
     error (_("Instruction number out of range."));
 
   tp = require_btrace_thread ();
 
   found = btrace_find_insn_by_number (&it, &tp->btrace, number);
 
-  /* Check if the instruction could not be found or is a gap.  */
-  if (found == 0 || btrace_insn_get (&it) == NULL)
+  /* Check if the instruction could not be found or is a gap or an
+     auxilliary instruction.  */
+  if (found == 0)
+    error (_("No such instruction."));
+
+  const struct btrace_insn *insn = btrace_insn_get (&it);
+
+  if ((insn == NULL) || (insn->iclass == BTRACE_INSN_AUX))
     error (_("No such instruction."));
 
   record_btrace_set_replay (tp, &it);
-- 
2.25.4

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 v3 04/12] btrace: Handle stepping and goto for auxiliary instructions.

[Metzger, Markus T]

Thanks, Felix,

The patch looks good with one issue.

>@@ -2400,12 +2405,25 @@ 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 we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
>+	 data and skip the instruction.  */
>+      if (insn->iclass == BTRACE_INSN_AUX)

We need to check for nullptr.  Btrace_insn_get() return nullptr in case of trace errors, where we add a gap to the trace.

> 	{
>-	  *replay = start;
>-	  return btrace_step_no_history ();
>+	  printf_unfiltered ("[%s]\n",
>+			     btinfo->aux_data[insn->aux_data_index].c_str ());
>+	  continue;
> 	}
>+
>+      if (insn != NULL)
>+	break;

Here you add the check.  Note that NULL is spelled nullptr nowadays.

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 v3 04/12] btrace: Handle stepping and goto for auxiliary instructions.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:07
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 04/12] btrace: Handle stepping and goto for auxiliary
> instructions.
> 
> Thanks, Felix,
> 
> The patch looks good with one issue.
> 
> >@@ -2400,12 +2405,25 @@ 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 we're stepping a BTRACE_INSN_AUX instruction, print the auxiliary
> >+	 data and skip the instruction.  */
> >+      if (insn->iclass == BTRACE_INSN_AUX)
> 
> We need to check for nullptr.  Btrace_insn_get() return nullptr in case of
> trace errors, where we add a gap to the trace.

Done. Same for step_backwards.

> > 	{
> >-	  *replay = start;
> >-	  return btrace_step_no_history ();
> >+	  printf_unfiltered ("[%s]\n",
> >+			     btinfo->aux_data[insn->aux_data_index].c_str ());
> >+	  continue;
> > 	}
> >+
> >+      if (insn != NULL)
> >+	break;
> 
> Here you add the check.  Note that NULL is spelled nullptr nowadays.

I removed the if now that we have a nullptr check above, we can just break here.

> 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

[PATCH v3 05/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* py-record-btrace.c (btpy_insn_or_gap_new): Removed.
	(btpy_item_new): New function.
	(btpy_list_item): Call btpy_item_new instead of recpy_insn_new.
	(recpy_bt_replay_position): Call btpy_item_new instead of
	btpy_insn_or_gap_new.
	(recpy_bt_begin): Call btpy_item_new instead of btpy_insn_or_gap_new.
	(recpy_bt_end): Call btpy_item_new instead of btpy_insn_or_gap_new.
	* py-record.c (recpy_aux_type): New static object.
	(recpy_aux_object): New typedef.
	(recpy_aux_new, recpy_aux_number, recpy_aux_data): New function.
	(recpy_aux_getset): New static object.
	(gdbpy_initialize_record): Initialize gdb.RecordAuxiliary type.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* python.texi (gdb.RecordAuxiliary): New documentation.
---
 gdb/doc/python.texi           | 13 +++++++
 gdb/python/py-record-btrace.c | 33 ++++++++++------
 gdb/python/py-record.c        | 73 ++++++++++++++++++++++++++++++++++-
 gdb/python/py-record.h        |  3 ++
 4 files changed, 109 insertions(+), 13 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index ab934a8c012..333732f42f2 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3623,6 +3623,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 1c10a0598da..128dd14dd81 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -55,7 +55,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;
 };
 
@@ -151,10 +152,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;
@@ -173,6 +175,12 @@ 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);
+
+  if (insn->iclass == BTRACE_INSN_AUX)
+    return recpy_aux_new (iter.btinfo->aux_data[insn->aux_data_index].c_str (),
+			  number);
+
   return recpy_insn_new (tinfo, RECORD_METHOD_BTRACE, number);
 }
 
@@ -456,8 +464,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)
@@ -471,10 +481,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.  */
@@ -661,8 +671,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
@@ -684,7 +693,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
@@ -706,7 +715,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 1747f74d7e6..481cd282ba3 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 (NULL, 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 PyString_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 == NULL)
+   return NULL;
+
+  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 PyString_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, NULL, "element number", NULL},
+  { "data", recpy_aux_data, NULL, "data", NULL},
+  { NULL }
+};
+
 /* 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 c1df6fde6e8..51dbc0ecafb 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.25.4

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 v3 05/12] python: Introduce gdb.RecordAuxiliary class.

[Metzger, Markus T]

Thanks, Felix,

This patch looks good to me with a few nits.

>       return recpy_gap_new (err_code, err_string, number);
>     }
>
>+  const struct btrace_insn *insn = btrace_insn_get (&iter);
>+
>+  if (insn->iclass == BTRACE_INSN_AUX)

We already checked that this is no gap, so INSN should really be non-nullptr.  Shall we assert this?


>+    return recpy_aux_new (iter.btinfo->aux_data[insn->aux_data_index].c_str (),
>+			  number);

Let's check bounds.


>+/* Python RecordAuxiliary type.  */
>+
>+PyTypeObject recpy_aux_type = {
>+  PyVarObject_HEAD_INIT (NULL, 0)

NULL is spelled nullptr.  More below.

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 v3 05/12] python: Introduce gdb.RecordAuxiliary class.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:07
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 05/12] python: Introduce gdb.RecordAuxiliary class.
> 
> Thanks, Felix,
> 
> This patch looks good to me with a few nits.
> 
> >       return recpy_gap_new (err_code, err_string, number);
> >     }
> >
> >+  const struct btrace_insn *insn = btrace_insn_get (&iter);
> >+
> >+  if (insn->iclass == BTRACE_INSN_AUX)
> 
> We already checked that this is no gap, so INSN should really be non-nullptr.
> Shall we assert this?

Done.

> 
> >+    return recpy_aux_new (iter.btinfo->aux_data[insn-
> >aux_data_index].c_str (),
> >+			  number);
> 
> Let's check bounds.
> 
> 
> >+/* Python RecordAuxiliary type.  */
> >+
> >+PyTypeObject recpy_aux_type = {
> >+  PyVarObject_HEAD_INIT (NULL, 0)
> 
> NULL is spelled nullptr.  More below.

Done.

> 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

[PATCH v3 06/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* py-record-btrace.c (recpy_bt_clear): New function.
	* py-record-btrace.h (recpy_bt_clear): New export.
	* py-record.c (recpy_clear): New function.
	(recpy_record_methods): Add clear().

testsuite/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.python/py-record-btrace.exp: Add tests for clear().

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* python.texi (gdb.Record): Document clear().
---
 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 333732f42f2..fa1c69a2283 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -3567,6 +3567,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 128dd14dd81..97649b206a5 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -815,6 +815,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 0022704883e..adb03c5eab4 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 481cd282ba3..9b46b2452bf 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 bf0fa18720e..7d45d6728a3 100644
--- a/gdb/testsuite/gdb.python/py-record-btrace.exp
+++ b/gdb/testsuite/gdb.python/py-record-btrace.exp
@@ -93,7 +93,11 @@ with_test_prefix "instruction " {
     }
     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.25.4

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 v3 06/12] python: Add clear() to gdb.Record.

[Metzger, Markus T]

Thanks, Felix,

> gdb/doc/python.texi                           |  5 +++++
> gdb/python/py-record-btrace.c                 | 13 +++++++++++++
> gdb/python/py-record-btrace.h                 |  3 +++
> gdb/python/py-record.c                        | 16 ++++++++++++++++
> gdb/testsuite/gdb.python/py-record-btrace.exp |  6 +++++-
> 5 files changed, 42 insertions(+), 1 deletion(-)

LGTM.  Maybe a python maintainer would like to review this, too?

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

[PATCH v3 07/12] 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.c (parse_xml_btrace_conf_pt): Add ptwrite to pt.conf.
	(btrace_conf_pt_attributes): Add ptwrite.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.texinfo (General Query Packets): Document Qbtrace-conf:pt:ptwrite.
	(Branch Trace Configuration Format): Document ptwrite in btrace-conf.

gdbserver/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* linux-low.cc (linux_process_target::read_btrace_conf): Add ptwrite.
	* server.cc (handle_btrace_conf_general_set): Handle pt:ptwrite.
	(supported_btrace_packets): Add Qbtrace-conf:pt:ptwrite.

gdbsupport/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace-common.h (btrace_config_pt): Add ptwrite.
---
 gdb/btrace.c                 |  7 ++++++-
 gdb/doc/gdb.texinfo          | 21 +++++++++++++++++++++
 gdb/features/btrace-conf.dtd |  1 +
 gdb/remote.c                 | 30 ++++++++++++++++++++++++++++++
 gdbserver/linux-low.cc       |  1 +
 gdbserver/server.cc          | 16 ++++++++++++++++
 gdbsupport/btrace-common.h   |  6 ++++++
 7 files changed, 81 insertions(+), 1 deletion(-)

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 9f117a5e90d..2c4f3f67c6b 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -2261,7 +2261,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;
@@ -2270,10 +2270,15 @@ parse_xml_btrace_conf_pt (struct gdb_xml_parser *parser,
   size = xml_find_attribute (attributes, "size");
   if (size != NULL)
     conf->pt.size = (unsigned int) *(ULONGEST *) size->value.get ();
+
+  ptwrite = xml_find_attribute (attributes, "ptwrite");
+  if (ptwrite != NULL)
+    conf->pt.ptwrite = (bool) *(ULONGEST *) ptwrite->value.get ();
 }
 
 static const struct gdb_xml_attribute btrace_conf_pt_attributes[] = {
   { "size", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { "ptwrite", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 5e4f8e73b7d..60c1340d31d 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -41906,6 +41906,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{-}
@@ -42217,6 +42222,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.
@@ -42717,6 +42725,18 @@ The ring buffer size has been set.
 A badly formed request or an error was encountered.
 @end table
 
+@item Qbtrace-conf:pt:ptwrite=@var{value}
+Control recording of ptwrite packets.  This was added 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
@@ -45359,6 +45379,7 @@ The formal DTD for the branch trace configuration format is given below:
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
 @end smallexample
 
 @include agentexpr.texi
diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
index 4b060bb408c..339ce4a4966 100644
--- a/gdb/features/btrace-conf.dtd
+++ b/gdb/features/btrace-conf.dtd
@@ -12,3 +12,4 @@
 
 <!ELEMENT pt	EMPTY>
 <!ATTLIST pt	size	CDATA	#IMPLIED>
+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
diff --git a/gdb/remote.c b/gdb/remote.c
index 22933eeaeec..554dbbf107b 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2168,6 +2168,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,
 
@@ -5328,6 +5331,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 },
@@ -14028,6 +14033,28 @@ remote_target::btrace_sync_conf (const btrace_config *conf)
 
       rs->btrace_config.pt.size = conf->pt.size;
     }
+
+  packet = &remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite];
+  if (packet_config_support (packet) == PACKET_ENABLE
+      && conf->pt.ptwrite != rs->btrace_config.pt.ptwrite)
+    {
+      pos = buf;
+      pos += xsnprintf (pos, endbuf - pos, "%s=%d", packet->name,
+                        conf->pt.ptwrite);
+
+      putpkt (buf);
+      getpkt (&rs->buf, 0);
+
+      if (packet_ok (buf, packet) == PACKET_ERROR)
+	{
+	  if (buf[0] == 'E' && buf[1] == '.')
+	    error (_("Failed to sync ptwrite config: %s"), buf + 2);
+	  else
+	    error (_("Failed to sync ptwrite config."));
+	}
+
+      rs->btrace_config.pt.ptwrite = conf->pt.ptwrite;
+    }
 }
 
 /* Read the current thread's btrace configuration from the target and
@@ -15230,6 +15257,9 @@ Show the maximum size of the address (in bits) in a memory packet."), NULL,
   add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_size],
        "Qbtrace-conf:pt:size", "btrace-conf-pt-size", 0);
 
+  add_packet_config_cmd (&remote_protocol_packets[PACKET_Qbtrace_conf_pt_ptwrite],
+       "Qbtrace-conf:pt:ptwrite", "btrace-conf-pt-ptwrite", 0);
+
   add_packet_config_cmd (&remote_protocol_packets[PACKET_vContSupported],
 			 "vContSupported", "verbose-resume-supported", 0);
 
diff --git a/gdbserver/linux-low.cc b/gdbserver/linux-low.cc
index 5c6191d941c..6554e9766ab 100644
--- a/gdbserver/linux-low.cc
+++ b/gdbserver/linux-low.cc
@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const btrace_target_info *tinfo,
 	case BTRACE_FORMAT_PT:
 	  buffer_xml_printf (buffer, "<pt");
 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
 	  buffer_xml_printf (buffer, "/>\n");
 	  break;
 	}
diff --git a/gdbserver/server.cc b/gdbserver/server.cc
index 32dcc05924e..b899f50db35 100644
--- a/gdbserver/server.cc
+++ b/gdbserver/server.cc
@@ -546,6 +546,21 @@ 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)
+    {
+      int ptwrite;
+      char *endp = NULL;
+
+      errno = 0;
+      ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);
+      if (endp == NULL || *endp != 0 || errno != 0 || ptwrite > 1)
+	{
+	  strcpy (own_buf, "E.Bad ptwrite value.");
+	  return -1;
+	}
+
+      current_btrace_conf.pt.ptwrite = (bool) ptwrite;
+    }
   else
     {
       strcpy (own_buf, "E.Bad Qbtrace configuration option.");
@@ -2189,6 +2204,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 26d26ec957f..ccdf2fc25af 100644
--- a/gdbsupport/btrace-common.h
+++ b/gdbsupport/btrace-common.h
@@ -117,6 +117,12 @@ struct btrace_config_pt
      This is unsigned int and not size_t since it is registered as
      control variable for "set record btrace pt buffer-size".  */
   unsigned int size;
+
+  /* Configuration bit for ptwrite packets.
+
+     If this is set, gdb will try to enable ptwrite packets if the OS
+     supports this.  */
+  bool ptwrite;
 };
 
 /* A branch tracing configuration.
-- 
2.25.4

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 v3 07/12] btrace, gdbserver: Add ptwrite to btrace_config_pt.

[Metzger, Markus T]

Thanks, Felix,

This patch looks good to me but I have a question about XML schema and a small nit.

>diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
>index 4b060bb408c..339ce4a4966 100644
>--- a/gdb/features/btrace-conf.dtd
>+++ b/gdb/features/btrace-conf.dtd
>@@ -12,3 +12,4 @@
>
> <!ELEMENT pt	EMPTY>
> <!ATTLIST pt	size	CDATA	#IMPLIED>
>+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>

I don't know if we can simply add new attributes.  Would we at least need to bump the version number?  Looking at git log, others have added new attributes in the past.  And they didn't bump the version.


>@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const
>btrace_target_info *tinfo,
> 	case BTRACE_FORMAT_PT:
> 	  buffer_xml_printf (buffer, "<pt");
> 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
>+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);

Would we need to guard that with a check whether GDB supports this new attribute?
Have you tried mixing an old GDB and a new gdbserver?


>diff --git a/gdbserver/server.cc b/gdbserver/server.cc
>index 32dcc05924e..b899f50db35 100644
>--- a/gdbserver/server.cc
>+++ b/gdbserver/server.cc
>@@ -546,6 +546,21 @@ 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)
>+    {
>+      int ptwrite;

Please declare below where it is initialized.

>+      char *endp = NULL;
>+
>+      errno = 0;
>+      ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);


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 v3 07/12] btrace, gdbserver: Add ptwrite to btrace_config_pt.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Donnerstag, 12. August 2021 13:08
> To: Willgerodt, Felix <felix.willgerodt@intel.com>; Simon Marchi
> <simon.marchi@polymtl.ca>; Pedro Alves <pedro@palves.net>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 07/12] btrace, gdbserver: Add ptwrite to
> btrace_config_pt.
> 
> Thanks, Felix,
> 
> This patch looks good to me but I have a question about XML schema and a
> small nit.
> 
> >diff --git a/gdb/features/btrace-conf.dtd b/gdb/features/btrace-conf.dtd
> >index 4b060bb408c..339ce4a4966 100644
> >--- a/gdb/features/btrace-conf.dtd
> >+++ b/gdb/features/btrace-conf.dtd
> >@@ -12,3 +12,4 @@
> >
> > <!ELEMENT pt	EMPTY>
> > <!ATTLIST pt	size	CDATA	#IMPLIED>
> >+<!ATTLIST pt	ptwrite	CDATA	#IMPLIED>
> 
> I don't know if we can simply add new attributes.  Would we at least need to
> bump the version number?  Looking at git log, others have added new
> attributes in the past.  And they didn't bump the version.

I also looked at git and didn't bump it because of those. I don't know what
impact the version numbers have. I was hoping for some feedback on
the mailing list in case it is needed.

> 
> 
> >@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const
> >btrace_target_info *tinfo,
> > 	case BTRACE_FORMAT_PT:
> > 	  buffer_xml_printf (buffer, "<pt");
> > 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
> >+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
> 
> Would we need to guard that with a check whether GDB supports this new
> attribute?
> Have you tried mixing an old GDB and a new gdbserver?

I don't see how we could guard here.
If I use master for GDB and master + ptwrite for gdbserver,
recording and the history commands work without error message.
That might just mean that there is no length check on GDB side.
But that won't be added to old GDBs anyway.

> 
> >diff --git a/gdbserver/server.cc b/gdbserver/server.cc
> >index 32dcc05924e..b899f50db35 100644
> >--- a/gdbserver/server.cc
> >+++ b/gdbserver/server.cc
> >@@ -546,6 +546,21 @@ 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)
> >+    {
> >+      int ptwrite;
> 
> Please declare below where it is initialized.

Done.

> >+      char *endp = NULL;
> >+
> >+      errno = 0;
> >+      ptwrite = strtoul (op + strlen ("pt:ptwrite="), &endp, 16);
> 
> 
> 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 v3 07/12] btrace, gdbserver: Add ptwrite to btrace_config_pt.

[Metzger, Markus T]

Hello Felix,

>> >@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const
>> >btrace_target_info *tinfo,
>> > 	case BTRACE_FORMAT_PT:
>> > 	  buffer_xml_printf (buffer, "<pt");
>> > 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
>> >+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
>>
>> Would we need to guard that with a check whether GDB supports this new
>> attribute?
>> Have you tried mixing an old GDB and a new gdbserver?
>
>I don't see how we could guard here.
>If I use master for GDB and master + ptwrite for gdbserver,
>recording and the history commands work without error message.
>That might just mean that there is no length check on GDB side.
>But that won't be added to old GDBs anyway.

I think the answer is that unknown attributes will get ignored so we should be fine.

Let's also check the other direction where GDB would be able to handle the new
ptwrite attribute but gdbserver doesn't provide it.

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 v3 07/12] btrace, gdbserver: Add ptwrite to btrace_config_pt.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 10. Mai 2022 15:59
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; Simon Marchi
> <simon.marchi@polymtl.ca>; Pedro Alves <pedro@palves.net>
> Subject: RE: [PATCH v3 07/12] btrace, gdbserver: Add ptwrite to
> btrace_config_pt.
> 
> Hello Felix,
> 
> >> >@@ -7096,6 +7096,7 @@ linux_process_target::read_btrace_conf (const
> >> >btrace_target_info *tinfo,
> >> > 	case BTRACE_FORMAT_PT:
> >> > 	  buffer_xml_printf (buffer, "<pt");
> >> > 	  buffer_xml_printf (buffer, " size=\"0x%x\"", conf->pt.size);
> >> >+	  buffer_xml_printf (buffer, " ptwrite=\"%d\"", conf->pt.ptwrite);
> >>
> >> Would we need to guard that with a check whether GDB supports this
> new
> >> attribute?
> >> Have you tried mixing an old GDB and a new gdbserver?
> >
> >I don't see how we could guard here.
> >If I use master for GDB and master + ptwrite for gdbserver,
> >recording and the history commands work without error message.
> >That might just mean that there is no length check on GDB side.
> >But that won't be added to old GDBs anyway.
> 
> I think the answer is that unknown attributes will get ignored so we should
> be fine.
> 
> Let's also check the other direction where GDB would be able to handle the
> new
> ptwrite attribute but gdbserver doesn't provide it.


Just fyi: I did that as well and didn't see any problems.

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 v3 08/12] btrace, linux: Enable ptwrite packets.

[Felix Willgerodt]

Enable ptwrite in the PT config, if it is supported by the kernel.

gdb/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* nat/linux-btrace.c (linux_supports_ptw): New function.
	(linux_enable_pt): Set attr.config if ptwrite is supported.
	* record-btrace.c (_initialize_record_btrace): Initialize ptwrite
	in record_btrace_conf.pt.
---
 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 324f7ef0407..5a48f397908 100644
--- a/gdb/nat/linux-btrace.c
+++ b/gdb/nat/linux-btrace.c
@@ -415,6 +415,29 @@ cpu_supports_bts (void)
     }
 }
 
+/* Check whether the linux target supports Intel Processor Trace PTWRITE.  */
+
+static bool
+linux_supports_ptwrite ()
+{
+  static const char filename[]
+      = "/sys/bus/event_source/devices/intel_pt/caps/ptwrite";
+  gdb_file_up file = gdb_fopen_cloexec (filename, "r");
+
+  if (file.get () == nullptr)
+    return false;
+
+  int status, found = fscanf (file.get (), "%d", &status);
+
+  if (found != 1)
+    {
+      warning (_("Failed to determine ptwrite support from %s."), filename);
+      return false;
+    }
+
+  return status == 1;
+}
+
 /* The perf_event_open syscall failed.  Try to print a helpful error
    message.  */
 
@@ -607,6 +630,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 3d0c344e8e6..99d962455ea 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -3312,4 +3312,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.25.4

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 v3 08/12] btrace, linux: Enable ptwrite packets.

[Metzger, Markus T]

Thanks, Felix,

> gdb/nat/linux-btrace.c | 29 +++++++++++++++++++++++++++++
> gdb/record-btrace.c    |  5 +++++
> 2 files changed, 34 insertions(+)

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

[PATCH v3 09/12] btrace, python: Enable ptwrite listener registration.

[Felix Willgerodt]

With this patch a default ptwrite listener is registered upon start of GDB.
It prints the plain ptwrite payload as hex.  The default listener can be
overwritten by registering a custom listener in python or by registering
"None", for no output at all.  Registering a listener function creates per
thread copies to allow unique internal states per thread.

gdb/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.c (ftrace_add_pt): Add call to
	apply_ext_lang_ptwrite_listener.
	* btrace.h (btrace_thread_info): New member ptw_listener.
	* data-directory/Makefile.in: Add ptwrite.py.
	* extension-priv.h (struct extension_language_ops): New member
	apply_ptwrite_listener.
	* extension.c (apply_ext_lang_ptwrite_listener): New function.
	* extension.h (apply_ext_lang_ptwrite_listener): New declaration.
	* guile/guile.c (guile_extension_ops): Add
	gdbscm_load_ptwrite_listener.
	* python/lib/gdb/ptwrite.py: New file.
	* python/py-record-btrace.c (recpy_initialize_listener,
	get_ptwrite_listener): New function.
	* python/py-record-btrace.h (recpy_initialize_listener): New
	declaration.
	* python/py-record.c (gdbpy_load_ptwrite_listener): New function.
	* python/python-internal.h (gdbpy_load_ptwrite_listener): New
	declaration.
	* python/python.c (python_extension_ops): New member
	gdbpy_load_ptwrite_listener.
---
 gdb/btrace.c                   |  3 ++
 gdb/btrace.h                   |  3 ++
 gdb/data-directory/Makefile.in |  1 +
 gdb/extension-priv.h           |  4 ++
 gdb/extension.c                | 21 +++++++++
 gdb/extension.h                |  3 ++
 gdb/guile/guile.c              |  1 +
 gdb/python/lib/gdb/ptwrite.py  | 84 ++++++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.c  | 67 +++++++++++++++++++++++++++
 gdb/python/py-record-btrace.h  |  6 +++
 gdb/python/python-internal.h   |  3 ++
 gdb/python/python.c            |  2 +
 12 files changed, 198 insertions(+)
 create mode 100644 gdb/python/lib/gdb/ptwrite.py

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2c4f3f67c6b..d66c3e21f11 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.  */
@@ -1311,6 +1312,8 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
   uint64_t offset;
   int status;
 
+  apply_ext_lang_ptwrite_listener (inferior_ptid);
+
   for (;;)
     {
       struct pt_insn insn;
diff --git a/gdb/btrace.h b/gdb/btrace.h
index e6694551c70..b15efcd147c 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -352,6 +352,9 @@ struct btrace_thread_info
      stepping through the execution history.  */
   std::vector<std::string> aux_data;
 
+  /* PyObject pointer to the ptwrite listener function.  */
+  void *ptw_listener = 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 888325f974e..c759920a343 100644
--- a/gdb/data-directory/Makefile.in
+++ b/gdb/data-directory/Makefile.in
@@ -74,6 +74,7 @@ PYTHON_FILE_LIST = \
 	gdb/frames.py \
 	gdb/printing.py \
 	gdb/prompt.py \
+	gdb/ptwrite.py \
 	gdb/types.py \
 	gdb/unwinder.py \
 	gdb/xmethod.py \
diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
index 77f23e0f911..36da4211738 100644
--- a/gdb/extension-priv.h
+++ b/gdb/extension-priv.h
@@ -183,6 +183,10 @@ 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 listener to the current thread.  */
+  enum ext_lang_bt_status (*apply_ptwrite_listener)
+    (const struct extension_language_defn *, ptid_t inferior_ptid);
+
   /* 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 27dce9befa0..826cb0285c3 100644
--- a/gdb/extension.c
+++ b/gdb/extension.c
@@ -550,6 +550,27 @@ apply_ext_lang_frame_filter (struct frame_info *frame,
   return EXT_LANG_BT_NO_FILTERS;
 }
 
+/* Used for registering the ptwrite listener to the current thread.  */
+
+enum ext_lang_bt_status
+apply_ext_lang_ptwrite_listener (ptid_t inferior_ptid)
+{
+  for (const struct extension_language_defn *extlang : extension_languages)
+    {
+      enum ext_lang_bt_status status;
+
+      if (extlang->ops == nullptr
+	  || extlang->ops->apply_ptwrite_listener == NULL)
+	continue;
+      status = extlang->ops->apply_ptwrite_listener (extlang, inferior_ptid);
+
+      if (status != EXT_LANG_BT_NO_FILTERS)
+	return status;
+    }
+
+  return EXT_LANG_BT_NO_FILTERS;
+}
+
 /* 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 56f57560de3..97562304ae4 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);
 
+enum ext_lang_bt_status apply_ext_lang_ptwrite_listener
+  (ptid_t inferior_ptid);
+
 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 a28dee37ed8..ef784885e87 100644
--- a/gdb/guile/guile.c
+++ b/gdb/guile/guile.c
@@ -124,6 +124,7 @@ static const struct extension_language_ops guile_extension_ops =
   gdbscm_apply_val_pretty_printer,
 
   NULL, /* gdbscm_apply_frame_filter, */
+  NULL, /* gdbscm_load_ptwrite_listener, */
 
   gdbscm_preserve_values,
 
diff --git a/gdb/python/lib/gdb/ptwrite.py b/gdb/python/lib/gdb/ptwrite.py
new file mode 100644
index 00000000000..f79463aefcf
--- /dev/null
+++ b/gdb/python/lib/gdb/ptwrite.py
@@ -0,0 +1,84 @@
+# Ptwrite utilities.
+# Copyright (C) 2020 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 listeners."""
+
+import gdb
+from copy import deepcopy
+
+
+def default_listener(payload, ip):
+    """Default listener that is active upon starting GDB."""
+    return "{:x}".format(payload)
+
+# This dict contains the per thread copies of the listener function and the
+# global template listener, from which the copies are created.
+_ptwrite_listener = {"global" : default_listener}
+
+
+def _update_listener_dict(thread_list):
+    """Helper function to update the listener dict.
+
+    Discards listener copies of threads that already exited and registers
+    copies of the listener for new threads."""
+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
+    lwp_list = [i.ptid[1] for i in thread_list]
+
+    # clean-up old listeners
+    for key in _ptwrite_listener.keys():
+      if key not in lwp_list and key != "global":
+        _ptwrite_listener.pop(key)
+
+    # Register listener for new threads
+    for key in lwp_list:
+        if key not in _ptwrite_listener.keys():
+            _ptwrite_listener[key] = deepcopy(_ptwrite_listener["global"])
+
+
+def _clear_traces(thread_list):
+    """Helper function to clear the trace of all threads."""
+    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_listener(listener):
+    """Register the ptwrite listener function."""
+    if listener is not None and not callable(listener):
+        raise TypeError("Listener must be callable!")
+
+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
+    _clear_traces(thread_list)
+
+    _ptwrite_listener.clear()
+    _ptwrite_listener["global"] = listener
+
+    _update_listener_dict(thread_list)
+
+
+def get_listener():
+    """Returns the listeners of the current thread."""
+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
+    _update_listener_dict(thread_list)
+
+    return _ptwrite_listener[gdb.selected_thread().ptid[1]]
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 97649b206a5..82f0e00d675 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -776,6 +776,73 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
   return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
 }
 
+/* Helper function returning the current ptwrite listener.  Returns nullptr
+   in case of errors.  */
+
+PyObject *
+get_ptwrite_listener ()
+{
+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
+
+  if (PyErr_Occurred ())
+  {
+    gdbpy_print_stack ();
+    return nullptr;
+  }
+
+  PyObject *default_ptw_listener = PyObject_CallMethod (module,	"get_listener",
+							NULL);
+
+  gdb_Py_DECREF (module);
+
+  if (PyErr_Occurred ())
+    gdbpy_print_stack ();
+
+  return default_ptw_listener;
+}
+
+/* Helper function to initialize the ptwrite listener.  Returns nullptr in
+   case of errors.  */
+
+PyObject *
+recpy_initialize_listener (ptid_t inferior_ptid)
+{
+  process_stratum_target *proc_target = current_inferior ()->process_target ();
+  struct thread_info * const tinfo = find_thread_ptid (proc_target, inferior_ptid);
+
+  tinfo->btrace.ptw_listener = get_ptwrite_listener ();
+
+  return (PyObject *) tinfo->btrace.ptw_listener;
+}
+
+/* Used for registering the default ptwrite listener to the current thread.  A
+   pointer to this function is stored in the python extension interface.  */
+
+enum ext_lang_bt_status
+gdbpy_load_ptwrite_listener (const struct extension_language_defn *extlang,
+			     ptid_t inferior_ptid)
+{
+  struct gdbarch *gdbarch = NULL;
+
+  if (!gdb_python_initialized)
+    return EXT_LANG_BT_NO_FILTERS;
+
+  try
+    {
+      gdbarch = target_gdbarch ();
+    }
+  catch (const gdb_exception &except)
+    {
+      return EXT_LANG_BT_NO_FILTERS;
+    }
+
+  gdbpy_enter enter_py (gdbarch, current_language);
+
+  recpy_initialize_listener (inferior_ptid);
+
+  return EXT_LANG_BT_OK;
+}
+
 /* 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 adb03c5eab4..36389e99ab8 100644
--- a/gdb/python/py-record-btrace.h
+++ b/gdb/python/py-record-btrace.h
@@ -91,4 +91,10 @@ 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 to initialize the ptwrite listener.  */
+extern PyObject *recpy_initialize_listener (ptid_t inferior_ptid);
+
+/* Helper function returning the current ptwrite listener.  */
+extern PyObject *get_ptwrite_listener ();
+
 #endif /* PYTHON_PY_RECORD_BTRACE_H */
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 690d2fb43c0..fcbd7a0b1b8 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -392,6 +392,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 enum ext_lang_bt_status gdbpy_load_ptwrite_listener
+  (const struct extension_language_defn *extlang,
+   ptid_t inferior_ptid);
 extern enum ext_lang_bt_status gdbpy_apply_frame_filter
   (const struct extension_language_defn *,
    struct frame_info *frame, frame_filter_flags flags,
diff --git a/gdb/python/python.c b/gdb/python/python.c
index e42cbc4fd5e..ad23532f9d8 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -174,6 +174,8 @@ const struct extension_language_ops python_extension_ops =
 
   gdbpy_apply_frame_filter,
 
+  gdbpy_load_ptwrite_listener,
+
   gdbpy_preserve_values,
 
   gdbpy_breakpoint_has_cond,
-- 
2.25.4

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 v3 09/12] btrace, python: Enable ptwrite listener registration.

[Metzger, Markus T]

Thanks, Felix,


>+  apply_ext_lang_ptwrite_listener (inferior_ptid);

Should this be called ptwrite_filter?


>+  /* PyObject pointer to the ptwrite listener function.  */
>+  void *ptw_listener = nullptr;
>+

The comment says that this is a PyObject.  Why is it declared void *?


>diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
>index 77f23e0f911..36da4211738 100644
>--- a/gdb/extension-priv.h
>+++ b/gdb/extension-priv.h
>@@ -183,6 +183,10 @@ 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 listener to the current thread.  */
>+  enum ext_lang_bt_status (*apply_ptwrite_listener)
>+    (const struct extension_language_defn *, ptid_t inferior_ptid);

There is a parameter name for the second but not for the first parameter.  Also, the name shadows a global variable.  Let's call it just 'ptid'.


>+/* Used for registering the ptwrite listener to the current thread.  */
>+
>+enum ext_lang_bt_status

This enum is specifically for frame filters.  We'd need to either generalize it or add our own for ptwrite filters.


>+apply_ext_lang_ptwrite_listener (ptid_t inferior_ptid)
>+{
>+  for (const struct extension_language_defn *extlang : extension_languages)
>+    {
>+      enum ext_lang_bt_status status;
>+
>+      if (extlang->ops == nullptr
>+	  || extlang->ops->apply_ptwrite_listener == NULL)
>+	continue;
>+      status = extlang->ops->apply_ptwrite_listener (extlang, inferior_ptid);
>+
>+      if (status != EXT_LANG_BT_NO_FILTERS)
>+	return status;

I would have inserted the empty line after the 'continue' from the if statement since obtaining the status and checking it belong together.

IIUC, this installs the ptwrite filter.  The code that actually calls it on a PTWRITE event is not part of this patch.  I think the installation of the filter and calling it should be inside one patch to make it clear how this works.


>+# Ptwrite utilities.
>+# Copyright (C) 2020 Free Software Foundation, Inc.

Year.


>+def default_listener(payload, ip):
>+    """Default listener that is active upon starting GDB."""
>+    return "{:x}".format(payload)
>+
>+# This dict contains the per thread copies of the listener function and the
>+# global template listener, from which the copies are created.
>+_ptwrite_listener = {"global" : default_listener}
>+
>+
>+def _update_listener_dict(thread_list):
>+    """Helper function to update the listener dict.
>+
>+    Discards listener copies of threads that already exited and registers
>+    copies of the listener for new threads."""
>+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
>+    lwp_list = [i.ptid[1] for i in thread_list]
>+
>+    # clean-up old listeners
>+    for key in _ptwrite_listener.keys():
>+      if key not in lwp_list and key != "global":
>+        _ptwrite_listener.pop(key)
>+
>+    # Register listener for new threads
>+    for key in lwp_list:
>+        if key not in _ptwrite_listener.keys():
>+            _ptwrite_listener[key] = deepcopy(_ptwrite_listener["global"])

So we can only update all at once?  In non-stop mode, I think we want to be able to update just the one for the current thread.

What's the purpose of this deepcopy?  For the default filter, this doesn't seem necessary.  For user-defined filters, this is initializing the per-thread filter from a template, correct?  Should 'global' be spelled 'template' in that case?

Should the template filter be part of the dict, at all?


>+def _clear_traces(thread_list):
>+    """Helper function to clear the trace of all threads."""

The comment doesn't match the code.  The function clears the recordings of 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()

Should there be a try around the above to ensure we switch back in case of exceptions?


>+def register_listener(listener):
>+    """Register the ptwrite listener function."""
>+    if listener is not None and not callable(listener):
>+        raise TypeError("Listener must be callable!")
>+
>+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
>+    _clear_traces(thread_list)

Please add a comment as to why we need to clear the recordings for all threads?


>+def get_listener():
>+    """Returns the listeners of the current thread."""

The comment says 'listeners' but there can be only one, correct?

>+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
>+    _update_listener_dict(thread_list)

Why do we need to update the filters for all threads?


>+/* Helper function returning the current ptwrite listener.  Returns nullptr
>+   in case of errors.  */
>+
>+PyObject *
>+get_ptwrite_listener ()
>+{
>+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
>+
>+  if (PyErr_Occurred ())
>+  {
>+    gdbpy_print_stack ();
>+    return nullptr;
>+  }
>+
>+  PyObject *default_ptw_listener = PyObject_CallMethod (module,
>	"get_listener",
>+							NULL);
>+
>+  gdb_Py_DECREF (module);
>+
>+  if (PyErr_Occurred ())
>+    gdbpy_print_stack ();
>+
>+  return default_ptw_listener;

This is not necessarily the default listener, correct?


>+/* Helper function to initialize the ptwrite listener.  Returns nullptr in
>+   case of errors.  */
>+
>+PyObject *
>+recpy_initialize_listener (ptid_t inferior_ptid)

The argument name shadows a global variable.  Let's just call it 'ptid'.


>+  process_stratum_target *proc_target = current_inferior ()->process_target ();

Any chance we can get here without having a current inferior?


>+  struct thread_info * const tinfo = find_thread_ptid (proc_target, inferior_ptid);
>+
>+  tinfo->btrace.ptw_listener = get_ptwrite_listener ();

Let's check tinfo.  Or pass it in as parameter.


>+  return (PyObject *) tinfo->btrace.ptw_listener;

It is called only once and that call ignores the result.


>+/* Used for registering the default ptwrite listener to the current thread.  A
>+   pointer to this function is stored in the python extension interface.  */
>+
>+enum ext_lang_bt_status
>+gdbpy_load_ptwrite_listener (const struct extension_language_defn *extlang,
>+			     ptid_t inferior_ptid)
>+{
>+  struct gdbarch *gdbarch = NULL;
>+
>+  if (!gdb_python_initialized)
>+    return EXT_LANG_BT_NO_FILTERS;
>+
>+  try
>+    {
>+      gdbarch = target_gdbarch ();

Is this a precaution or did you run into cases where target_gdbarch() throws?  Should the rest also be done inside a try block?


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 v3 09/12] btrace, python: Enable ptwrite listener registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Freitag, 13. August 2021 12:36
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 09/12] btrace, python: Enable ptwrite listener
> registration.
> 
> Thanks, Felix,
> 
> 
> >+  apply_ext_lang_ptwrite_listener (inferior_ptid);
> 
> Should this be called ptwrite_filter?
> 

No, that wasn't my intention. I can rename "listener" to "filter" if you
want, but that would affect the whole series and documentation.
Or are you just requesting the extension functions to be updated?

> 
> >+  /* PyObject pointer to the ptwrite listener function.  */
> >+  void *ptw_listener = nullptr;
> >+
> 
> The comment says that this is a PyObject.  Why is it declared void *?
>

Because I didn't want to include a python header for PyObject here.
Afaik, btrace.h should be kept free of those.

> 
> >diff --git a/gdb/extension-priv.h b/gdb/extension-priv.h
> >index 77f23e0f911..36da4211738 100644
> >--- a/gdb/extension-priv.h
> >+++ b/gdb/extension-priv.h
> >@@ -183,6 +183,10 @@ 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 listener to the current thread.  */
> >+  enum ext_lang_bt_status (*apply_ptwrite_listener)
> >+    (const struct extension_language_defn *, ptid_t inferior_ptid);
> 
> There is a parameter name for the second but not for the first parameter.
> Also, the name shadows a global variable.  Let's call it just 'ptid'.
> 

The first parameter never seems to be named in this file. Apart from one
occasion. That said, I think it might be only for formatting reasons, and we have
enough space here. So I added a name. I also changed it to ptid.

> 
> >+/* Used for registering the ptwrite listener to the current thread.  */
> >+
> >+enum ext_lang_bt_status
> 
> This enum is specifically for frame filters.  We'd need to either generalize it or
> add our own for ptwrite filters.
> 

Good catch. Since we are not checking it anyway and allow nullptr/None
as a valid listener I decided to make this return void.

> 
> >+apply_ext_lang_ptwrite_listener (ptid_t inferior_ptid)
> >+{
> >+  for (const struct extension_language_defn *extlang :
> extension_languages)
> >+    {
> >+      enum ext_lang_bt_status status;
> >+
> >+      if (extlang->ops == nullptr
> >+	  || extlang->ops->apply_ptwrite_listener == NULL)
> >+	continue;
> >+      status = extlang->ops->apply_ptwrite_listener (extlang, inferior_ptid);
> >+
> >+      if (status != EXT_LANG_BT_NO_FILTERS)
> >+	return status;
> 
> I would have inserted the empty line after the 'continue' from the if
> statement since obtaining the status and checking it belong together.
>

Done
 
> IIUC, this installs the ptwrite filter.  The code that actually calls it on a
> PTWRITE event is not part of this patch.  I think the installation of the filter
> and calling it should be inside one patch to make it clear how this works.
> 

Done.

> 
> >+# Ptwrite utilities.
> >+# Copyright (C) 2020 Free Software Foundation, Inc.
> 
> Year.
> 

Done

> 
> >+def default_listener(payload, ip):
> >+    """Default listener that is active upon starting GDB."""
> >+    return "{:x}".format(payload)
> >+
> >+# This dict contains the per thread copies of the listener function and the
> >+# global template listener, from which the copies are created.
> >+_ptwrite_listener = {"global" : default_listener}
> >+
> >+
> >+def _update_listener_dict(thread_list):
> >+    """Helper function to update the listener dict.
> >+
> >+    Discards listener copies of threads that already exited and registers
> >+    copies of the listener for new threads."""
> >+    # thread_list[x].ptid returns the tuple (pid, lwp, tid)
> >+    lwp_list = [i.ptid[1] for i in thread_list]
> >+
> >+    # clean-up old listeners
> >+    for key in _ptwrite_listener.keys():
> >+      if key not in lwp_list and key != "global":
> >+        _ptwrite_listener.pop(key)
> >+
> >+    # Register listener for new threads
> >+    for key in lwp_list:
> >+        if key not in _ptwrite_listener.keys():
> >+            _ptwrite_listener[key] = deepcopy(_ptwrite_listener["global"])
> 
> So we can only update all at once?  In non-stop mode, I think we want to be
> able to update just the one for the current thread.
> 
> What's the purpose of this deepcopy?  For the default filter, this doesn't
> seem necessary.  For user-defined filters, this is initializing the per-thread
> filter from a template, correct?  Should 'global' be spelled 'template' in that
> case?
> 
> Should the template filter be part of the dict, at all?
> 

We discussed this offline last year. To summarize:
We deepcopy to allow every thread to keep it's own internal state. We also
discussed the architecture repeatedly. We decided to update every thread
if we update the listener. I had a version some years ago that allowed to
specify which threads to update, but we decided against that.

I didn't change anything.

> 
> >+def _clear_traces(thread_list):
> >+    """Helper function to clear the trace of all threads."""
> 
> The comment doesn’t match the code.  The function clears the recordings of
> threads in THREAD_LIST.
> 

I updated the comment.


> 
> >+    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()
> 
> Should there be a try around the above to ensure we switch back in case of
> exceptions?
>

I am not sure. I don't think clear() can really throw. If switch throws, it
might be because of the thread_list being wrong. But we got the list from
gdb itself just before this call. It might also be that there is a problem with
switch(), but in that case trying to switch again seems dangerous.
 
> 
> >+def register_listener(listener):
> >+    """Register the ptwrite listener function."""
> >+    if listener is not None and not callable(listener):
> >+        raise TypeError("Listener must be callable!")
> >+
> >+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
> >+    _clear_traces(thread_list)
> 
> Please add a comment as to why we need to clear the recordings for all
> threads?
> 

Done.

> 
> >+def get_listener():
> >+    """Returns the listeners of the current thread."""
> 
> The comment says 'listeners' but there can be only one, correct?
> 
> >+    thread_list = gdb.Inferior.threads(gdb.selected_inferior())
> >+    _update_listener_dict(thread_list)
> 
> Why do we need to update the filters for all threads?
> 

Because we are registering a new copy of the new listener for every
thread.  An earlier (internal) version that I had allowed to register
different listeners per thread, but we decided against that.
This is needed to enable per-thread internal states, even if the user
can only register one listener globally.

> 
> >+/* Helper function returning the current ptwrite listener.  Returns nullptr
> >+   in case of errors.  */
> >+
> >+PyObject *
> >+get_ptwrite_listener ()
> >+{
> >+  PyObject *module = PyImport_ImportModule ("gdb.ptwrite");
> >+
> >+  if (PyErr_Occurred ())
> >+  {
> >+    gdbpy_print_stack ();
> >+    return nullptr;
> >+  }
> >+
> >+  PyObject *default_ptw_listener = PyObject_CallMethod (module,
> >	"get_listener",
> >+							NULL);
> >+
> >+  gdb_Py_DECREF (module);
> >+
> >+  if (PyErr_Occurred ())
> >+    gdbpy_print_stack ();
> >+
> >+  return default_ptw_listener;
> 
> This is not necessarily the default listener, correct?
>

Correct, I renamed it.


> 
> >+/* Helper function to initialize the ptwrite listener.  Returns nullptr in
> >+   case of errors.  */
> >+
> >+PyObject *
> >+recpy_initialize_listener (ptid_t inferior_ptid)
> 
> The argument name shadows a global variable.  Let's just call it 'ptid'.
>

I rewrote this a bit so this is no longer an issue.
 
> 
> >+  process_stratum_target *proc_target = current_inferior ()-
> >process_target ();
> 
> Any chance we can get here without having a current inferior?
>

I rewrote this to pass btinfo from btrace.c. That made this function
redundant.
 
> 
> >+  struct thread_info * const tinfo = find_thread_ptid (proc_target,
> inferior_ptid);
> >+
> >+  tinfo->btrace.ptw_listener = get_ptwrite_listener ();
> 
> Let's check tinfo.  Or pass it in as parameter.
>

See above, I rewrote this. In the new version I pass and check btinfo directly.

> 
> >+  return (PyObject *) tinfo->btrace.ptw_listener;
> 
> It is called only once and that call ignores the result.
>

I rewrote this. Since we allow nullptr/None to silence the listener I decided
to make everything return void.
 
> 
> >+/* Used for registering the default ptwrite listener to the current thread.
> A
> >+   pointer to this function is stored in the python extension interface.  */
> >+
> >+enum ext_lang_bt_status
> >+gdbpy_load_ptwrite_listener (const struct extension_language_defn
> *extlang,
> >+			     ptid_t inferior_ptid)
> >+{
> >+  struct gdbarch *gdbarch = NULL;
> >+
> >+  if (!gdb_python_initialized)
> >+    return EXT_LANG_BT_NO_FILTERS;
> >+
> >+  try
> >+    {
> >+      gdbarch = target_gdbarch ();
> 
> Is this a precaution or did you run into cases where target_gdbarch() throws?
> Should the rest also be done inside a try block?
>

I blindly copied this from py-framefilter.c:gdbpy_apply_frame_filter() years ago.
But looking at git and some recent changes to enter_py I don't think any of this
is necessary anymore. I rewrote this.

 
> 
> 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 v3 09/12] btrace, python: Enable ptwrite listener registration.

[Metzger, Markus T]

Hello Felix,

>> >+  apply_ext_lang_ptwrite_listener (inferior_ptid);
>>
>> Should this be called ptwrite_filter?
>>
>
>No, that wasn't my intention. I can rename "listener" to "filter" if you
>want, but that would affect the whole series and documentation.
>Or are you just requesting the extension functions to be updated?

This reminded me of frame filters or frame decorators.  Maybe
ptwrite_interpreter would be another term that is more specific than
ptwrite_listener.

Yes, we'd want to change this everywhere to make GDB's internals
match the terminology we use in the documentation.


>> >+  /* PyObject pointer to the ptwrite listener function.  */
>> >+  void *ptw_listener = nullptr;
>> >+
>>
>> The comment says that this is a PyObject.  Why is it declared void *?
>>
>
>Because I didn't want to include a python header for PyObject here.
>Afaik, btrace.h should be kept free of those.

Including python headers is better than void * declarations IMHO.

Maybe we can hide it all behind a callback function that is provided
by the python layer?

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 v3 09/12] btrace, python: Enable ptwrite listener registration.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Montag, 30. Mai 2022 16:55
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 09/12] btrace, python: Enable ptwrite listener
> registration.
> 
> Hello Felix,
> 
> >> >+  apply_ext_lang_ptwrite_listener (inferior_ptid);
> >>
> >> Should this be called ptwrite_filter?
> >>
> >
> >No, that wasn't my intention. I can rename "listener" to "filter" if you
> >want, but that would affect the whole series and documentation.
> >Or are you just requesting the extension functions to be updated?
> 
> This reminded me of frame filters or frame decorators.  Maybe
> ptwrite_interpreter would be another term that is more specific than
> ptwrite_listener.
> 
> Yes, we'd want to change this everywhere to make GDB's internals
> match the terminology we use in the documentation.

I personally don't have a strong preference and Eli didn't reject the listener
terminology. I was a bit against filter in the past, as we don't "hide" anything
for ptwrite (contrary to frame filters). But I think either of the three is fine.

Eli, are you fine with changing everything to the filter terminology, also in
the documentation? I just want agreement before making all the necessary
changes.

For convenience, the last patch with the documentation I posted:
https://sourceware.org/pipermail/gdb-patches/2022-May/188782.html

> 
> >> >+  /* PyObject pointer to the ptwrite listener function.  */
> >> >+  void *ptw_listener = nullptr;
> >> >+
> >>
> >> The comment says that this is a PyObject.  Why is it declared void *?
> >>
> >
> >Because I didn't want to include a python header for PyObject here.
> >Afaik, btrace.h should be kept free of those.
> 
> Including python headers is better than void * declarations IMHO.
> 
> Maybe we can hide it all behind a callback function that is provided
> by the python layer?

Sorry, I just realized that actually the comment is wrong. Other extension
languages could also provide a different type to call. In the end it is a
function pointer. I will fix the comment.

Regards,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

Re: [PATCH v3 09/12] btrace, python: Enable ptwrite listener registration.

[Eli Zaretskii]

> From: "Willgerodt, Felix" <felix.willgerodt@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, Eli Zaretskii
> 	<eliz@gnu.org>
> Date: Tue, 31 May 2022 11:26:23 +0000
> 
> Eli, are you fine with changing everything to the filter terminology, also in
> the documentation? I just want agreement before making all the necessary
> changes.

I don't object such a change, so feel free.

[PATCH v3 10/12] btrace, python: Enable calling the ptwrite listener.

[Felix Willgerodt]

Adding a new function to btinfo that allows to call the python ptwrite listener
from inside GDB.

gdb/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.h (btrace_thread_info): New member ptw_callback_fun.
	* python/py-record-btrace.c (recpy_call_listener): New function.
	(recpy_initialize_listener): Save recpy_call_listener in btinfo.
---
 gdb/btrace.h                  |  8 +++++
 gdb/python/py-record-btrace.c | 67 +++++++++++++++++++++++++++++++++++
 gdb/python/py-record-btrace.h |  5 +++
 3 files changed, 80 insertions(+)

diff --git a/gdb/btrace.h b/gdb/btrace.h
index b15efcd147c..7ca71277372 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -352,6 +352,14 @@ struct btrace_thread_info
      stepping through the execution history.  */
   std::vector<std::string> aux_data;
 
+  /* Function pointer to the ptwrite callback.  Returns the string returned
+     by the ptwrite listener function or nullptr if no string is supposed to
+     be printed.  */
+  gdb::unique_xmalloc_ptr<char> (*ptw_callback_fun) (
+						const uint64_t *payload,
+						const uint64_t *ip,
+						const void *ptw_listener);
+
   /* PyObject pointer to the ptwrite listener function.  */
   void *ptw_listener = nullptr;
 
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 82f0e00d675..6ca94168655 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -776,6 +776,72 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
   return btpy_list_new (tinfo, first, last, 1, &recpy_func_type);
 }
 
+/* Helper function that calls the ptwrite listener PTW_LISTENER with
+   PAYLOAD and IP as arguments.  Returns a pointer to the string that will
+   be printed or nullptr if nothing should be printed.  IP can be nullptr,
+   PAYLOAD must point to a valid integer.  */
+gdb::unique_xmalloc_ptr<char>
+recpy_call_listener (const uint64_t *payload, const uint64_t *ip,
+		     const void *ptw_listener)
+{
+  if ((PyObject *) ptw_listener == Py_None)
+    return nullptr;
+  else if ((PyObject *) ptw_listener == nullptr)
+    error (_("No valid ptwrite listener."));
+
+  /* As Python is started as a seperate thread, we need to
+     acquire the GIL to safely call the listener function.  */
+  PyGILState_STATE gstate = PyGILState_Ensure ();
+
+  PyObject *py_payload = PyLong_FromUnsignedLongLong (*payload);
+  PyObject *py_ip;
+
+  if (ip == nullptr)
+    {
+      py_ip = Py_None;
+      Py_INCREF (Py_None);
+    }
+  else
+    py_ip = PyLong_FromUnsignedLongLong (*ip);
+
+  PyObject *py_result = PyObject_CallFunctionObjArgs ((PyObject *) ptw_listener,
+						      py_payload, py_ip, NULL);
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      gdb_Py_DECREF (py_ip);
+      gdb_Py_DECREF (py_payload);
+      PyGILState_Release (gstate);
+      error (_("Error while executing Python code."));
+    }
+
+  gdb_Py_DECREF (py_ip);
+  gdb_Py_DECREF (py_payload);
+
+  if (py_result == Py_None)
+    {
+      gdb_Py_DECREF (py_result);
+      PyGILState_Release (gstate);
+      return nullptr;
+    }
+
+  gdb::unique_xmalloc_ptr<char> resultstring = gdbpy_obj_to_string (py_result);
+
+  if (PyErr_Occurred ())
+    {
+      gdbpy_print_stack ();
+      gdb_Py_DECREF (py_result);
+      PyGILState_Release (gstate);
+      error (_("Error while executing Python code."));
+    }
+
+  gdb_Py_DECREF (py_result);
+  PyGILState_Release (gstate);
+
+  return resultstring;
+}
+
 /* Helper function returning the current ptwrite listener.  Returns nullptr
    in case of errors.  */
 
@@ -810,6 +876,7 @@ recpy_initialize_listener (ptid_t inferior_ptid)
   process_stratum_target *proc_target = current_inferior ()->process_target ();
   struct thread_info * const tinfo = find_thread_ptid (proc_target, inferior_ptid);
 
+  tinfo->btrace.ptw_callback_fun = &recpy_call_listener;
   tinfo->btrace.ptw_listener = get_ptwrite_listener ();
 
   return (PyObject *) tinfo->btrace.ptw_listener;
diff --git a/gdb/python/py-record-btrace.h b/gdb/python/py-record-btrace.h
index 36389e99ab8..098d6701e97 100644
--- a/gdb/python/py-record-btrace.h
+++ b/gdb/python/py-record-btrace.h
@@ -97,4 +97,9 @@ extern PyObject *recpy_initialize_listener (ptid_t inferior_ptid);
 /* Helper function returning the current ptwrite listener.  */
 extern PyObject *get_ptwrite_listener ();
 
+/* Callback function for the ptwrite listener.  */
+extern gdb::unique_xmalloc_ptr<char>
+recpy_call_listener (const uint64_t *payload, const uint64_t *ip,
+                     const void *ptw_listener);
+
 #endif /* PYTHON_PY_RECORD_BTRACE_H */
-- 
2.25.4

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 v3 10/12] btrace, python: Enable calling the ptwrite listener.

[Metzger, Markus T]

Thanks, Felix,


>+  /* Function pointer to the ptwrite callback.  Returns the string returned
>+     by the ptwrite listener function or nullptr if no string is supposed to
>+     be printed.  */
>+  gdb::unique_xmalloc_ptr<char> (*ptw_callback_fun) (

Should the function return std::string?


>+						const uint64_t *payload,
>+						const uint64_t *ip,

Should we pass the actual values rather than const pointers to them?


>+						const void *ptw_listener);
>+
>   /* PyObject pointer to the ptwrite listener function.  */
>   void *ptw_listener = nullptr;

The callback and its context should be added in a single patch.  In that case, it's also OK to declare it void * since the callback is supposed to know. I assume this will be necessary if we wanted to support other extension languages that require different contexts.


>+  /* As Python is started as a seperate thread, we need to
>+     acquire the GIL to safely call the listener function.  */
>+  PyGILState_STATE gstate = PyGILState_Ensure ();
>+
>+  PyObject *py_payload = PyLong_FromUnsignedLongLong (*payload);
>+  PyObject *py_ip;
>+
>+  if (ip == nullptr)

I see.  We could define zero as invalid.

>+    {
>+      py_ip = Py_None;
>+      Py_INCREF (Py_None);
>+    }
>+  else
>+    py_ip = PyLong_FromUnsignedLongLong (*ip);
>+
>+  PyObject *py_result = PyObject_CallFunctionObjArgs ((PyObject *) ptw_listener,
>+						      py_payload, py_ip, NULL);

s/NULL/nullptr/


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 v3 10/12] btrace, python: Enable calling the ptwrite listener.

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Freitag, 13. August 2021 12:36
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 10/12] btrace, python: Enable calling the ptwrite
> listener.
> 
> Thanks, Felix,
> 
> 
> >+  /* Function pointer to the ptwrite callback.  Returns the string returned
> >+     by the ptwrite listener function or nullptr if no string is supposed to
> >+     be printed.  */
> >+  gdb::unique_xmalloc_ptr<char> (*ptw_callback_fun) (
> 
> Should the function return std::string?
> 

We use nullptr as a measure to check if the listener was disabled.
If we were to switch we couldn't distinguish between the listener returning
an empty string and the listener being None (disabled).

Currently, the difference between the two is that an empty string would
be printed as an empty newline in the history commands. A disabled listener
would not print an empty newline. It wouldn't print anything.

I am fine if you want me to change this to std::string and just print no empty
line in either case. I don't think printing an empty newline is very useful.


> 
> >+						const uint64_t *payload,
> >+						const uint64_t *ip,
> 
> Should we pass the actual values rather than const pointers to them?
>

We use nullptr, e.g. for IP to say that no IP is available, passing None
to the python code. In your comment below, you mentioned to use
0 as invalid. I am fine with that and changed it.

> 
> >+						const void *ptw_listener);
> >+
> >   /* PyObject pointer to the ptwrite listener function.  */
> >   void *ptw_listener = nullptr;
> 
> The callback and its context should be added in a single patch.  In that case,
> it's also OK to declare it void * since the callback is supposed to know. I
> assume this will be necessary if we wanted to support other extension
> languages that require different contexts.
> 
>

Done.
 
> >+  /* As Python is started as a seperate thread, we need to
> >+     acquire the GIL to safely call the listener function.  */
> >+  PyGILState_STATE gstate = PyGILState_Ensure ();
> >+
> >+  PyObject *py_payload = PyLong_FromUnsignedLongLong (*payload);
> >+  PyObject *py_ip;
> >+
> >+  if (ip == nullptr)
> 
> I see.  We could define zero as invalid.
>

See comment above. I changed it to not being a ptr and treating 0 as invalid.

> >+    {
> >+      py_ip = Py_None;
> >+      Py_INCREF (Py_None);
> >+    }
> >+  else
> >+    py_ip = PyLong_FromUnsignedLongLong (*ip);
> >+
> >+  PyObject *py_result = PyObject_CallFunctionObjArgs ((PyObject *)
> ptw_listener,
> >+						      py_payload, py_ip, NULL);
> 
> s/NULL/nullptr/
>

Done

> 
> 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 v3 10/12] btrace, python: Enable calling the ptwrite listener.

[Metzger, Markus T]

Hello Felix,

>> >+  /* Function pointer to the ptwrite callback.  Returns the string returned
>> >+     by the ptwrite listener function or nullptr if no string is supposed to
>> >+     be printed.  */
>> >+  gdb::unique_xmalloc_ptr<char> (*ptw_callback_fun) (
>>
>> Should the function return std::string?
>>
>
>We use nullptr as a measure to check if the listener was disabled.
>If we were to switch we couldn't distinguish between the listener returning
>an empty string and the listener being None (disabled).
>
>Currently, the difference between the two is that an empty string would
>be printed as an empty newline in the history commands. A disabled listener
>would not print an empty newline. It wouldn't print anything.
>
>I am fine if you want me to change this to std::string and just print no empty
>line in either case. I don't think printing an empty newline is very useful.

I agree.  Let's treat empty as no output, then.

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

[PATCH v3 11/12] gdb, testsuite, lib: Add libipt version check.

[Felix Willgerodt]

This adds a version test for libipt, which is needed by future commits.

gdb/testsuite/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* lib/gdb.exp (version_at_least): Add revision args.
	(tcl_version_at_least): Adjust call of version_at_least.
	(readelf_prints_pie): Same.
	(require_libipt_version): New function.
---
 gdb/testsuite/lib/gdb.exp | 69 +++++++++++++++++++++++++++++++++++----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 4bb2da31c1f..a060b1767e5 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -1313,12 +1313,18 @@ proc gdb_test { args } {
     return [gdb_test_multiple $command $message $user_code]
 }
 
-# Return 1 if version MAJOR.MINOR is at least AT_LEAST_MAJOR.AT_LEAST_MINOR.
-proc version_at_least { major minor at_least_major at_least_minor} {
+# Return 1 if version MAJOR.MINOR.REVISION is at least
+# AT_LEAST_MAJOR.AT_LEAST_MINOR.AT_LEAST_REVISION.
+proc version_at_least { major minor revision at_least_major at_least_minor \
+			at_least_revision } {
     if { $major > $at_least_major } {
         return 1
     } elseif { $major == $at_least_major \
-		   && $minor >= $at_least_minor } {
+		   && $minor > $at_least_minor } {
+        return 1
+    } elseif { $major == $at_least_major \
+		   && $minor == $at_least_minor \
+		   && $revision >= $at_least_revision } {
         return 1
     } else {
         return 0
@@ -1330,8 +1336,8 @@ proc tcl_version_at_least { major minor } {
     global tcl_version
     regexp {^([0-9]+)\.([0-9]+)$} $tcl_version \
 	dummy tcl_version_major tcl_version_minor
-    return [version_at_least $tcl_version_major $tcl_version_minor \
-		$major $minor]
+    return [version_at_least $tcl_version_major $tcl_version_minor 0 \
+		$major $minor 0]
 }
 
 if { [tcl_version_at_least 8 5] == 0 } {
@@ -3451,6 +3457,57 @@ gdb_caching_proc skip_btrace_pt_tests {
     return $skip_btrace_tests
 }
 
+# Run a test on the target to see if we have a minimum libipt version.
+# Return 0 if so, 1 if it does not.
+
+proc require_libipt_version { major minor revision } {
+    global srcdir subdir gdb_prompt inferior_exited_re
+
+    set me "libipt_version_tests"
+    if { ![istarget "i?86-*-*"] && ![istarget "x86_64-*-*"] } {
+        verbose "$me:  target does not support btrace, returning 1" 2
+        return 1
+    }
+
+    # Compile a test program.
+    set src { int main() { return 0; } }
+    if {![gdb_simple_compile $me $src executable]} {
+        return 1
+    }
+
+    # No error message, compilation succeeded so now run it via gdb.
+
+    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 actual_major ""
+    set actual_minor ""
+    set actual_revision ""
+    gdb_test_multiple "maint info btrace" "$me: maint info btrace" {
+	-re ".*Version: (\[0-9\]+)\.(\[0-9\]+)\.(\[0-9\]+).*$gdb_prompt $" {
+	    append actual_major $expect_out(1,string)
+	    append actual_minor $expect_out(2,string)
+	    append actual_revision $expect_out(3,string)
+	}
+	default {}
+    }
+
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me: Using version: $actual_major.$actual_minor.$actual_revision" 2
+    verbose "$me: Required minimum version: $major.$minor.$revision" 2
+
+    return [expr ![version_at_least $actual_major $actual_minor \
+		   $actual_revision $major $minor $revision]]
+}
+
 # Run a test on the target to see if it supports Aarch64 SVE hardware.
 # Return 0 if so, 1 if it does not.  Note this causes a restart of GDB.
 
@@ -6059,7 +6116,7 @@ proc readelf_prints_pie { } {
     # flag is printed by readelf, but we cannot reliably construct a PIE
     # executable if the multilib_flags dictate otherwise
     # (--target_board=unix/-no-pie/-fno-PIE).
-    return [version_at_least $major $minor 2 26]
+    return [version_at_least $major $minor 0 2 26 0]
 }
 
 # Return 1 if EXECUTABLE is a Position Independent Executable, 0 if it is not,
-- 
2.25.4

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 v3 11/12] gdb, testsuite, lib: Add libipt version check.

[Metzger, Markus T]

Thanks, Felix,

>-# Return 1 if version MAJOR.MINOR is at least
>AT_LEAST_MAJOR.AT_LEAST_MINOR.
>-proc version_at_least { major minor at_least_major at_least_minor} {
>+# Return 1 if version MAJOR.MINOR.REVISION is at least
>+# AT_LEAST_MAJOR.AT_LEAST_MINOR.AT_LEAST_REVISION.
>+proc version_at_least { major minor revision at_least_major at_least_minor \
>+			at_least_revision } {

In which case do we need to check for a particular patch version?  There shouldn't be any changes other than bug fixes between patch versions.


>+# Run a test on the target to see if we have a minimum libipt version.
>+# Return 0 if so, 1 if it does not.
>+
>+proc require_libipt_version { major minor revision } {

This is an indirect check for, I assume, ptwrite support.  Would it instead be possible to just try it?  E.g. compile a trivial program that contains a ptwrite and see if it runs and can be decoded without decode error.

This would cover compiler support for ptwrite as well as GDB support for PT in general and for ptwrite in particular.

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 v3 11/12] gdb, testsuite, lib: Add libipt version check.

[Willgerodt, Felix]

Hi,

Sorry for taking so long to get back to this. This time I will follow
through with this better. I will send the rest of my responses
for the other patches soon, but I have one question before doing
that.

See inline below.

Thanks,
Felix

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Freitag, 13. August 2021 12:36
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 11/12] gdb, testsuite, lib: Add libipt version check.
> 
> Thanks, Felix,
> 
> >-# Return 1 if version MAJOR.MINOR is at least
> >AT_LEAST_MAJOR.AT_LEAST_MINOR.
> >-proc version_at_least { major minor at_least_major at_least_minor} {
> >+# Return 1 if version MAJOR.MINOR.REVISION is at least
> >+# AT_LEAST_MAJOR.AT_LEAST_MINOR.AT_LEAST_REVISION.
> >+proc version_at_least { major minor revision at_least_major
> at_least_minor \
> >+			at_least_revision } {
> 
> In which case do we need to check for a particular patch version?  There
> shouldn't be any changes other than bug fixes between patch versions.
> 

I don't think it hurts if this can check for more than we need, it is a
generic function. But for our ULI tests, which I yet have to upstream,
we would actually check for 2.0.4 for UIRET.

> 
> >+# Run a test on the target to see if we have a minimum libipt version.
> >+# Return 0 if so, 1 if it does not.
> >+
> >+proc require_libipt_version { major minor revision } {
> 
> This is an indirect check for, I assume, ptwrite support.  Would it instead be
> possible to just try it?  E.g. compile a trivial program that contains a ptwrite
> and see if it runs and can be decoded without decode error.
> 
> This would cover compiler support for ptwrite as well as GDB support for PT
> in general and for ptwrite in particular.

I have tried implementing that, but I get no error (or other) indication in any
GDB command with libipt 1.6. I added ptwrite support to
"maintenance btrace packet-history" instead to check for ptwrite there.

Is that approach fine with you? It makes skip_btrace_ptw_tests() a bit more
complicated though.
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 v3 12/12] btrace: Extend ptwrite event decoding.

[Felix Willgerodt]

Call the ptwrite listener 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/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* btrace.c (handle_pt_insn_events): Handle ptev_ptwrite.

gdb/testsuite/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* gdb.btrace/ptwrite.c: New file.
	* gdb.btrace/ptwrite.exp: New file.
	* gdb.btrace/x86_64-ptwrite.S: New file.
	* lib/gdb.exp (skip_btrace_ptw_tests): New function.

gdb/doc/ChangeLog:
2021-06-14  Felix Willgerodt  <felix.willgerodt@intel.com>

	* python.texi (gdb.ptwrite): New documentation.
---
 gdb/NEWS                                  |   6 +
 gdb/btrace.c                              |  52 +++
 gdb/doc/python.texi                       | 134 ++++++
 gdb/testsuite/gdb.btrace/ptwrite.c        |  39 ++
 gdb/testsuite/gdb.btrace/ptwrite.exp      | 194 +++++++++
 gdb/testsuite/gdb.btrace/x86_64-ptwrite.S | 479 ++++++++++++++++++++++
 gdb/testsuite/lib/gdb.exp                 |  82 ++++
 7 files changed, 986 insertions(+)
 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 56743fc9aea..c920a5cb8ec 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,12 @@
 
 *** Changes since GDB 10
 
+* GDB now supports printing of ptwrite payloads from the Intel Processor
+  Trace during 'record instruction-history', 'record function-call-history',
+  all stepping commands and in  Python.  Printing is customizable via a
+  ptwrite listener function in  Python.  By default, the raw ptwrite
+  payload is printed for each ptwrite that is encountered.
+
 * The 'set disassembler-options' command now supports specifying options
   for the ARC target.
 
diff --git a/gdb/btrace.c b/gdb/btrace.c
index d66c3e21f11..d276de723c0 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1247,6 +1247,58 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 		   bfun->insn_offset - 1, offset);
 
 	  break;
+
+	case ptev_ptwrite:
+	  {
+	    uint64_t *ip = nullptr;
+	    gdb::unique_xmalloc_ptr<char> ptw_string = nullptr;
+	    struct btrace_insn ptw_insn;
+	    btrace_insn_flags flags;
+
+	    /* Lookup the ip if available.  */
+	    if (event.ip_suppressed == 0)
+	      ip = &event.variant.ptwrite.ip;
+	    else if (!btinfo->functions.empty ()
+		     && !btinfo->functions.back ().insn.empty ())
+	      ip = &btinfo->functions.back ().insn.back ().pc;
+
+	    if (btinfo->ptw_callback_fun != nullptr)
+	      ptw_string = btinfo->ptw_callback_fun (
+					  &event.variant.ptwrite.payload, ip,
+					  btinfo->ptw_listener);
+
+	    if (ptw_string == nullptr)
+	      break;
+
+	    btinfo->aux_data.emplace_back (ptw_string.get ());
+
+	    if (!btinfo->functions.empty ()
+		&& !btinfo->functions.back ().insn.empty ())
+	      flags = btinfo->functions.back ().insn.back ().flags;
+	    else
+	      flags = 0;
+
+	    /* Update insn list with ptw payload insn.  */
+	    ptw_insn.aux_data_index = btinfo->aux_data.size () - 1;
+	    ptw_insn.flags = flags;
+	    ptw_insn.iclass = BTRACE_INSN_AUX;
+	    ptw_insn.size = 0;
+
+	    if (ip != nullptr)
+	      bfun = ftrace_update_function (btinfo, *ip);
+	    else
+	      {
+		if (btinfo->functions.empty ())
+		  bfun = ftrace_new_function (btinfo, NULL, NULL);
+		else
+		  bfun = &btinfo->functions.back ();
+	      }
+
+	    bfun->flags |= BFUN_AUX_DECODED;
+	    ftrace_update_insns (bfun, ptw_insn);
+
+	    break;
+	  }
 	}
     }
 #endif /* defined (HAVE_PT_INSN_EVENT) */
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index fa1c69a2283..3fe5b80e01b 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -6110,6 +6110,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 listener registration.
 @end menu
 
 @node gdb.printing
@@ -6300,3 +6301,136 @@ 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} 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);
+@}
+
+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         0x000000000040074c <ptwrite64+16>:	ptwrite %rbx
+13         [42]
+14         0x0000000000400751 <ptwrite64+21>:	mov    -0x8(%rbp),%rbx
+(gdb) record function-call-history
+1       main
+2       ptwrite64
+                [42]
+3       main
+@end group
+@end smallexample
+
+The @code{gdb.ptwrite} module allows customizing the default output of ptwrite
+auxiliary information.  A custom Python function can be registered via
+@code{gdb.ptwrite.register_listener} as the ptwrite listener function.
+This function will be called with the ptwrite payload and PC as arguments
+during trace decoding.
+
+@findex gdb.ptwrite.register_listener
+@defun register_listener (@var{listener})
+Used to register the ptwrite listener.  The listener can be any callable
+object that accepts two arguments.  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_listener
+@defun get_listener ()
+Returns the currently active ptwrite listener function.
+@end defun
+
+@findex gdb.ptwrite.default_listener
+@defun default_listener (@var{payload}, @var{ip})
+The listener function active by default.
+@end defun
+
+@value{GDBN} creates a new copy of the listener function for each thread to
+allow for independent internal states.  There is no support for registering
+different listeners for different threads.  The listener 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_listener(object):
+...    def __init__(self):
+...        self.var = 0
+...    def __call__(self, payload, ip):
+...        if gdb.selected_thread().global_num == 1:
+...            self.var += 1
+...            return "counter: @{@}, ip: @{:#x@}".format(self.var, ip)
+...        else:
+...            return None
+...
+>>> import gdb.ptwrite
+>>> gdb.ptwrite.register_listener(my_listener())
+>>>
+(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]
+(gdb) info threads 
+* 1    Thread 0x7ffff7fd8740 (LWP 25796) "ptwrite_threads" task (arg=0x0)
+    at bin/ptwrite/ptwrite_threads.c:45
+  2    Thread 0x7ffff6eb8700 (LWP 25797) "ptwrite_threads" task (arg=0x0)
+    at bin/ptwrite/ptwrite_threads.c:45
+(gdb) thread 2
+[Switching to thread 2 (Thread 0x7ffff6eb8700 (LWP 25797))]
+#0  task (arg=0x0) at ptwrite_threads.c:45
+45	  return NULL;
+(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/ptwrite.c b/gdb/testsuite/gdb.btrace/ptwrite.c
new file mode 100644
index 00000000000..f5ead4e7d89
--- /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 <stdint.h>
+
+void
+ptwrite64 (uint64_t value)
+{
+  asm volatile ("PTWRITE %0;" : : "b" (value));
+}
+
+void
+ptwrite32 (uint32_t value)
+{
+  asm volatile ("PTWRITE %0;" : : "b" (value));
+}
+
+int
+main (void)
+{
+  ptwrite64 (0x42);
+  ptwrite32 (0x43);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.btrace/ptwrite.exp b/gdb/testsuite/gdb.btrace/ptwrite.exp
new file mode 100644
index 00000000000..2bbf0f41d3c
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/ptwrite.exp
@@ -0,0 +1,194 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2021 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+load_lib gdb-python.exp
+
+if { [skip_btrace_pt_tests] } {
+    unsupported "Target does not support record btrace pt."
+    return -1
+}
+
+if { [skip_btrace_ptw_tests] } {
+    unsupported "Hardware does not support ptwrite instructions."
+    return -1
+}
+
+# Test libipt version (must be >= 2.0.0).
+if {[require_libipt_version 2 0 0]} {
+    unsupported "Libipt doesn't support ptwrite decoding."
+    return -1
+}
+
+set opts {}
+
+if [info exists COMPILE] {
+    # make check RUNTESTFLAGS="gdb.btrace/ptwrite.exp COMPILE=1"
+    standard_testfile ptwrite.c
+    lappend opts debug
+} else {
+	if {[is_amd64_regs_target]} {
+		standard_testfile x86_64-ptwrite.S
+	} else {
+		unsupported "target architecture not supported"
+		return -1
+	}
+}
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile $opts] {
+    return -1
+}
+
+if { ![runto_main] } {
+    untested "failed to run to main"
+    return -1
+}
+
+# This needs to be after runto_main
+if { [skip_python_tests] } {
+    unsupported "Configuration doesn't support python scripting."
+    return -1
+}
+
+
+### 1. Default testrun
+
+# Setup recording
+gdb_test_no_output "set record instruction-history-size unlimited" "Default: set unlimited"
+gdb_test_no_output "record btrace pt" "Default: record btrace pt"
+gdb_test "next" ".*" "Default: first next"
+gdb_test "next" ".*" "Default: second next"
+
+# Test record instruction-history
+gdb_test "record instruction-history 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[42\\\]" \
+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[43\\\].*" \
+  ] "Default: record instruction-history 1"
+
+gdb_test "record instruction-history /a 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+.*" \
+  ] "Default: record instruction-history /a 1"
+
+# Test function call history
+gdb_test "record function-call-history 1,4" [multi_line \
+  "1\tmain" \
+  "2\tptwrite64" \
+  "\t\t\\\[42\\\]" \
+  "3\tmain" \
+  "4\tptwrite32" \
+  "\t\t\\\[43\\\]" \
+  ] "Default: record function-call-history 1,4"
+
+gdb_test "record function-call-history /a 1,4" [multi_line \
+  "1\tmain" \
+  "2\tptwrite64" \
+  "3\tmain" \
+  "4\tptwrite32" \
+  ] "Default: record function-call-history /a 1,4"
+
+# Test payload printing during stepping
+gdb_test "record goto 10" "No such instruction\."
+gdb_test "record goto 9" ".*ptwrite64.* at .*ptwrite.c:23.*"
+gdb_test "stepi" ".*\\\[42\\\].*"
+gdb_test "reverse-stepi" ".*\\\[42\\\].*"
+gdb_test "continue" ".*\\\[42\\\].*\\\[43\\\].*"
+gdb_test "reverse-continue" ".*\\\[43\\\].*\\\[42\\\].*"
+
+# Test auxiliary type in python
+gdb_test_multiline "auxiliary type in python" \
+  "python" "" \
+  "h = gdb.current_recording().instruction_history" "" \
+  "for insn in h: print(insn)" "" \
+  "end" [multi_line \
+  ".*RecordAuxiliary.*" \
+  ".*RecordAuxiliary.*" \
+  ]
+
+
+### 2. Test listener registration
+### 2.1 Custom listener
+
+gdb_test_multiline "Custom: register listener in python" \
+  "python" "" \
+  "def my_listener(payload, ip):" "" \
+  "    if  payload == 66:" "" \
+  "        return \"payload: {0}, ip: {1:#x}\".format(payload, ip)" "" \
+  "    else:" "" \
+  "        return None" "" \
+  "import gdb.ptwrite" "" \
+  "gdb.ptwrite.register_listener(my_listener)" "" \
+  "end" ""
+
+gdb_test "record instruction-history 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[payload: 66, ip: $hex\\\]" \
+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:.*" \
+  ] "Custom: record instruction-history 1"
+
+### 2.2 None as listener
+
+gdb_test_multiline "None: register listener in python" \
+  "python" "" \
+  "import gdb.ptwrite" "" \
+  "gdb.ptwrite.register_listener(None)" "" \
+  "end" ""
+
+gdb_test "record instruction-history 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:.*" \
+  "\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:.*" \
+  ] "None: record instruction-history 1"
+
+### 2.3 Lambdas as listener
+
+gdb_test_multiline "Lambdas: register listener in python" \
+  "python" "" \
+  "import gdb.ptwrite" "" \
+  "gdb.ptwrite.register_listener(lambda payload, ip: \"{}\".format(payload + 2))" "" \
+  "end" ""
+
+gdb_test "record instruction-history 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[68\\\]" \
+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[69\\\].*" \
+  ] "Lambdas: record instruction-history 1"
+
+### 2.4 Functors as listener
+
+gdb_test_multiline "Functors: register listener 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_listener(foobar())" "" \
+  "end" ""
+
+gdb_test "record instruction-history 1" [multi_line \
+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[1, 66\\\]" \
+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
+  "\[0-9\]+\t   \\\[2, 67\\\].*" \
+  ] "Functors: record instruction-history 1"
diff --git a/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S b/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
new file mode 100644
index 00000000000..0411870eca4
--- /dev/null
+++ b/gdb/testsuite/gdb.btrace/x86_64-ptwrite.S
@@ -0,0 +1,479 @@
+/* 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/>.
+
+
+   This file has been generated using gcc version 8.1.1 20180502:
+   gcc -S -dA -g ptwrite.c -o x86_64-ptwrite.S.  */
+
+	.file	"ptwrite.c"
+	.text
+.Ltext0:
+	.globl	ptwrite64
+	.type	ptwrite64, @function
+ptwrite64:
+.LFB0:
+	.file 1 "ptwrite.c"
+	# ptwrite.c: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
+	pushq	%rbx
+	.cfi_offset 3, -24
+	movq	%rdi, -16(%rbp)
+	# ptwrite.c:23:3
+	.loc 1 23 3
+	movq	-16(%rbp), %rax
+	movq	%rax, %rbx
+#APP
+# 23 "ptwrite.c" 1
+	PTWRITE %rbx;
+# 0 "" 2
+	# ptwrite.c:24:1
+	.loc 1 24 1
+#NO_APP
+	nop
+	popq	%rbx
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	ptwrite64, .-ptwrite64
+	.globl	ptwrite32
+	.type	ptwrite32, @function
+ptwrite32:
+.LFB1:
+	# 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
+	pushq	%rbx
+	.cfi_offset 3, -24
+	movl	%edi, -12(%rbp)
+	# ptwrite.c:29:3
+	.loc 1 29 3
+	movl	-12(%rbp), %eax
+	movl	%eax, %ebx
+#APP
+# 29 "ptwrite.c" 1
+	PTWRITE %ebx;
+# 0 "" 2
+	# ptwrite.c:30:1
+	.loc 1 30 1
+#NO_APP
+	nop
+	popq	%rbx
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	ptwrite32, .-ptwrite32
+	.globl	main
+	.type	main, @function
+main:
+.LFB2:
+	# 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:36:3
+	.loc 1 36 3
+	movl	$66, %edi
+	call	ptwrite64
+	# ptwrite.c:37:3
+	.loc 1 37 3
+	movl	$67, %edi
+	call	ptwrite32
+	# ptwrite.c:39:10
+	.loc 1 39 10
+	movl	$0, %eax
+	# ptwrite.c:40:1
+	.loc 1 40 1
+	popq	%rbp
+	.cfi_def_cfa 7, 8
+# SUCC: EXIT [always] 
+	ret
+	.cfi_endproc
+.LFE2:
+	.size	main, .-main
+.Letext0:
+	.file 2 "/usr/include/bits/types.h"
+	.file 3 "/usr/include/bits/stdint-uintn.h"
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.long	0x10f	# 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	.LASF13	# DW_AT_producer: "GNU C17 8.1.1 20180502 (Red Hat 8.1.1-1) -mtune=generic -march=x86-64 -g"
+	.byte	0xc	# DW_AT_language
+	.long	.LASF14	# DW_AT_name: "ptwrite.c"
+	.long	.LASF15	# 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	0x1	# DW_AT_byte_size
+	.byte	0x8	# DW_AT_encoding
+	.long	.LASF0	# DW_AT_name: "unsigned char"
+	.uleb128 0x2	# (DIE (0x34) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF1	# DW_AT_name: "short unsigned int"
+	.uleb128 0x2	# (DIE (0x3b) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF2	# DW_AT_name: "unsigned int"
+	.uleb128 0x2	# (DIE (0x42) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x7	# DW_AT_encoding
+	.long	.LASF3	# DW_AT_name: "long unsigned int"
+	.uleb128 0x2	# (DIE (0x49) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF4	# DW_AT_name: "signed char"
+	.uleb128 0x2	# (DIE (0x50) DW_TAG_base_type)
+	.byte	0x2	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF5	# DW_AT_name: "short int"
+	.uleb128 0x3	# (DIE (0x57) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x4	# (DIE (0x5e) DW_TAG_typedef)
+	.long	.LASF7	# DW_AT_name: "__uint32_t"
+	.byte	0x2	# DW_AT_decl_file (/usr/include/bits/types.h)
+	.byte	0x29	# DW_AT_decl_line
+	.byte	0x16	# DW_AT_decl_column
+	.long	0x3b	# DW_AT_type
+	.uleb128 0x2	# (DIE (0x6a) DW_TAG_base_type)
+	.byte	0x8	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.long	.LASF6	# DW_AT_name: "long int"
+	.uleb128 0x4	# (DIE (0x71) DW_TAG_typedef)
+	.long	.LASF8	# DW_AT_name: "__uint64_t"
+	.byte	0x2	# DW_AT_decl_file (/usr/include/bits/types.h)
+	.byte	0x2c	# DW_AT_decl_line
+	.byte	0x1b	# DW_AT_decl_column
+	.long	0x42	# DW_AT_type
+	.uleb128 0x2	# (DIE (0x7d) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF9	# DW_AT_name: "char"
+	.uleb128 0x4	# (DIE (0x84) DW_TAG_typedef)
+	.long	.LASF10	# DW_AT_name: "uint32_t"
+	.byte	0x3	# DW_AT_decl_file (/usr/include/bits/stdint-uintn.h)
+	.byte	0x1a	# DW_AT_decl_line
+	.byte	0x14	# DW_AT_decl_column
+	.long	0x5e	# DW_AT_type
+	.uleb128 0x4	# (DIE (0x90) DW_TAG_typedef)
+	.long	.LASF11	# DW_AT_name: "uint64_t"
+	.byte	0x3	# DW_AT_decl_file (/usr/include/bits/stdint-uintn.h)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0x14	# DW_AT_decl_column
+	.long	0x71	# DW_AT_type
+	.uleb128 0x5	# (DIE (0x9c) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF16	# 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	0x57	# DW_AT_type
+	.quad	.LFB2	# DW_AT_low_pc
+	.quad	.LFE2-.LFB2	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_tail_call_sites
+	.uleb128 0x6	# (DIE (0xba) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF17	# DW_AT_name: "ptwrite32"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB1	# DW_AT_low_pc
+	.quad	.LFE1-.LFB1	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.long	0xe8	# DW_AT_sibling
+	.uleb128 0x7	# (DIE (0xd8) DW_TAG_formal_parameter)
+	.long	.LASF12	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x1b	# DW_AT_decl_line
+	.byte	0x15	# DW_AT_decl_column
+	.long	0x84	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -28
+	.byte	0	# end of children of DIE 0xba
+	.uleb128 0x8	# (DIE (0xe8) DW_TAG_subprogram)
+			# DW_AT_external
+	.long	.LASF18	# DW_AT_name: "ptwrite64"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_decl_column
+			# DW_AT_prototyped
+	.quad	.LFB0	# DW_AT_low_pc
+	.quad	.LFE0-.LFB0	# DW_AT_high_pc
+	.uleb128 0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+			# DW_AT_GNU_all_call_sites
+	.uleb128 0x7	# (DIE (0x102) DW_TAG_formal_parameter)
+	.long	.LASF12	# DW_AT_name: "value"
+	.byte	0x1	# DW_AT_decl_file (ptwrite.c)
+	.byte	0x15	# DW_AT_decl_line
+	.byte	0x15	# DW_AT_decl_column
+	.long	0x90	# DW_AT_type
+	.uleb128 0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 -32
+	.byte	0	# end of children of DIE 0xe8
+	.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 0xe	# (DW_FORM_strp)
+	.byte	0
+	.byte	0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0
+	.byte	0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x16	# (TAG: DW_TAG_typedef)
+	.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)
+	.byte	0
+	.byte	0
+	.uleb128 0x5	# (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 0x6	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0
+	.byte	0
+	.uleb128 0x7	# (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 0x8	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x39	# (DW_AT_decl_column)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x7	# (DW_FORM_data8)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0x18	# (DW_FORM_exprloc)
+	.uleb128 0x2117	# (DW_AT_GNU_all_call_sites)
+	.uleb128 0x19	# (DW_FORM_flag_present)
+	.byte	0
+	.byte	0
+	.byte	0
+	.section	.debug_aranges,"",@progbits
+	.long	0x2c	# Length of Address Ranges Info
+	.value	0x2	# DWARF aranges version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x8	# Size of Address
+	.byte	0	# Size of Segment Descriptor
+	.value	0	# Pad to 16 byte boundary
+	.value	0
+	.quad	.Ltext0	# Address
+	.quad	.Letext0-.Ltext0	# Length
+	.quad	0
+	.quad	0
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.section	.debug_str,"MS",@progbits,1
+.LASF2:
+	.string	"unsigned int"
+.LASF7:
+	.string	"__uint32_t"
+.LASF15:
+	.string	"gdb/gdb/testsuite/gdb.btrace"
+.LASF14:
+	.string	"ptwrite.c"
+.LASF18:
+	.string	"ptwrite64"
+.LASF3:
+	.string	"long unsigned int"
+.LASF11:
+	.string	"uint64_t"
+.LASF0:
+	.string	"unsigned char"
+.LASF16:
+	.string	"main"
+.LASF10:
+	.string	"uint32_t"
+.LASF6:
+	.string	"long int"
+.LASF8:
+	.string	"__uint64_t"
+.LASF13:
+	.string	"GNU C17 8.1.1 20180502 (Red Hat 8.1.1-1) -mtune=generic -march=x86-64 -g"
+.LASF1:
+	.string	"short unsigned int"
+.LASF4:
+	.string	"signed char"
+.LASF12:
+	.string	"value"
+.LASF5:
+	.string	"short int"
+.LASF17:
+	.string	"ptwrite32"
+.LASF9:
+	.string	"char"
+	.ident	"GCC: (GNU) 8.1.1 20180502 (Red Hat 8.1.1-1)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index a060b1767e5..8ac16f83849 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -3457,6 +3457,88 @@ gdb_caching_proc skip_btrace_pt_tests {
     return $skip_btrace_tests
 }
 
+# Run a test on the target to see if it supports ptwrite instructions.
+# Return 0 if so, 1 if it does not.  Based on 'check_vmx_hw_available'
+# from the GCC testsuite.
+
+gdb_caching_proc skip_btrace_ptw_tests {
+    global srcdir subdir gdb_prompt inferior_exited_re
+    set me "skip_ptw_tests"
+
+    set src {
+	#include <stdbool.h>
+	#include <stdio.h>
+
+	bool
+	linux_supports_ptwrite ()
+	{
+	  static const char filename[]
+	      = "/sys/bus/event_source/devices/intel_pt/caps/ptwrite";
+	  FILE *file = fopen (filename, "r");
+
+	  if (file == NULL)
+	    return false;
+
+	  int status, found = fscanf (file, "%d", &status);
+	  fclose (file);
+
+	  if (found != 1)
+	    return false;
+
+	  return status == 1;
+	}
+
+	int
+	main ()
+	{
+	  if (linux_supports_ptwrite ())
+	    {
+	      asm volatile("PTWRITE %0;" : : "b"(0x42));
+	      return 1;
+	    };
+
+	  return 0;
+	}
+    }
+
+    if {![gdb_simple_compile $me $src executable]} {
+        return 1
+    }
+
+    # No error message, compilation succeeded so now run it via gdb.
+
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load "$obj"
+    gdb_run_cmd
+
+    gdb_expect {
+        -re ".*Illegal instruction.*${gdb_prompt} $" {
+            verbose -log "$me:  ptwrite support not detected."
+            set skip_ptw_tests 1
+        }
+        -re ".*$inferior_exited_re normally.*${gdb_prompt} $" {
+            verbose -log "$me:  ptwrite support not detected."
+            set skip_ptw_tests 1
+        }
+        -re ".*exited with code 01.*${gdb_prompt} $" {
+            verbose -log "$me:  ptwrite support detected."
+            set skip_ptw_tests 0
+        }
+        default {
+            warning "\n$me:  default case taken."
+            set skip_ptw_tests 1
+        }
+    }
+
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$me:  returning $skip_ptw_tests" 2
+    return $skip_ptw_tests
+}
+
 # Run a test on the target to see if we have a minimum libipt version.
 # Return 0 if so, 1 if it does not.
 
-- 
2.25.4

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 v3 12/12] btrace: Extend ptwrite event decoding.

[Eli Zaretskii]

> Date: Wed, 16 Jun 2021 09:42:05 +0200
> From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> 
> +@exdent Sample program:
> +@smallexample
> +@group
> +void
> +ptwrite64 (unsigned long long value)
> +@{
> +  __builtin_ia32_ptwrite64 (value);
> +@}
> +
> +int
> +main (void)
> +@{
> +  ptwrite64 (0x42);
> +  return 0; /* break here.  */
> +@}
> +@end group
> +@end smallexample

Here and elsewhere in the patch, you use @group..@end group in a
sub-optimal manner.  That directive is intended to allow TeX to break
long @example stretches into several parts that can be printed on
different pages.  That's because by default each @example will be
typeset entirely on a single page (leaving empty space before and
after as needed) -- TeX won't break any @example between pages.  So
the correct use of @group is to subdivide the @example into separate
independent groups and mark each one of them with @group.  For
example, the above could be rewritten as

  @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

That is, you want to have each function on one page, but there can be
a page break between the functions.

> +@smallexample
> +@group
> +(gdb) python-interactive
> +>>> class my_listener(object):
> +...    def __init__(self):
> +...        self.var = 0
> +...    def __call__(self, payload, ip):
> +...        if gdb.selected_thread().global_num == 1:
> +...            self.var += 1
> +...            return "counter: @{@}, ip: @{:#x@}".format(self.var, ip)
> +...        else:
> +...            return None
> +...
> +>>> import gdb.ptwrite
> +>>> gdb.ptwrite.register_listener(my_listener())
> +>>>
> +(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]
> +(gdb) info threads 
> +* 1    Thread 0x7ffff7fd8740 (LWP 25796) "ptwrite_threads" task (arg=0x0)
> +    at bin/ptwrite/ptwrite_threads.c:45
> +  2    Thread 0x7ffff6eb8700 (LWP 25797) "ptwrite_threads" task (arg=0x0)
> +    at bin/ptwrite/ptwrite_threads.c:45
> +(gdb) thread 2
> +[Switching to thread 2 (Thread 0x7ffff6eb8700 (LWP 25797))]
> +#0  task (arg=0x0) at ptwrite_threads.c:45
> +45	  return NULL;
> +(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

Likewise here: the Python code would be one @group, and then each GDB
command that follows, with its results, would be a separate @group.

Otherwise, the documentation part is OK.  Since the changes I request
are mechanical, you don't need to post the documentation part for an
additional review, provided that you make those changes in the final
commit.

Thanks.

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

[Willgerodt, Felix]

>Likewise here: the Python code would be one @group, and then each GDB
>command that follows, with its results, would be a separate @group.
>
>Otherwise, the documentation part is OK.  Since the changes I request
>are mechanical, you don't need to post the documentation part for an
>additional review, provided that you make those changes in the final
>commit.
>
>Thanks.

Thanks, I changed it in my local version as you proposed. I will only repost once
there have been more comments.

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 v3 12/12] btrace: Extend ptwrite event decoding.

[Metzger, Markus T]

Thanks, Felix,

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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


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

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


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

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

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

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

[Willgerodt, Felix]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Freitag, 13. August 2021 15:37
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v3 12/12] btrace: Extend ptwrite event decoding.
> 
> Thanks, Felix,
> 
> >+	case ptev_ptwrite:
> >+	  {
> >+	    uint64_t *ip = nullptr;
> >+	    gdb::unique_xmalloc_ptr<char> ptw_string = nullptr;
> >+	    struct btrace_insn ptw_insn;
> >+	    btrace_insn_flags flags;
> 
> We're meanwhile declaring variables where they are initialized and used.
> The rest of the code has not been updated but new code should follow this.
> 

Done.

> >+
> >+	    /* Lookup the ip if available.  */
> >+	    if (event.ip_suppressed == 0)
> >+	      ip = &event.variant.ptwrite.ip;
> >+	    else if (!btinfo->functions.empty ()
> >+		     && !btinfo->functions.back ().insn.empty ())
> >+	      ip = &btinfo->functions.back ().insn.back ().pc;
> 
> This isn't necessary; libipt will fill in the IP.  It suffices to check for
> EVENT.IP_SUPPRESSED.
>

Removed the "else if".

> >+
> >+	    if (btinfo->ptw_callback_fun != nullptr)
> 
> This is probably the first check we should do, i.e. if it is nullptr, break right at
> the beginning.
> 
> >+	      ptw_string = btinfo->ptw_callback_fun (
> >+					  &event.variant.ptwrite.payload, ip,
> >+					  btinfo->ptw_listener);
> 
> The indentation looks a bit odd.  You may break before '= btinfo->...'  if the
> line is getting too long, otherwise.

Done.

> >+
> >+	    if (ptw_string == nullptr)
> >+	      break;
> >+
> >+	    btinfo->aux_data.emplace_back (ptw_string.get ());
> >+
> >+	    if (!btinfo->functions.empty ()
> >+		&& !btinfo->functions.back ().insn.empty ())
> >+	      flags = btinfo->functions.back ().insn.back ().flags;
> >+	    else
> >+	      flags = 0;
> >+
> >+	    /* Update insn list with ptw payload insn.  */
> 
> We should probably start by memset()ing ptw_insn to all-zero.

Done.

> >+If an inferior uses the instruction, @value{GDBN} inserts the raw payload
> value
> 
> Maybe add 'by default' to stress that this behavior can be overwritten, which
> is documented below.

Done.

> >+load_lib gdb-python.exp
> >+
> >+if { [skip_btrace_pt_tests] } {
> >+    unsupported "Target does not support record btrace pt."
> >+    return -1
> >+}
> >+
> >+if { [skip_btrace_ptw_tests] } {
> >+    unsupported "Hardware does not support ptwrite instructions."
> >+    return -1
> >+}
> >+
> >+# Test libipt version (must be >= 2.0.0).
> >+if {[require_libipt_version 2 0 0]} {
> >+    unsupported "Libipt doesn't support ptwrite decoding."
> >+    return -1
> >+}
> 
> We shouldn't need to check the patch version.  See comments on a previous
> patch regarding feature checking.  We can probably combine it with
> skip_btrace_ptw_tests.

See other email.

> >+### 1. Default testrun
> >+
> >+# Setup recording
> >+gdb_test_no_output "set record instruction-history-size unlimited"
> "Default: set
> >unlimited"
> >+gdb_test_no_output "record btrace pt" "Default: record btrace pt"
> >+gdb_test "next" ".*" "Default: first next"
> >+gdb_test "next" ".*" "Default: second next"
> 
> You may use with_test_prefix to add the "Default: " prefix to the test
> output.  This groups tests and makes it a bit more readable IMHO.  For the
> above group, the first two wouldn't need any additional test name string.
> And if you combined the latter two into "next 2", it wouldn't either.

Done

> >+# Test payload printing during stepping
> >+gdb_test "record goto 10" "No such instruction\."
> >+gdb_test "record goto 9" ".*ptwrite64.* at .*ptwrite.c:23.*"
> >+gdb_test "stepi" ".*\\\[42\\\].*"
> >+gdb_test "reverse-stepi" ".*\\\[42\\\].*"
> >+gdb_test "continue" ".*\\\[42\\\].*\\\[43\\\].*"
> >+gdb_test "reverse-continue" ".*\\\[43\\\].*\\\[42\\\].*"
> 
> Between [42] and [43] we wouldn't get any other output we need to ignore,
> would we?  There's multi_line to capture multiple lines of output.

Done.

> >+# Test auxiliary type in python
> >+gdb_test_multiline "auxiliary type in python" \
> >+  "python" "" \
> >+  "h = gdb.current_recording().instruction_history" "" \
> >+  "for insn in h: print(insn)" "" \
> >+  "end" [multi_line \
> >+  ".*RecordAuxiliary.*" \
> >+  ".*RecordAuxiliary.*" \
> >+  ]
> 
> Could we inspect the auxiliary records to see if we get the correct strings?
> Also, it would be nice to print the surrounding instructions instead of ignoring
> all the output.
> 
> If we just printed the instruction_history, wouldn't we get output very similar
> to the CLI?

If we just print the history we only get RecordInstruction and RecordAuxiliary
Objects. But we can use decoded and data members of them to print something
similar. I have done that and added a couple more instructions to the regex.

> >+### 2. Test listener registration
> >+### 2.1 Custom listener
> >+
> >+gdb_test_multiline "Custom: register listener in python" \
> >+  "python" "" \
> >+  "def my_listener(payload, ip):" "" \
> >+  "    if  payload == 66:" "" \
> >+  "        return \"payload: {0}, ip: {1:#x}\".format(payload, ip)" "" \
> >+  "    else:" "" \
> >+  "        return None" "" \
> >+  "import gdb.ptwrite" "" \
> >+  "gdb.ptwrite.register_listener(my_listener)" "" \
> >+  "end" ""
> >+
> >+gdb_test "record instruction-history 1" [multi_line \
> >+  ".*\[0-9\]+\t   $hex <ptwrite64\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
> >+  "\[0-9\]+\t   \\\[payload: 66, ip: $hex\\\]" \
> >+  ".*\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:\tptwrite %\[a-z\]+" \
> >+  "\[0-9\]+\t   $hex <ptwrite32\\+\[0-9\]+>:.*" \
> 
> What output do we need to ignore here at the beginning of every other line?

We ignore the rest of the instructions, before, inbetween and after the two
PTWRITEs.

> 
> >+# Run a test on the target to see if it supports ptwrite instructions.
> >+# Return 0 if so, 1 if it does not.  Based on 'check_vmx_hw_available'
> >+# from the GCC testsuite.
> 
> See above comments.  I think we should simply record a single ptwrite inside
> main() and check that GDB can decode the trace without errors - and check
> 'info record' that we actually decoded something.

See other email.

> 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