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

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

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