Re: [PATCH v4 10/10] btrace: Extend ptwrite event decoding.

[Eli Zaretskii <eliz@gnu.org>]

> Date: Fri,  6 May 2022 13:40:10 +0200
> From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> 
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a72fee81550..cf807d6209e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -331,6 +331,12 @@ GNU/Linux/OpenRISC		or1k*-*-linux*
>  
>  *** Changes in GDB 11
>  
> +* GDB now supports printing of ptwrite payloads from the Intel Processor
> +  Trace during 'record instruction-history', 'record function-call-history',
> +  all stepping commands and in  Python.  Printing is customizable via a
> +  ptwrite listener function in  Python.  By default, the raw ptwrite
> +  payload is printed for each ptwrite that is encountered.
> +

This part is OK.

> +@exdent @value{GDBN} output after recording the sample program in pt format:
> +@smallexample
> +@group
> +(gdb) record instruction-history 12,14
> +12         0x000000000040074c <ptwrite64+16>:	ptwrite %rbx
> +13         [42]
> +14         0x0000000000400751 <ptwrite64+21>:	mov    -0x8(%rbp),%rbx

The last line is too long, and will cause overfull hbox errors when
producing the printed manual.  Please make it shorter, or break into 2
lines.

> +@findex gdb.ptwrite.register_listener
> +@defun register_listener (@var{listener})

@defun automatically inserts the function into the index, so no need
for a separate @findex (here and elsewhere in the patch).

> +Used to register the ptwrite listener.  The listener can be any callable
> +object that accepts two arguments.

I think we should describe those 2 arguments, at least shortly.

> +@defun default_listener (@var{payload}, @var{ip})
> +The listener function active by default.

Which does what?  If we don't say anything about the default behavior,
how can a user decide whether to provide his/her specialized listener?

> +@smallexample
> +@group
> +(gdb) python-interactive
> +>>> class my_listener(object):
> +...    def __init__(self):
> +...        self.var = 0
> +...    def __call__(self, payload, ip):
> +...        if gdb.selected_thread().global_num == 1:
> +...            self.var += 1
> +...            return "counter: @{@}, ip: @{:#x@}".format(self.var, ip)

This last line is too long.

(And I wonder whether this example really adds something of value to
what you already described above.)

> +@group
> +(gdb) info threads 
> +* 1    Thread 0x7ffff7fd8740 (LWP 25796) "ptwrite_threads" task (arg=0x0)
> +    at bin/ptwrite/ptwrite_threads.c:45
> +  2    Thread 0x7ffff6eb8700 (LWP 25797) "ptwrite_threads" task (arg=0x0)
> +    at bin/ptwrite/ptwrite_threads.c:45

These lines are also too long.

> +This @value{GDBN} feature is dependent on hardware and operating system
> +support and requires the Intel Processor Trace decoder library in version
> +2.0.0 or newer.

You say "hardware and operating system support", but say nothing about
the OSes on which this can be supported.

Thanks.