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

Metzger, Markus T via Gdb-patches
2023-07-25T13:31:00Z

Hello Felix,

>+/* Read config bits. */
>+
>+static bool
>+linux_read_pt_config_bit (const std::string &feature, uint64_t *config_bit)

This isn't really necessary since the bit resembles the enable bit in the control MSR.
It also doesn't hurt so I'm OK with the change.

>+{
>+ std::string filename
>+ = "/sys/bus/event_source/devices/intel_pt/format/" + feature;
>+ gdb_file_up file = gdb_fopen_cloexec (filename.c_str (), "r");
>+
>+ if (file.get () == nullptr || config_bit == nullptr)
>+ return false;

We could check CONFIG_BIT before opening the file.

>+
>+ int found = fscanf (file.get (), "config:%lu", config_bit);

The format string doesn't match the type. We should use SCNu64.

>+ if (found != 1)
>+ {
>+ warning (_("Failed to determine config bit from %s."),
>+ filename.c_str ());

The issue is that the file didn't contain the expected content.
This will probably need some changes in GDB to fix. Could we
print the actual content and what we expected?

>+/* Check whether the linux target supports Intel Processor Trace PTWRITE. */
>+
>+static bool
>+linux_supports_ptwrite (uint64_t *config_bit)
>+{
>+ 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;
>+ }
>+
>+ if (!linux_read_pt_config_bit ("ptw", config_bit))

We should check STATUS before. If caps indicates that ptwrite isn't
available, I'm not sure format will contain the bit position for enabling
it.

>+ uint64_t config_bit;
>+ if (conf->ptwrite && linux_supports_ptwrite (&config_bit))
>+ {
>+ pt->attr.config |= 1 << config_bit;
>+ 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 c93b3d7c8de..ad3160d42c5 100644
>--- a/gdb/record-btrace.c
>+++ b/gdb/record-btrace.c
>@@ -3295,4 +3295,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

Is there a way to get an error when attempting to install a custom
ptwrite filter when either GDB (via libipt) or the kernel does not
support PTWRITE?

Regards,
Markus.