Re: [PATCH v2] gdb: c++ify btrace_target_info

[Simon Marchi <simon.marchi@polymtl.ca>]

On 9/7/23 06:44, Markus Metzger wrote:
> Following the example of private_thread_info and private_inferior, turn
> struct btrace_target_info into a small class hierarchy.
> 
> Fixes PR gdb/30751.
> ---
>  gdb/nat/linux-btrace.c      | 50 ++++++++++++++++++++++++++-----------
>  gdb/nat/linux-btrace.h      | 11 +++++++-
>  gdb/remote.c                | 48 ++++++++++++++++++++++++++---------
>  gdbsupport/btrace-common.cc |  2 ++
>  gdbsupport/btrace-common.h  |  5 +++-
>  5 files changed, 87 insertions(+), 29 deletions(-)
> 
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index c5b3f1c93cf..eefcabf509f 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -277,7 +277,7 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
>     part at the end and its upper part at the beginning of the buffer.  */
>  
>  static std::vector<btrace_block> *
> -perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
> +perf_event_read_bts (linux_btrace_target_info* tinfo, const uint8_t *begin,

Could you fix the misplaced * while at it?

> @@ -81,8 +82,16 @@ struct btrace_tinfo_pt
>  #endif /* HAVE_LINUX_PERF_EVENT_H */
>  
>  /* Branch trace target information per thread.  */
> -struct btrace_target_info
> +struct linux_btrace_target_info final : public btrace_target_info
>  {
> +  linux_btrace_target_info (ptid_t ptid, const btrace_config &conf)
> +    : ptid (ptid), conf (conf), variant ({})
> +    {}

It seems like that this constructor is unused.

> +
> +  linux_btrace_target_info (ptid_t ptid)
> +    : ptid (ptid), conf ({}), variant ({})
> +    {}

I read your reply about zero-initializing the conf and variant fields.

I think that's fine, it's probably necessary since the users initialize
the relevant fields right after, but it's on the safer side since we are
replacing XCNEW.  But I would suggest putting the {} in the member
declaration directly (which we often do when C++ifying things):

  btrace_config conf {};

  union
  {
    /* CONF.FORMAT == BTRACE_FORMAT_BTS.  */
    struct btrace_tinfo_bts bts;

    /* CONF.FORMAT == BTRACE_FORMAT_PT.  */
    struct btrace_tinfo_pt pt;
  } variant {};


With those fixed:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon