[v2,4/6] btrace: Add support for interrupt events.

  Newer Intel CPUs support recording asynchronous events in the PT trace.
Libipt also recently added support for decoding these.

This patch adds support for interrupt events, based on the existing aux
infrastructure.  GDB can now display such events during the record
instruction-history and function-call-history commands.

Subsequent patches will add the rest of the events currently supported.
---
 gdb/btrace.c                                  | 95 ++++++++++++++++++-
 gdb/btrace.h                                  |  6 +-
 gdb/testsuite/gdb.btrace/event-tracing-gap.c  | 32 +++++++
 .../gdb.btrace/event-tracing-gap.exp          | 73 ++++++++++++++
 gdb/testsuite/gdb.btrace/event-tracing.exp    | 55 +++++++++++
 gdb/testsuite/gdb.btrace/null-deref.c         | 33 +++++++
 gdb/testsuite/lib/gdb.exp                     | 56 +++++++++++
 7 files changed, 346 insertions(+), 4 deletions(-)
 create mode 100644 gdb/testsuite/gdb.btrace/event-tracing-gap.c
 create mode 100644 gdb/testsuite/gdb.btrace/event-tracing-gap.exp
 create mode 100644 gdb/testsuite/gdb.btrace/event-tracing.exp
 create mode 100644 gdb/testsuite/gdb.btrace/null-deref.c
  

Hello Felix,

>@@ -572,6 +574,10 @@ ftrace_update_function (struct btrace_thread_info
>*btinfo, CORE_ADDR pc)
>   if (bfun->errcode != 0)
>     return ftrace_new_function (btinfo, mfun, fun);
>
>+  /* If there is no PC, we don't update anything.  */
>+  if (pc == 0)
>+    return bfun;

Would this interfere with, say, calling through a null function pointer?

It might be safer to use a separate flag to denote the absence of an IP
for events.  Once we introduced such a flag, we can also use it for ptwrite.


>+	case BTRACE_INSN_AUX:
>+	  /* An aux insn couldn't have switched the function.  But the
>+	     segment might not have had a symbol name resolved yet, as events
>+	     might not have an IP.  Use the current IP in that case and update
>+	     the name.  */
>+	  if (bfun->sym == nullptr && bfun->msym == nullptr)
>+	    {
>+	      bfun->sym = fun;
>+	      bfun->msym = mfun;
>+	    }

This needs a break at the end.


>+/* Check if the recording contains real instructions and not only auxiliary
>+   instructions since the last gap (or since the beginning).  */
>+
>+static bool
>+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info *btinfo)
>+{
>+  const std::vector<btrace_function> &functions = btinfo->functions;
>+
>+  for (auto rit = functions.crbegin (); rit != functions.crend (); ++rit)

Could we declare rit using a proper type?  We can do so before the loop.

>+    {
>+      if (rit->errcode != 0)
>+	return false;
>+      if ((rit->flags & BFUN_CONTAINS_NON_AUX) != 0)
>+	return true;
>+    }

Let's add an empty line to separate the two if statements more clearly.


>+# We should have 2 gaps and events for each breakpoint we hit.
>+# Note that due to the asynchronous nature of certain events, we use
>+# gdb_test_sequence and check only for events that we can control.
>+gdb_test_sequence "record function-call-history" "function-call-history" {
>+    "\[0-9\]+\tmain"
>+    "\\\[interrupt: vector = 0x1 \\\(#db\\\)(, ip = 0x\[0-9a-fA-F\]+)?\\\]"

Didn't we want to remove that one?  It is not really necessary for the test
and gdb_test_sequence should just skip it.


>+# Test the instruction-history.  Assembly can differ between compilers. To
>+# avoid creating a .S file for this test we just check the last two lines.
>+gdb_test "record instruction-history" [multi_line \
>+  "$decimal\t   $hex <main\\+$decimal>.*" \
>+  "$decimal\t     \\\[interrupt: vector = 0xe \\\(#pf\\\)(, cr2 = 0x0)?(, ip =
>$hex)?\\\]"
>+  ]

Wouldn't the trailing .* skip multiple lines in-between?  The comment says
that we want to check the last two lines.

I wonder if it really matters, though.  IMHO it would suffice to check that
we get the event.



>+/* To have a function-call-history.  */
>+int
>+call1 (int a)
>+{
>+  return a;
>+}

I don't think we really need that for the test.

We'll just have main in the function-call-history, but we cover
longer histories in the other test.  This test is checking that we
get the #pf event.


