Re: [PATCH 2/2] gdb: c++ify btrace_target_info

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

> @@ -460,9 +483,8 @@ linux_enable_bts (ptid_t ptid, const struct btrace_config_bts *conf)
>    if (!cpu_supports_bts ())
>      error (_("BTS support has been disabled for the target cpu."));
>  
> -  gdb::unique_xmalloc_ptr<btrace_target_info> tinfo
> -    (XCNEW (btrace_target_info));
> -  tinfo->ptid = ptid;
> +  std::unique_ptr<linux_btrace_target_info> tinfo
> +    { new linux_btrace_target_info (ptid) };
Not mandatory, but we often define aliases for unique pointer types, to
ease reading a bit:

using linux_btrace_target_info_up = std::unique_ptr<linux_btrace_target_info>;

> @@ -775,7 +799,7 @@ linux_disable_btrace (struct btrace_target_info *tinfo)
>      }
>  
>    if (errcode == BTRACE_ERR_NONE)
> -    xfree (tinfo);
> +    delete gtinfo;

A question about this, even if it's not introduced by this patch... if
errcode is not BTRACE_ERR_NONE, who deletes gtinfo?

> @@ -81,7 +82,7 @@ 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 : public btrace_target_info

You can make the class (struct) final.

>  {
>    /* The ptid of this thread.  */
>    ptid_t ptid;

Not shown here, but the linux_btrace_target_info has this union:

  /* The branch tracing format specific information.  */
  union
  {
    /* CONF.FORMAT == BTRACE_FORMAT_BTS.  */
    struct btrace_tinfo_bts bts;

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

This might be a good candidate to become separate sub-classes (not in
this patch, but later)?

> @@ -100,6 +101,15 @@ struct btrace_target_info
>      struct btrace_tinfo_pt pt;
>    } variant;
>  #endif /* HAVE_LINUX_PERF_EVENT_H */
> +
> +
> +  linux_btrace_target_info (ptid_t _ptid, const struct btrace_config &_conf)
> +    : ptid (_ptid), conf (_conf), variant ({})
> +    {}
> +
> +  linux_btrace_target_info (ptid_t _ptid)
> +    : ptid (_ptid), conf ({}), variant ({})

It's probably not needed to explicitly initialize conf and variant.

> +    {}

We usually put constructors and methods before fields.

>  };
>  
>  /* See to_enable_btrace in target.h.  */
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 55f2fc3b6b5..043781cea06 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -14423,15 +14423,45 @@ parse_xml_btrace_conf (struct btrace_config *conf, const char *xml)
>  #endif  /* !defined (HAVE_LIBEXPAT) */
>  }
>  
> -struct btrace_target_info
> +struct remote_btrace_target_info : public btrace_target_info

final here too.

>  {
>    /* The ptid of the traced thread.  */
>    ptid_t ptid;
>  
>    /* The obtained branch trace configuration.  */
>    struct btrace_config conf;

So, these two fields are common to both btrace_target_info sub-classes.
Do you intend to move them to the base class?  That would allow getting
rid of some (all?) casts in remote.c.

> +
> +  remote_btrace_target_info (ptid_t _ptid, const struct btrace_config &_conf)

In C++ we tend to drop the struct keyword when not needed.

> +    : ptid (_ptid), conf (_conf)
> +    {}

Did you take this style of leading underscore from somewhere?  I
typically don't mind giving the same name to fields an constructor
parameters, it works just fine.  Otherwise, I've seen others use
trailing underscores.

> +
> +  remote_btrace_target_info (ptid_t _ptid)
> +    : ptid (_ptid), conf ({})

The latter (conf) is probably not need, it will automatically
default-construct.

> +    {}
>  };
>  
> +/* Get the remote version of a btrace_target_info.  */
> +
> +static remote_btrace_target_info *
> +get_remote_btrace_target_info (btrace_target_info *gtinfo)
> +{
> +  if (gtinfo == nullptr)
> +    return nullptr;

I don't think the nullptr check is needed, checked_static_cast (which is
either static_cast or dynamic_cast) should accept nullptr.

