gdb, btrace: support libipt v2.2 events

  ---
 gdb/btrace.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
  

Hi,

On 4/30/26 4:24 AM, Markus Metzger wrote:
> ---
>   gdb/btrace.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)

There appears to be no commit message other than the (admittedly
rather complete) title?

I think a brief NEWS entry is warranted, too.

There is one tiny nit (below), but otherwise, I think you should approve 
your patch. ;-)

Reviewed-By: Keith Seitz <keiths@redhat.com>

Keith

> 
> diff --git a/gdb/btrace.c b/gdb/btrace.c
> index 6a8f0f549d1..f9c6f0e5b6a 100644
> --- a/gdb/btrace.c
> +++ b/gdb/btrace.c
> @@ -1597,6 +1597,59 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
>   	    break;
>   	  }
>   #endif /* defined (LIBIPT_VERSION >= 0x201) */
> +
> +#if (LIBIPT_VERSION >= 0x202)
> +	case ptev_trig:
> +	  {
> +	    std::string aux_string = std::string (_("trig: trbv = "))
> +	      + hex_string (event.variant.trig.trbv);
> +
> +	    if (event.variant.trig.mult != 0)
> +	      aux_string += std::string (", mult");
> +
> +	    if (event.ip_suppressed == 0)
> +	      {
> +		pc = event.variant.trig.ip;
> +		aux_string += std::string (", ip = ") + hex_string (*pc);
> +	      }
> +
> +	    if (event.variant.trig.icnt != 0)
> +	      aux_string += std::string (", icnt = ")
> +		+ hex_string (event.variant.trig.icnt);
> +
> +	    handle_pt_aux_insn (btinfo, aux_string, pc);
> +	    break;
> +	  }
> +
> +	case ptev_swintr:
> +	  {
> +	    std::string aux_string = std::string (_("swintr: vector = "))
> +	      + hex_string (event.variant.swintr.vector);
> +
> +	    if (event.ip_suppressed == 0)
> +	      {
> +		pc = event.variant.swintr.ip;
> +		aux_string += std::string (", ip = ") + hex_string (*pc);
> +	      }
> +
> +	    handle_pt_aux_insn (btinfo, aux_string, pc);
> +	    break;
> +	  }
> +
> +	case ptev_syscall:
> +	  {
> +	    std::string aux_string = std::string (_("syscall"));
> +
> +	    if (event.ip_suppressed == 0)
> +	      {
> +		pc = event.variant.syscall.ip;
> +		aux_string += std::string (": ip = ") + hex_string (*pc);
> +	      }
> +
> +	    handle_pt_aux_insn (btinfo, aux_string, pc);
> +	    break;
> +	  }
> +#endif /* (LIBIPT_VERSION >= 0x202) */

Elsewhere in the file, this comment includes "defined".

>   	}
>       }
>   #endif /* defined (HAVE_PT_INSN_EVENT) */
  

>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:

>> +	    std::string aux_string = std::string (_("trig: trbv = "))
>> +	      + hex_string (event.variant.trig.trbv);

Normally here we'd wrap the RSH in parens, per the coding standards.

>> +#endif /* (LIBIPT_VERSION >= 0x202) */

Keith> Elsewhere in the file, this comment includes "defined".

True but those spots seem to be in error.

Tom
  

Hello Tom,

>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>
>>> +	    std::string aux_string = std::string (_("trig: trbv = "))
>>> +	      + hex_string (event.variant.trig.trbv);
>
>Normally here we'd wrap the RSH in parens, per the coding standards.

I'd need to break at the =, then, like this:

	    std::string aux_string
	      = (std::string (_("trig: trbv = "))
		 + hex_string (event.variant.trig.trbv));

The patch matches the surrounding style, so if you want this fixed, I'd need
to fix it everywhere in a separate patch.


>>> +#endif /* (LIBIPT_VERSION >= 0x202) */
>
>Keith> Elsewhere in the file, this comment includes "defined".
>
>True but those spots seem to be in error.

I fixed those in a separate obvious patch:

diff --git a/gdb/btrace.c b/gdb/btrace.c
index f9c6f0e5b6a..ef152c531fb 100644
--- a/gdb/btrace.c
+++ b/gdb/btrace.c
@@ -1283,7 +1283,7 @@ decode_interrupt_vector (const uint8_t vector)
 
   return nullptr;
 }