>+    set allow_event_trace_tests 0
>+    gdb_test_multiple "set record btrace pt event-tracing on" "$me:  first
>check" {
>+	-re -wrap "Event-tracing support was disabled at compile time." {

I think that text was changed in this patch.

>+	}
>+	-re -wrap "" {
>+	    set allow_event_trace_tests 1
>+	}
>+    }
>+
>+    if { $allow_event_trace_tests == 1 } {
>+	gdb_test_multiple "record btrace pt" "$me:  check OS support" {
>+	    -re -wrap "^" {
>+	    }
>+	    -re -wrap "" {
>+		verbose -log "$me:  Target doesn't support event tracing."

Let's double-check that one, too.


Mostly nits and simplifications.  OK with those addressed.
The rest of the series looks good to me, too.

Thanks,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 18. September 2024 15:22
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 4/6] btrace: Add support for interrupt events.
> 
> Hello Felix,
> 
> >@@ -572,6 +574,10 @@ ftrace_update_function (struct btrace_thread_info
> >*btinfo, CORE_ADDR pc)
> >   if (bfun->errcode != 0)
> >     return ftrace_new_function (btinfo, mfun, fun);
> >
> >+  /* If there is no PC, we don't update anything.  */
> >+  if (pc == 0)
> >+    return bfun;
> 
> Would this interfere with, say, calling through a null function pointer?

As far as I know in both C and C++ and most if not all languages calling a
null function pointer is undefined behaviour leading to a segfault.

> It might be safer to use a separate flag to denote the absence of an IP
> for events.  Once we introduced such a flag, we can also use it for ptwrite.

I am not sure where exactly you would see me add this. Just a parameter to
this function? Or something for aux_insns? Though I don't really see how
this enables a cleaner check in this function. It currently only checks the last
insn in the bfun, not the insn at PC that the caller actually wants to add to
a bfun. If we add a flag to aux_insns, we would need to extend the function
significantly to check both the last insn as well as the insn at PC.

And I don't really see what this function accomplishes if it get's called
with pc == 0. 

I was also thinking of just repeating this code (checking if we have some
bfun or not) in handle_pt_aux_insn(). Though I opted to try and share
as much code as possible.

> 
> >+	case BTRACE_INSN_AUX:
> >+	  /* An aux insn couldn't have switched the function.  But the
> >+	     segment might not have had a symbol name resolved yet, as events
> >+	     might not have an IP.  Use the current IP in that case and update
> >+	     the name.  */
> >+	  if (bfun->sym == nullptr && bfun->msym == nullptr)
> >+	    {
> >+	      bfun->sym = fun;
> >+	      bfun->msym = mfun;
> >+	    }
> 
> This needs a break at the end.

Thanks, I fixed this locally.

> 
> >+/* Check if the recording contains real instructions and not only auxiliary
> >+   instructions since the last gap (or since the beginning).  */
> >+
> >+static bool
> >+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info *btinfo)
> >+{
> >+  const std::vector<btrace_function> &functions = btinfo->functions;
> >+
> >+  for (auto rit = functions.crbegin (); rit != functions.crend (); ++rit)
> 
> Could we declare rit using a proper type?  We can do so before the loop.

We could, though auto is already accepted by upstream and this is a
somewhat reasonable usecase for it, as the type would be long:
std::vector<btrace_function>::const_reverse_iterator
But I will do it:

-  for (auto rit = functions.crbegin (); rit != functions.crend (); ++rit)
+  std::vector<btrace_function>::const_reverse_iterator rit;
+  for (rit = functions.crbegin (); rit != functions.crend (); ++rit)
     {

> >+    {
> >+      if (rit->errcode != 0)
> >+	return false;
> >+      if ((rit->flags & BFUN_CONTAINS_NON_AUX) != 0)
> >+	return true;
> >+    }
> 
> Let's add an empty line to separate the two if statements more clearly.

Done locally.

> 
> >+# We should have 2 gaps and events for each breakpoint we hit.
> >+# Note that due to the asynchronous nature of certain events, we use
> >+# gdb_test_sequence and check only for events that we can control.
> >+gdb_test_sequence "record function-call-history" "function-call-history" {
> >+    "\[0-9\]+\tmain"
> >+    "\\\[interrupt: vector = 0x1 \\\(#db\\\)(, ip = 0x\[0-9a-fA-F\]+)?\\\]"
> 
> Didn't we want to remove that one?  It is not really necessary for the test
> and gdb_test_sequence should just skip it.

Ok, I only removed it from the other testfile. I will remove it here as well.

> 
> >+# Test the instruction-history.  Assembly can differ between compilers. To
> >+# avoid creating a .S file for this test we just check the last two lines.
> >+gdb_test "record instruction-history" [multi_line \
> >+  "$decimal\t   $hex <main\\+$decimal>.*" \
> >+  "$decimal\t     \\\[interrupt: vector = 0xe \\\(#pf\\\)(, cr2 = 0x0)?(, ip =
> >$hex)?\\\]"
> >+  ]
> 
> Wouldn't the trailing .* skip multiple lines in-between?  The comment says
> that we want to check the last two lines.
> 
> I wonder if it really matters, though.  IMHO it would suffice to check that
> we get the event.

I added the .* to prevent any random events from failing this test.
Though yes, I can for sure only test the event regex. I will do that.

> 
> 
> >+/* To have a function-call-history.  */
> >+int
> >+call1 (int a)
> >+{
> >+  return a;
> >+}
> 
> I don't think we really need that for the test.
> 
> We'll just have main in the function-call-history, but we cover
> longer histories in the other test.  This test is checking that we
> get the #pf event.

I disagree, we use this testfile for all *.exp files. This helps
to test that events are in the right function.
We are not only testing #pf, we also test IRET later.

> 
> >+    set allow_event_trace_tests 0
> >+    gdb_test_multiple "set record btrace pt event-tracing on" "$me:  first
> >check" {
> >+	-re -wrap "Event-tracing support was disabled at compile time." {
> 
> I think that text was changed in this patch.

Right, thanks. I fixed this.

I also noticed that my message had a duplicate "Support" in it.
I fixed that as well.

> >+	}
> >+	-re -wrap "" {
> >+	    set allow_event_trace_tests 1
> >+	}
> >+    }
> >+
> >+    if { $allow_event_trace_tests == 1 } {
> >+	gdb_test_multiple "record btrace pt" "$me:  check OS support" {
> >+	    -re -wrap "^" {
> >+	    }
> >+	    -re -wrap "" {
> >+		verbose -log "$me:  Target doesn't support event tracing."
> 
> Let's double-check that one, too.

Done, I tested all combinations of gdb and gdbserver supporting and
not supporting event tracing.
On two different systems, one with and one without support.
I saw no issues.

> 
> Mostly nits and simplifications.  OK with those addressed.
> The rest of the series looks good to me, too.
> 
> Thanks,
> Markus.

Thanks, I think we need to agree on the nullptr story before
getting this merged or re-posted.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hello Felix,

>> >+  /* If there is no PC, we don't update anything.  */
>> >+  if (pc == 0)
>> >+    return bfun;
>>
>> Would this interfere with, say, calling through a null function pointer?
>
>As far as I know in both C and C++ and most if not all languages calling a
>null function pointer is undefined behaviour leading to a segfault.

Right, and we will get a segfault with RIP=0.  That's exactly my point that we
should not ignore PC=0.

There is (normally) no instruction at PC=0, so the trace would contain the
async branch for the call and the subsequent #pf when attempting to fetch
instructions.

We explicitly add the current PC to the end of the trace in btrace_add_pc(),
which is called from btrace_finalize_ftrace_pt().

There's a test case for this in gdb.btrace/segv.exp, but it doesn't check the
function-call-history or it would have noticed that we stopped switching to
a new bfun.


>> It might be safer to use a separate flag to denote the absence of an IP
>> for events.  Once we introduced such a flag, we can also use it for ptwrite.
>
>I am not sure where exactly you would see me add this. Just a parameter to
>this function?

Yes.

> Or something for aux_insns? Though I don't really see how
>this enables a cleaner check in this function. It currently only checks the last
>insn in the bfun, not the insn at PC that the caller actually wants to add to
>a bfun. If we add a flag to aux_insns, we would need to extend the function
>significantly to check both the last insn as well as the insn at PC.
>
>And I don't really see what this function accomplishes if it get's called
>with pc == 0.

It would add another bfun since the symbols change.


>> >+/* Check if the recording contains real instructions and not only auxiliary
>> >+   instructions since the last gap (or since the beginning).  */
>> >+
>> >+static bool
>> >+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info
>*btinfo)
>> >+{
>> >+  const std::vector<btrace_function> &functions = btinfo->functions;
>> >+
>> >+  for (auto rit = functions.crbegin (); rit != functions.crend (); ++rit)
>>
>> Could we declare rit using a proper type?  We can do so before the loop.
>
>We could, though auto is already accepted by upstream and this is a
>somewhat reasonable usecase for it, as the type would be long:
>std::vector<btrace_function>::const_reverse_iterator
>But I will do it:
>
>-  for (auto rit = functions.crbegin (); rit != functions.crend (); ++rit)
>+  std::vector<btrace_function>::const_reverse_iterator rit;
>+  for (rit = functions.crbegin (); rit != functions.crend (); ++rit)
>     {

Thanks.


>> >+/* To have a function-call-history.  */
>> >+int
>> >+call1 (int a)
>> >+{
>> >+  return a;
>> >+}
>>
>> I don't think we really need that for the test.
>>
>> We'll just have main in the function-call-history, but we cover
>> longer histories in the other test.  This test is checking that we
>> get the #pf event.
>
>I disagree, we use this testfile for all *.exp files. This helps
>to test that events are in the right function.
>We are not only testing #pf, we also test IRET later.

You can't iret from a #pf.  Well, you can, but you'd just get the #pf
again, unless you jumped.  And we're not covering any of that, here.
Also, breakpoints, as well as their corresponding IRETs, are covered
in the other test.


>> >+	}
>> >+	-re -wrap "" {
>> >+	    set allow_event_trace_tests 1
>> >+	}
>> >+    }
>> >+
>> >+    if { $allow_event_trace_tests == 1 } {
>> >+	gdb_test_multiple "record btrace pt" "$me:  check OS support" {
>> >+	    -re -wrap "^" {
>> >+	    }
>> >+	    -re -wrap "" {
>> >+		verbose -log "$me:  Target doesn't support event tracing."
>>
>> Let's double-check that one, too.
>
>Done, I tested all combinations of gdb and gdbserver supporting and
>not supporting event tracing.
>On two different systems, one with and one without support.
>I saw no issues.

Good, thanks.


>Thanks, I think we need to agree on the nullptr story before
>getting this merged or re-posted.

Let's discuss this here.  Once we agree, the changes should be straight-forward
and wouldn't need a v3.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> We explicitly add the current PC to the end of the trace in btrace_add_pc(),
> which is called from btrace_finalize_ftrace_pt().
> 
> There's a test case for this in gdb.btrace/segv.exp, but it doesn't check the
> function-call-history or it would have noticed that we stopped switching to
> a new bfun.

Ah ok, I wasn't aware of this. Thanks for pointing that out.

> 
> >> It might be safer to use a separate flag to denote the absence of an IP
> >> for events.  Once we introduced such a flag, we can also use it for ptwrite.
> >
> >I am not sure where exactly you would see me add this. Just a parameter to
> >this function?
> 
> Yes.

Ok, I have implemented this.

> >> >+/* To have a function-call-history.  */
> >> >+int
> >> >+call1 (int a)
> >> >+{
> >> >+  return a;
> >> >+}
> >>
> >> I don't think we really need that for the test.
> >>
> >> We'll just have main in the function-call-history, but we cover
> >> longer histories in the other test.  This test is checking that we
> >> get the #pf event.
> >
> >I disagree, we use this testfile for all *.exp files. This helps
> >to test that events are in the right function.
> >We are not only testing #pf, we also test IRET later.
> 
> You can't iret from a #pf.  Well, you can, but you'd just get the #pf
> again, unless you jumped.  And we're not covering any of that, here.
> Also, breakpoints, as well as their corresponding IRETs, are covered
> in the other test.

I was actually incorrect, we are using two different *.c files. Sorry that
was my fault, I will remove the function.

> 
> >Thanks, I think we need to agree on the nullptr story before
> >getting this merged or re-posted.
> 
> Let's discuss this here.  Once we agree, the changes should be straight-forward
> and wouldn't need a v3.
> 
> Thanks,
> Markus.

I updated some comments and added the flag.
Here is my new diff of btrace.c for this patch:

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 649e0ad59d7..85324ff0276 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -544,10 +544,12 @@ ftrace_new_gap (struct btrace_thread_info *btinfo, int errcode,
 
 /* Update the current function segment at the end of the trace in BTINFO with
    respect to the instruction at PC.  This may create new function segments.
-   Return the chronologically latest function segment, never NULL.  */
+   Return the chronologically latest function segment, never NULL.
+   If VALID_PC is false, skip analyzing whether the function has changed.  */
 
 static struct btrace_function *
-ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
+ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc,
+			const bool valid_pc = true)
 {
   struct minimal_symbol *mfun;
   struct symbol *fun;
@@ -572,6 +574,11 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
   if (bfun->errcode != 0)
     return ftrace_new_function (btinfo, mfun, fun);
 
+  /* If there is no valid PC, which can happen for events with a
+     suppressed IP, we can't do more than return the last bfun.  */
+  if (!valid_pc)
+    return bfun;
+
   /* Check the last instruction, if we have one.
      We do this check first, since it allows us to fill in the call stack
      links in addition to the normal flow links.  */
@@ -642,6 +649,18 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
 
 	    break;
 	  }
+
+	case BTRACE_INSN_AUX:
+	  /* An aux insn couldn't have switched the function.  But the
+	     segment might not have had a symbol name resolved yet, as events
+	     might not have an IP.  Use the current IP in that case and update
+	     the name.  */
+	  if (bfun->sym == nullptr && bfun->msym == nullptr)
+	    {
+	      bfun->sym = fun;
+	      bfun->msym = mfun;
+	    }
+	  break;
 	}
     }
 
@@ -668,6 +687,8 @@ ftrace_update_insns (struct btrace_function *bfun, const btrace_insn &insn)
 
   if (insn.iclass == BTRACE_INSN_AUX)
     bfun->flags |= BFUN_CONTAINS_AUX;
+  else
+    bfun->flags |= BFUN_CONTAINS_NON_AUX;
 
   if (record_debug > 1)
     ftrace_debug (bfun, "update insn");
@@ -1211,10 +1232,11 @@ pt_btrace_insn (const struct pt_insn &insn)
 
 static void
 handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
-		    CORE_ADDR ip)
+		    CORE_ADDR pc, const bool valid_pc)
 {
   btinfo->aux_data.emplace_back (std::move (aux_str));
-  struct btrace_function *bfun = ftrace_update_function (btinfo, ip);
+  struct btrace_function *bfun = ftrace_update_function (btinfo, pc,
+							 valid_pc);
 
   btrace_insn insn {{btinfo->aux_data.size () - 1}, 0,
 		    BTRACE_INSN_AUX, 0};
@@ -1222,8 +1244,47 @@ handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
   ftrace_update_insns (bfun, insn);
 }
 
+/* Check if the recording contains real instructions and not only auxiliary
+   instructions since the last gap (or since the beginning).  */
+
+static bool
+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info *btinfo)
+{
+  const std::vector<btrace_function> &functions = btinfo->functions;
+
+  std::vector<btrace_function>::const_reverse_iterator rit;
+  for (rit = functions.crbegin (); rit != functions.crend (); ++rit)
+    {
+      if (rit->errcode != 0)
+	return false;
+
+      if ((rit->flags & BFUN_CONTAINS_NON_AUX) != 0)
+	return true;
+    }
+
+  return false;
+}
 #endif /* defined (HAVE_PT_INSN_EVENT) */
 
+#if (LIBIPT_VERSION >= 0x201)
+/* Translate an interrupt vector to a mnemonic string as defined for x86.
+   Returns nullptr if there is none.  */
+
+static const char *
+decode_interrupt_vector (const uint8_t vector)
+{
+  static const char *mnemonic[]
+    = { "#de", "#db", nullptr, "#bp", "#of", "#br", "#ud", "#nm",
+	"#df", "#mf", "#ts", "#np", "#ss", "#gp", "#pf", nullptr,
+	"#mf", "#ac", "#mc", "#xm", "#ve", "#cp" };
+
+  if (vector < (sizeof (mnemonic) / sizeof (mnemonic[0])))
+    return mnemonic[vector];
+
+  return nullptr;
+}
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
+
 /* Handle instruction decode events (libipt-v2).  */
 
 static int
@@ -1236,6 +1297,8 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
     {
       struct pt_event event;
       uint64_t offset;
+      CORE_ADDR pc = 0;
+      bool valid_pc = false;
 
       status = pt_insn_event (decoder, &event, sizeof (event));
       if (status < 0)
@@ -1251,8 +1314,13 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	    if (event.status_update != 0)
 	      break;
 
+	    /* Only create a new gap if there are non-aux instructions in
+	       the trace since the last gap.  We could be at the beginning
+	       of the recording and could already have handled one or more
+	       events, like ptev_iret, that created aux insns.  In that
+	       case we don't want to create a gap or print a warning.  */
 	    if (event.variant.enabled.resumed == 0
-		&& !btinfo->functions.empty ())
+		&& ftrace_contains_non_aux_since_last_gap (btinfo))
 	      {
 		struct btrace_function *bfun
 		  = ftrace_new_gap (btinfo, BDE_PT_NON_CONTIGUOUS, gaps);
@@ -1282,7 +1350,6 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
 	case ptev_ptwrite:
 	  {
-	    uint64_t pc = 0;
 	    std::optional<std::string> ptw_string;
 
 	    /* Lookup the PC if available.  The event often doesn't provide
@@ -1316,6 +1383,8 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 
 	    if (pc == 0)
 	      warning (_("Failed to determine the PC for ptwrite."));
+	    else
+	      valid_pc = true;
 
 	    if (btinfo->ptw_callback_fun != nullptr)
 	      ptw_string
@@ -1328,11 +1397,40 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	    if (!ptw_string.has_value ())
 	      *ptw_string = hex_string (event.variant.ptwrite.payload);
 
-	    handle_pt_aux_insn (btinfo, *ptw_string, pc);
+	    handle_pt_aux_insn (btinfo, *ptw_string, pc, valid_pc);
 
 	    break;
 	  }
 #endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
+
+#if (LIBIPT_VERSION >= 0x201)
+	case ptev_interrupt:
+	  {
+	    std::string aux_string = std::string (_("interrupt: vector = "))
+	      + hex_string (event.variant.interrupt.vector);
+
+	    const char *decoded
+	      = decode_interrupt_vector (event.variant.interrupt.vector);
+	    if (decoded != nullptr)
+	      aux_string += std::string (" (") + decoded + ")";
+
+	    if (event.variant.interrupt.has_cr2 != 0)
+	      {
+		aux_string += std::string (", cr2 = ")
+		  + hex_string (event.variant.interrupt.cr2);
+	      }
+
+	    if (event.ip_suppressed == 0)
+	      {
+		pc = event.variant.interrupt.ip;
+		valid_pc = true;
+		aux_string += std::string (", ip = ") + hex_string (pc);
+	      }
+
+	    handle_pt_aux_insn (btinfo, aux_string, pc, valid_pc);
+	    break;
+	  }
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
 	}
     }
 #endif /* defined (HAVE_PT_INSN_EVENT) */


The subsequent patches have the same mechanical changes applied, setting valid_pc.
And I fixed the rest of the nits you had locally. Is this ok?

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hello Felix,

> /* Update the current function segment at the end of the trace in BTINFO with
>    respect to the instruction at PC.  This may create new function segments.
>-   Return the chronologically latest function segment, never NULL.  */
>+   Return the chronologically latest function segment, never NULL.
>+   If VALID_PC is false, skip analyzing whether the function has changed.  */
>
> static struct btrace_function *
>-ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
>+ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc,
>+			const bool valid_pc = true)

I would go without the default.  If you want to simplify calls, we could add two
overloads:

ftrace_update_function (btrace_thread_info *)
ftrace_update_function (btrace_thread_info *, CORE_ADDR)

This would make it clearer that one is w/o a PC and one is with.

Or we make the PC argument std::optional<CORE_ADDR>.  This probably fits
best how we process events.

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Montag, 23. September 2024 15:45
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 4/6] btrace: Add support for interrupt events.
> 
> Hello Felix,
> 
> > /* Update the current function segment at the end of the trace in BTINFO
> with
> >    respect to the instruction at PC.  This may create new function segments.
> >-   Return the chronologically latest function segment, never NULL.  */
> >+   Return the chronologically latest function segment, never NULL.
> >+   If VALID_PC is false, skip analyzing whether the function has changed.  */
> >
> > static struct btrace_function *
> >-ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
> >+ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc,
> >+			const bool valid_pc = true)
> 
> I would go without the default.  If you want to simplify calls, we could add two
> overloads:
> 
> ftrace_update_function (btrace_thread_info *)
> ftrace_update_function (btrace_thread_info *, CORE_ADDR)
> 
> This would make it clearer that one is w/o a PC and one is with.
> 
> Or we make the PC argument std::optional<CORE_ADDR>.  This probably fits
> best how we process events.
> 
> Regards,
> Markus.

