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

["Willgerodt, Felix" <felix.willgerodt@intel.com>]

> -----Original Message-----
> From: Metzger, Markus T <markus.t.metzger@intel.com>
> Sent: Mittwoch, 9. August 2023 11:48
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; simark@simark.ca
> Subject: RE: [PATCH v10 08/10] btrace, linux: Enable ptwrite packets.
> 
> Hello Felix,
> 
> >> >> >+#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?
> >> >
> >> >Good point. Unfortunately I don't see one with the current design.
> >> >Currently we just cache the filter functions in python when one is
> >> >registered. The configuration isn't exposed to the python layer in
> >> >any way.
> >> >
> >> >I am not sure how we could do this. Exposing "maint btrace info"
> >> >and parsing that would be one way, not sure if we want to go down
> >> >that path.
> >>
> >> We could warn at decode-time when applying the ptwrite filter.
> >> We should have different warning texts for GDB (via libipt) not
> >> supporting ptwrite and the system (kernel or h/w) not supporting it.
> >>
> >> Not as good as an error when installing the ptwrite filter but better
> >> than wondering why it isn't working as expected.
> >
> >Mhm, I don't see this either. We can't warn when we actually would
> >apply the filter, as we never would get a ptwrite event.
> 
> I was referring to the apply_ext_lang_ptwrite_filter() call in ftrace_add_pt().
> 
> > As we don't
> >enable ptwrite in Linux if libipt doesn't support it.  And if libipt doesn't
> >support it, we wouldn't get to that place in GDB in the first place,
> >as there is an #ifdef protecting it.
> >
> >I looked at doing it when installing the extension language callback
> >for ptwrite in btrace.c, e.g. something similar to this:
> >
> >  /* Register the ptwrite filter.  */
> >  if (btinfo->target->conf.pt.ptwrite)
> >	apply_ext_lang_ptwrite_filter (btinfo);
> >  else
> >    warning (_("Can't decode ptwrite events."));
> >
> >This we do regardless of libipt. But then we would get a warning
> >regardless of the user trying to use ptwrite or not (with old libipt
> >versions). Plus this doesn't compile, as struct btrace_target_info
> >is forward declared, and we probably can't change that as btrace.c
> >should be target independent.
> 
> We'd want the check inside gdbpy_load_ptwrite_filter(), which gets
> called by apply_ext_lang_ptwrite_filter() to install the btrace callback.

If I add a similar warning to what I posted above for btrace.c, to
python/py-record-btrace.c:gdbpy_load_ptwrite_filter(), I still can't
compile. I get the following error:

py-record-btrace.c:906:22: error: invalid use of incomplete type ?struct btrace_target_info?
  906 |   if (!btinfo->target->conf.pt.ptwrite)

The solution would be to include nat/linux-btrace.h to the file, or to move
that struct to some other common header, e.g. btrace-common.h.
I don't think either is possible, it is adding linux specific things to common files.
Or is there any other solution that I missed?

Another point is, even if I would put it there, gdbpy_load_ptwrite_filter()
is called regardless of the user having installed a python filter.
But I think we could additionally check that get_ptwrite_filter() didn't return
Py_None to avoid that issue.

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