-#endif /* defined (LIBIPT_VERSION >= 0x201) */
+#endif /* (LIBIPT_VERSION >= 0x201) */
 
 /* Handle instruction decode events (libipt-v2).  */
 
@@ -1596,7 +1596,7 @@ handle_pt_insn_events (struct btrace_thread_info *btinfo,
            handle_pt_aux_insn (btinfo, aux_string, pc);
            break;
          }
-#endif /* defined (LIBIPT_VERSION >= 0x201) */
+#endif /* (LIBIPT_VERSION >= 0x201) */
 
 #if (LIBIPT_VERSION >= 0x202)
        case ptev_trig:
@@ -3064,7 +3064,7 @@ pt_print_packet (const struct pt_packet *packet)
                  packet->payload.ptw.payload,
                  packet->payload.ptw.ip ? (" ip") : (""));
       break;
-#endif /* defined (LIBIPT_VERSION >= 0x200)  */
+#endif /* (LIBIPT_VERSION >= 0x200)  */
 
 #if (LIBIPT_VERSION >= 0x201)
     case ppt_cfe:
@@ -3077,7 +3077,7 @@ pt_print_packet (const struct pt_packet *packet)
       gdb_printf (("evd %u: 0x%" PRIx64 ""), packet->payload.evd.type,
                  packet->payload.evd.payload);
       break;
-#endif /* defined (LIBIPT_VERSION >= 0x201)  */
+#endif /* (LIBIPT_VERSION >= 0x201)  */
     }
 }

Markus.
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hello Keith,

>On 4/30/26 4:24 AM, Markus Metzger wrote:
>> ---
>>   gdb/btrace.c | 53
>++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 53 insertions(+)
>
>There appears to be no commit message other than the (admittedly
>rather complete) title?
>
>I think a brief NEWS entry is warranted, too.

I extended the former and added the latter.  Will send a v2 so we can review
the NEWS wording.

>Reviewed-By: Keith Seitz <keiths@redhat.com>

Thanks for your review,
Makus.

Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

On 5/4/26 3:16 AM, Metzger, Markus T wrote:
> Hello Tom,
> 
>>>>>>> "Keith" == Keith Seitz <keiths@redhat.com> writes:
>>
>>>> +	    std::string aux_string = std::string (_("trig: trbv = "))
>>>> +	      + hex_string (event.variant.trig.trbv);
>>
>> Normally here we'd wrap the RSH in parens, per the coding standards.
> 
> I'd need to break at the =, then, like this:
> 
> 	    std::string aux_string
> 	      = (std::string (_("trig: trbv = "))
> 		 + hex_string (event.variant.trig.trbv));
> 
> The patch matches the surrounding style, so if you want this fixed, I'd need
> to fix it everywhere in a separate patch.

Just noting that you could also use `string_printf` for this, which
sometimes gives more readable code than using concatenation (and
perhaps one day we'll use {fmt}/std::format, one can dream).

Simon
  

> I'd need to break at the =, then, like this:

> 	    std::string aux_string
> 	      = (std::string (_("trig: trbv = "))
> 		 + hex_string (event.variant.trig.trbv));

> The patch matches the surrounding style, so if you want this fixed, I'd need
> to fix it everywhere in a separate patch.

In gdb it's pretty normal for some code to be weird or wrong.
For example tdep files are less reviewed than others.

However the presence of some code that isn't standards-conformant
doesn't mean that new code should be.  You see this a lot with things
like NULL/nullptr -- using nullptr in new code doesn't imply that all
the old code has to be updated at the same time.

So IMO new code should follow the relevant coding standards, even if
other code in the same function does not.

thanks,
Tom
  

Hello Tom,

>> I'd need to break at the =, then, like this:
>
>> 	    std::string aux_string
>> 	      = (std::string (_("trig: trbv = "))
>> 		 + hex_string (event.variant.trig.trbv));
>
>> The patch matches the surrounding style, so if you want this fixed, I'd need
>> to fix it everywhere in a separate patch.
>
>In gdb it's pretty normal for some code to be weird or wrong.
>For example tdep files are less reviewed than others.
>
>However the presence of some code that isn't standards-conformant
>doesn't mean that new code should be.  You see this a lot with things
>like NULL/nullptr -- using nullptr in new code doesn't imply that all
>the old code has to be updated at the same time.
>
>So IMO new code should follow the relevant coding standards, even if
>other code in the same function does not.

I changed this in v5 [1].

[1]: https://sourceware.org/pipermail/gdb-patches/2026-May/227179.html

Regards,
Markus.
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928