I also had thought about optional previously and prefer it as well.
Here are the new changes:


diff --git a/gdb/btrace.c b/gdb/btrace.c
index 649e0ad59d7..3cf386d5a75 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -547,31 +547,39 @@ ftrace_new_gap (struct btrace_thread_info *btinfo, int errcode,
    Return the chronologically latest function segment, never NULL.  */
 
 static struct btrace_function *
-ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
+ftrace_update_function (struct btrace_thread_info *btinfo,
+			std::optional<CORE_ADDR> pc)
 {
-  struct minimal_symbol *mfun;
-  struct symbol *fun;
-  struct btrace_function *bfun;
+  struct minimal_symbol *mfun = nullptr;
+  struct symbol *fun = nullptr;
 
   /* Try to determine the function we're in.  We use both types of symbols
      to avoid surprises when we sometimes get a full symbol and sometimes
      only a minimal symbol.  */
-  fun = find_pc_function (pc);
-  bound_minimal_symbol bmfun = lookup_minimal_symbol_by_pc (pc);
-  mfun = bmfun.minsym;
+  if (pc.has_value ())
+    {
+      fun = find_pc_function (*pc);
+      bound_minimal_symbol bmfun = lookup_minimal_symbol_by_pc (*pc);
+      mfun = bmfun.minsym;
 
-  if (fun == NULL && mfun == NULL)
-    DEBUG_FTRACE ("no symbol at %s", core_addr_to_string_nz (pc));
+      if (fun == nullptr && mfun == nullptr)
+	DEBUG_FTRACE ("no symbol at %s", core_addr_to_string_nz (*pc));
+    }
 
   /* If we didn't have a function, we create one.  */
   if (btinfo->functions.empty ())
     return ftrace_new_function (btinfo, mfun, fun);
 
   /* If we had a gap before, we create a function.  */
-  bfun = &btinfo->functions.back ();
+  btrace_function *bfun = &btinfo->functions.back ();
   if (bfun->errcode != 0)
     return ftrace_new_function (btinfo, mfun, fun);
 
+  /* If there is no valid PC, which can happen for events with a
+     suppressed IP, we can't do more than return the last bfun.  */
+  if (!pc.has_value ())
+    return bfun;
+
   /* Check the last instruction, if we have one.
      We do this check first, since it allows us to fill in the call stack
      links in addition to the normal flow links.  */
@@ -605,7 +613,7 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
 
 	case BTRACE_INSN_CALL:
 	  /* Ignore calls to the next instruction.  They are used for PIC.  */
-	  if (last->pc + last->size == pc)
+	  if (last->pc + last->size == *pc)
 	    break;
 
 	  return ftrace_new_call (btinfo, mfun, fun);
@@ -614,10 +622,10 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
 	  {
 	    CORE_ADDR start;
 
-	    start = get_pc_function_start (pc);
+	    start = get_pc_function_start (*pc);
 
 	    /* A jump to the start of a function is (typically) a tail call.  */
-	    if (start == pc)
+	    if (start == *pc)
 	      return ftrace_new_tailcall (btinfo, mfun, fun);
 
 	    /* Some versions of _Unwind_RaiseException use an indirect
@@ -642,6 +650,18 @@ ftrace_update_function (struct btrace_thread_info *btinfo, CORE_ADDR pc)
 
 	    break;
 	  }
+
+	case BTRACE_INSN_AUX:
+	  /* An aux insn couldn't have switched the function.  But the
+	     segment might not have had a symbol name resolved yet, as events
+	     might not have an IP.  Use the current IP in that case and update
+	     the name.  */
+	  if (bfun->sym == nullptr && bfun->msym == nullptr)
+	    {
+	      bfun->sym = fun;
+	      bfun->msym = mfun;
+	    }
+	  break;
 	}
     }
 
@@ -668,6 +688,8 @@ ftrace_update_insns (struct btrace_function *bfun, const btrace_insn &insn)
 
   if (insn.iclass == BTRACE_INSN_AUX)
     bfun->flags |= BFUN_CONTAINS_AUX;
+  else
+    bfun->flags |= BFUN_CONTAINS_NON_AUX;
 
   if (record_debug > 1)
     ftrace_debug (bfun, "update insn");
@@ -1101,7 +1123,8 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
 	      break;
 	    }
 