> @@ -14576,7 +14605,7 @@ struct btrace_target_info *
>  remote_target::enable_btrace (thread_info *tp,
>  			      const struct btrace_config *conf)
>  {
> -  struct btrace_target_info *tinfo = NULL;
> +  struct remote_btrace_target_info *tinfo = NULL;

Since you touch this line, I would suggest moving the declaration to
where the variable is initialized.

>    struct packet_config *packet = NULL;
>    struct remote_state *rs = get_remote_state ();
>    char *buf = rs->buf.data ();
> @@ -14620,8 +14649,7 @@ remote_target::enable_btrace (thread_info *tp,
>  	       target_pid_to_str (ptid).c_str ());
>      }
>  
> -  tinfo = XCNEW (struct btrace_target_info);
> -  tinfo->ptid = ptid;
> +  tinfo = new remote_btrace_target_info { ptid };

If I read things correctly, I think that a further step (not in this
patch) could be to make remote_target::enable_btrace return an
std::unique_ptr<btrace_target_info> (btrace_target_info_up).

>  
>    /* If we fail to read the configuration, we lose some information, but the
>       tracing itself is not impacted.  */
> @@ -14641,7 +14669,7 @@ remote_target::enable_btrace (thread_info *tp,
>  /* Disable branch tracing.  */
>  
>  void
> -remote_target::disable_btrace (struct btrace_target_info *tinfo)
> +remote_target::disable_btrace (struct btrace_target_info *gtinfo)

... and this could probably accept a btrace_target_info_up.

>  {
>    struct remote_state *rs = get_remote_state ();
>    char *buf = rs->buf.data ();
> @@ -14650,6 +14678,7 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo)
>    if (m_features.packet_support (PACKET_Qbtrace_off) != PACKET_ENABLE)
>      error (_("Target does not support branch tracing."));
>  
> +  remote_btrace_target_info *tinfo = get_remote_btrace_target_info (gtinfo);
>    set_general_thread (tinfo->ptid);
>  
>    buf += xsnprintf (buf, endbuf - buf, "%s",
> @@ -14667,7 +14696,7 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo)
>  	       target_pid_to_str (tinfo->ptid).c_str ());
>      }
>  
> -  xfree (tinfo);
> +  delete gtinfo;
>  }
>  
>  /* Teardown branch tracing.  */
> @@ -14676,7 +14705,7 @@ void
>  remote_target::teardown_btrace (struct btrace_target_info *tinfo)
>  {
>    /* We must not talk to the target during teardown.  */
> -  xfree (tinfo);
> +  delete tinfo;

And it seems like this could take a btrace_target_info_up too?

I just noticed that remote_target::teardown_btrace deletes the tinfo
it receives, but x86_linux_nat_target::teardown_btrace doesn't.  Is that
a bug / leak on the x86-linux-nat side?

>  }
>  
>  /* Read the branch trace.  */
> @@ -14723,8 +14752,10 @@ remote_target::read_btrace (struct btrace_data *btrace,
>  }
>  
>  const struct btrace_config *
> -remote_target::btrace_conf (const struct btrace_target_info *tinfo)
> +remote_target::btrace_conf (const struct btrace_target_info *gtinfo)
>  {
> +  const remote_btrace_target_info *tinfo
> +    = get_remote_btrace_target_info (gtinfo);
>    return &tinfo->conf;
>  }
>  
> diff --git a/gdbsupport/btrace-common.h b/gdbsupport/btrace-common.h
> index e287c93a6c1..7ae865b3978 100644
> --- a/gdbsupport/btrace-common.h
> +++ b/gdbsupport/btrace-common.h
> @@ -214,7 +214,10 @@ struct btrace_data
>  };
>  
>  /* Target specific branch trace information.  */
> -struct btrace_target_info;
> +struct btrace_target_info
> +{
> +  virtual ~btrace_target_info () {}
> +};

I would typically make the destructor pure to make it impossible to
instantiate an object of this type:

  virtual ~btrace_target_info () = 0;

If I remember correctly, you then need to define the destructor in the
.cc file then, to avoid an undefined reference, something like:

btrace_target_info::~btrace_target_info () = default;

Simon