-	  bfun = ftrace_update_function (btinfo, pc);
+	  bfun = ftrace_update_function (btinfo,
+					 std::make_optional<CORE_ADDR> (pc));
 
 	  /* Maintain the function level offset.
 	     For all but the last block, we do it here.  */
@@ -1211,10 +1234,10 @@ pt_btrace_insn (const struct pt_insn &insn)
 
 static void
 handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
-		    CORE_ADDR ip)
+		    std::optional<CORE_ADDR> pc)
 {
   btinfo->aux_data.emplace_back (std::move (aux_str));
-  struct btrace_function *bfun = ftrace_update_function (btinfo, ip);
+  struct btrace_function *bfun = ftrace_update_function (btinfo, pc);
 
   btrace_insn insn {{btinfo->aux_data.size () - 1}, 0,
 		    BTRACE_INSN_AUX, 0};
@@ -1222,8 +1245,47 @@ handle_pt_aux_insn (btrace_thread_info *btinfo, std::string &aux_str,
   ftrace_update_insns (bfun, insn);
 }
 
+/* Check if the recording contains real instructions and not only auxiliary
+   instructions since the last gap (or since the beginning).  */
+
+static bool
+ftrace_contains_non_aux_since_last_gap (const btrace_thread_info *btinfo)
+{
+  const std::vector<btrace_function> &functions = btinfo->functions;
+
+  std::vector<btrace_function>::const_reverse_iterator rit;
+  for (rit = functions.crbegin (); rit != functions.crend (); ++rit)
+    {
+      if (rit->errcode != 0)
+	return false;
+
+      if ((rit->flags & BFUN_CONTAINS_NON_AUX) != 0)
+	return true;
+    }
+
+  return false;
+}
 #endif /* defined (HAVE_PT_INSN_EVENT) */
 
+#if (LIBIPT_VERSION >= 0x201)
+/* Translate an interrupt vector to a mnemonic string as defined for x86.
+   Returns nullptr if there is none.  */
+
+static const char *
+decode_interrupt_vector (const uint8_t vector)
+{
+  static const char *mnemonic[]
+    = { "#de", "#db", nullptr, "#bp", "#of", "#br", "#ud", "#nm",
+	"#df", "#mf", "#ts", "#np", "#ss", "#gp", "#pf", nullptr,
+	"#mf", "#ac", "#mc", "#xm", "#ve", "#cp" };
+
+  if (vector < (sizeof (mnemonic) / sizeof (mnemonic[0])))
+    return mnemonic[vector];
+
+  return nullptr;
+}
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
+
 /* Handle instruction decode events (libipt-v2).  */
 
 static int
@@ -1236,6 +1298,7 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
     {
       struct pt_event event;
       uint64_t offset;
+      std::optional<CORE_ADDR> pc;
 
       status = pt_insn_event (decoder, &event, sizeof (event));
       if (status < 0)
@@ -1251,8 +1314,13 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	    if (event.status_update != 0)
 	      break;
 
+	    /* Only create a new gap if there are non-aux instructions in
+	       the trace since the last gap.  We could be at the beginning
+	       of the recording and could already have handled one or more
+	       events, like ptev_iret, that created aux insns.  In that
+	       case we don't want to create a gap or print a warning.  */
 	    if (event.variant.enabled.resumed == 0
-		&& !btinfo->functions.empty ())
+		&& ftrace_contains_non_aux_since_last_gap (btinfo))
 	      {
 		struct btrace_function *bfun
 		  = ftrace_new_gap (btinfo, BDE_PT_NON_CONTIGUOUS, gaps);
@@ -1282,7 +1350,6 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
 	case ptev_ptwrite:
 	  {
-	    uint64_t pc = 0;
 	    std::optional<std::string> ptw_string;
 
 	    /* Lookup the PC if available.  The event often doesn't provide
@@ -1314,13 +1381,16 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 		  }
 	      }
 
-	    if (pc == 0)
-	      warning (_("Failed to determine the PC for ptwrite."));
+	    if (!pc.has_value ())
+	      {
+		warning (_("Failed to determine the PC for ptwrite."));
+		pc = 0;
+	      }
 
 	    if (btinfo->ptw_callback_fun != nullptr)
 	      ptw_string
 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
-					    pc, btinfo->ptw_context);
+					    *pc, btinfo->ptw_context);
 
 	    if (ptw_string.has_value () && (*ptw_string).empty ())
 	      continue;
@@ -1333,6 +1403,34 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	    break;
 	  }
 #endif /* defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE) */
+
+#if (LIBIPT_VERSION >= 0x201)
+	case ptev_interrupt:
+	  {
+	    std::string aux_string = std::string (_("interrupt: vector = "))
+	      + hex_string (event.variant.interrupt.vector);
+
+	    const char *decoded
+	      = decode_interrupt_vector (event.variant.interrupt.vector);
+	    if (decoded != nullptr)
+	      aux_string += std::string (" (") + decoded + ")";
+
+	    if (event.variant.interrupt.has_cr2 != 0)
+	      {
+		aux_string += std::string (", cr2 = ")
+		  + hex_string (event.variant.interrupt.cr2);
+	      }
+
+	    if (event.ip_suppressed == 0)
+	      {
+		pc = event.variant.interrupt.ip;
+		aux_string += std::string (", ip = ") + hex_string (*pc);
+	      }
+
+	    handle_pt_aux_insn (btinfo, aux_string, pc);
+	    break;
+	  }
+#endif /* defined (LIBIPT_VERSION >= 0x201) */
 	}
     }
 #endif /* defined (HAVE_PT_INSN_EVENT) */
@@ -1428,7 +1526,9 @@ ftrace_add_pt (struct btrace_thread_info *btinfo,
 	  /* Handle events indicated by flags in INSN.  */
 	  handle_pt_insn_event_flags (btinfo, decoder, insn, gaps);
 
-	  bfun = ftrace_update_function (btinfo, insn.ip);
+	  bfun
+	    = ftrace_update_function (btinfo,
+				      std::make_optional<CORE_ADDR> (insn.ip));
 
 	  /* Maintain the function level offset.  */
 	  *plevel = std::min (*plevel, bfun->level);



Please let me know if this is ok. I can also send a v3.

Thanks,
Felix

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hello Felix,

> #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
> 	case ptev_ptwrite:
> 	  {
>-	    uint64_t pc = 0;
> 	    std::optional<std::string> ptw_string;
>
> 	    /* Lookup the PC if available.  The event often doesn't provide
>@@ -1314,13 +1381,16 @@ handle_pt_insn_events (struct
>btrace_thread_info *btinfo,
> 		  }
> 	      }
>
>-	    if (pc == 0)
>-	      warning (_("Failed to determine the PC for ptwrite."));
>+	    if (!pc.has_value ())
>+	      {
>+		warning (_("Failed to determine the PC for ptwrite."));
>+		pc = 0;
>+	      }

Why do we assign to pc?


> 	    if (btinfo->ptw_callback_fun != nullptr)
> 	      ptw_string
> 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
>-					    pc, btinfo->ptw_context);
>+					    *pc, btinfo->ptw_context);

We need to pass 0 here, if pc is not available, but later (not shown here), we
call handle_pt_aux_insn(), which calls ftrace_update_function().


>Please let me know if this is ok. I can also send a v3.

This looks good with the above fixed.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Dienstag, 24. September 2024 11:26
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 4/6] btrace: Add support for interrupt events.
> 
> Hello Felix,
> 
> > #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
> > 	case ptev_ptwrite:
> > 	  {
> >-	    uint64_t pc = 0;
> > 	    std::optional<std::string> ptw_string;
> >
> > 	    /* Lookup the PC if available.  The event often doesn't provide
> >@@ -1314,13 +1381,16 @@ handle_pt_insn_events (struct
> >btrace_thread_info *btinfo,
> > 		  }
> > 	      }
> >
> >-	    if (pc == 0)
> >-	      warning (_("Failed to determine the PC for ptwrite."));
> >+	    if (!pc.has_value ())
> >+	      {
> >+		warning (_("Failed to determine the PC for ptwrite."));
> >+		pc = 0;
> >+	      }
> 
> Why do we assign to pc?

I wanted to do what you commented below, pass 0 to the ptwrite functions.
But I see that this is wrong now.

> 
> > 	    if (btinfo->ptw_callback_fun != nullptr)
> > 	      ptw_string
> > 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
> >-					    pc, btinfo->ptw_context);
> >+					    *pc, btinfo->ptw_context);
> 
> We need to pass 0 here, if pc is not available, but later (not shown here), we
> call handle_pt_aux_insn(), which calls ftrace_update_function().

See above, I wanted to avoid to make this too complex or to touch more ptwrite code,
as that would probably require a separate commit.

But I don't see an easy way to pass 0 here without adding another bandaid temp
variable or another bandaid if statement that aren't really necessary.

I guess I should really either do these changes to the ptwrite layer as well and post v4,
or revert back to the previous flag based approach, and just not have a default arg.
Do you have any preference?

Thanks,
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> -----Original Message-----
> From: Willgerodt, Felix <felix.willgerodt@intel.com>
> Sent: Dienstag, 24. September 2024 11:56
> To: Metzger, Markus T <markus.t.metzger@intel.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH v2 4/6] btrace: Add support for interrupt events.
> 
> > -----Original Message-----
> > From: Metzger, Markus T <markus.t.metzger@intel.com>
> > Sent: Dienstag, 24. September 2024 11:26
> > To: Willgerodt, Felix <felix.willgerodt@intel.com>
> > Cc: gdb-patches@sourceware.org
> > Subject: RE: [PATCH v2 4/6] btrace: Add support for interrupt events.
> >
> > Hello Felix,
> >
> > > #if defined (HAVE_STRUCT_PT_EVENT_VARIANT_PTWRITE)
> > > 	case ptev_ptwrite:
> > > 	  {
> > >-	    uint64_t pc = 0;
> > > 	    std::optional<std::string> ptw_string;
> > >
> > > 	    /* Lookup the PC if available.  The event often doesn't provide
> > >@@ -1314,13 +1381,16 @@ handle_pt_insn_events (struct
> > >btrace_thread_info *btinfo,
> > > 		  }
> > > 	      }
> > >
> > >-	    if (pc == 0)
> > >-	      warning (_("Failed to determine the PC for ptwrite."));
> > >+	    if (!pc.has_value ())
> > >+	      {
> > >+		warning (_("Failed to determine the PC for ptwrite."));
> > >+		pc = 0;
> > >+	      }
> >
> > Why do we assign to pc?
> 
> I wanted to do what you commented below, pass 0 to the ptwrite functions.
> But I see that this is wrong now.
> 
> >
> > > 	    if (btinfo->ptw_callback_fun != nullptr)
> > > 	      ptw_string
> > > 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
> > >-					    pc, btinfo->ptw_context);
> > >+					    *pc, btinfo->ptw_context);
> >
> > We need to pass 0 here, if pc is not available, but later (not shown here), we
> > call handle_pt_aux_insn(), which calls ftrace_update_function().
> 
> See above, I wanted to avoid to make this too complex or to touch more ptwrite
> code,
> as that would probably require a separate commit.
> 
> But I don't see an easy way to pass 0 here without adding another bandaid temp
> variable or another bandaid if statement that aren't really necessary.
> 
> I guess I should really either do these changes to the ptwrite layer as well and
> post v4,
> or revert back to the previous flag based approach, and just not have a default
> arg.
> Do you have any preference?
> 

On second thought, the ptwrite changes aren't actually that big. I wrongly
thought I would need to touch the extension interface.
This is the diff based on the previous diff I posted:

diff --git a/gdb/btrace.c b/gdb/btrace.c
index 2665547d8d4..152f6f2ec47 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1382,15 +1382,13 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
 	      }
 
 	    if (!pc.has_value ())
-	      {
-		warning (_("Failed to determine the PC for ptwrite."));
-		pc = 0;
-	      }
+	      warning (_("Failed to determine the PC for ptwrite."));
+
 
 	    if (btinfo->ptw_callback_fun != nullptr)
 	      ptw_string
 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
-					    *pc, btinfo->ptw_context);
+					    pc, btinfo->ptw_context);
 
 	    if (ptw_string.has_value () && (*ptw_string).empty ())
 	      continue;
diff --git a/gdb/btrace.h b/gdb/btrace.h
index 3d8aee767c3..a4960f31063 100644
--- a/gdb/btrace.h
+++ b/gdb/btrace.h
@@ -360,7 +360,7 @@ struct btrace_thread_info
   /* Function pointer to the ptwrite callback.  Returns the string returned
      by the ptwrite filter function.  */
   std::optional<std::string> (*ptw_callback_fun) (const uint64_t payload,
-						  const uint64_t ip,
+						  std::optional<uint64_t> ip,
 						  const void *ptw_context)
 						    = nullptr;
 
diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
index 55ee67efd81..ded1be93cd8 100644
--- a/gdb/python/py-record-btrace.c
+++ b/gdb/python/py-record-btrace.c
@@ -810,7 +810,7 @@ recpy_bt_function_call_history (PyObject *self, void *closure)
 /* Helper function that calls PTW_FILTER with PAYLOAD and IP as arguments.
    Returns the string that will be printed, if there is a filter to call.  */
 static std::optional<std::string>
-recpy_call_filter (const uint64_t payload, const uint64_t ip,
+recpy_call_filter (const uint64_t payload, std::optional<uint64_t> ip,
 		   const void *ptw_filter)
 {
   std::optional<std::string> result;
@@ -824,10 +824,10 @@ recpy_call_filter (const uint64_t payload, const uint64_t ip,
   gdbpy_ref<> py_payload = gdb_py_object_from_ulongest (payload);
 
   gdbpy_ref<> py_ip;
-  if (ip == 0)
+  if (!ip.has_value ())
     py_ip = gdbpy_ref<>::new_reference (Py_None);
   else
-    py_ip = gdb_py_object_from_ulongest (ip);
+    py_ip = gdb_py_object_from_ulongest (*ip);
 
   gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *) ptw_filter,
 							py_payload.get (),

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hello Felix,

>On second thought, the ptwrite changes aren't actually that big. I wrongly
>thought I would need to touch the extension interface.
>This is the diff based on the previous diff I posted:
>
>diff --git a/gdb/btrace.c b/gdb/btrace.c
>index 2665547d8d4..152f6f2ec47 100644
>--- a/gdb/btrace.c
>+++ b/gdb/btrace.c
>@@ -1382,15 +1382,13 @@ handle_pt_insn_events (struct
>btrace_thread_info *btinfo,
> 	      }
>
> 	    if (!pc.has_value ())
>-	      {
>-		warning (_("Failed to determine the PC for ptwrite."));
>-		pc = 0;
>-	      }
>+	      warning (_("Failed to determine the PC for ptwrite."));
>+
>
> 	    if (btinfo->ptw_callback_fun != nullptr)
> 	      ptw_string
> 		= btinfo->ptw_callback_fun (event.variant.ptwrite.payload,
>-					    *pc, btinfo->ptw_context);
>+					    pc, btinfo->ptw_context);
>
> 	    if (ptw_string.has_value () && (*ptw_string).empty ())
> 	      continue;
>diff --git a/gdb/btrace.h b/gdb/btrace.h
>index 3d8aee767c3..a4960f31063 100644
>--- a/gdb/btrace.h
>+++ b/gdb/btrace.h
>@@ -360,7 +360,7 @@ struct btrace_thread_info
>   /* Function pointer to the ptwrite callback.  Returns the string returned
>      by the ptwrite filter function.  */
>   std::optional<std::string> (*ptw_callback_fun) (const uint64_t payload,
>-						  const uint64_t ip,
>+						  std::optional<uint64_t> ip,
> 						  const void *ptw_context)
> 						    = nullptr;
>
>diff --git a/gdb/python/py-record-btrace.c b/gdb/python/py-record-btrace.c
>index 55ee67efd81..ded1be93cd8 100644
>--- a/gdb/python/py-record-btrace.c
>+++ b/gdb/python/py-record-btrace.c
>@@ -810,7 +810,7 @@ recpy_bt_function_call_history (PyObject *self, void
>*closure)
> /* Helper function that calls PTW_FILTER with PAYLOAD and IP as arguments.
>    Returns the string that will be printed, if there is a filter to call.  */
> static std::optional<std::string>
>-recpy_call_filter (const uint64_t payload, const uint64_t ip,
>+recpy_call_filter (const uint64_t payload, std::optional<uint64_t> ip,
> 		   const void *ptw_filter)
> {
>   std::optional<std::string> result;
>@@ -824,10 +824,10 @@ recpy_call_filter (const uint64_t payload, const
>uint64_t ip,
>   gdbpy_ref<> py_payload = gdb_py_object_from_ulongest (payload);
>
>   gdbpy_ref<> py_ip;
>-  if (ip == 0)
>+  if (!ip.has_value ())
>     py_ip = gdbpy_ref<>::new_reference (Py_None);
>   else
>-    py_ip = gdb_py_object_from_ulongest (ip);
>+    py_ip = gdb_py_object_from_ulongest (*ip);
>
>   gdbpy_ref<> py_result (PyObject_CallFunctionObjArgs ((PyObject *)
>ptw_filter,
> 							py_payload.get (),

LGTM,
Markus.
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

> LGTM,
> Markus.

Thanks, I just merged this series.

Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928