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

  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     | 57 ++++++++++++++++++++++++++++----------
 gdb/nat/linux-btrace.h     | 12 +++++++-
 gdb/remote.c               | 55 ++++++++++++++++++++++++++++--------
 gdbsupport/btrace-common.h |  5 +++-
 4 files changed, 101 insertions(+), 28 deletions(-)
  

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

Hello Simon,

Thanks for your review.

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

It would be leaked.  Since tracing couldn't be disabled, it would still
be in use, so leaking it seems better than deleting an in-use object
and risking either crash or data corruption.

Now, that cannot happen today, since both linux_disable_pt and
linux_disable_bts return BTRACE_ERR_NONE.

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

That's an interesting question.  My interpretation is that not specifying a
ctor-initializer would leave things undefined*, whereas supplying an empty
aggregate would zero-initialize everything.

*we'd default-construct conf, which has an implicitly defined default
constructor, which then default-constructs all the data members, and so on,
until we reach plain integers, on which no initialization is performed.


>>  {
>>    /* 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.

No.  I think it's better to leave the base class empty to give full freedom
to targets.


>> +    : 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.

I saw it in GDB.  I also saw (ptid_).  And, now that I searched for it,
I also see _foo_, __foo, __foo__.  Dropping the _.


>>  /* 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?

The linux target leaves that to linux_disable_btrace().


I'll send a v2 patch with the changes you requested.

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
  

On 9/7/23 06:42, Metzger, Markus T wrote:
> Hello Simon,
> 
> Thanks for your review.
> 
>>>    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?
> 
> It would be leaked.  Since tracing couldn't be disabled, it would still
> be in use, so leaking it seems better than deleting an in-use object
> and risking either crash or data corruption.

I'm not sure I follow.  Even if something fails when trying to disable
btrace (like a call to the OS fails for an unknown reason), GDB can
still decide it doesn't want to do tracing / consume btrace data and get
rid of the object.

> Now, that cannot happen today, since both linux_disable_pt and
> linux_disable_bts return BTRACE_ERR_NONE.

Ok, well if these operations can't fail I think they should not return
an error code (for the same reason I wrote below about hypothetical
scenarios).

>>> @@ -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.
> 
> That's an interesting question.  My interpretation is that not specifying a
> ctor-initializer would leave things undefined*, whereas supplying an empty
> aggregate would zero-initialize everything.
> 
> *we'd default-construct conf, which has an implicitly defined default
> constructor, which then default-constructs all the data members, and so on,
> until we reach plain integers, on which no initialization is performed.

Ok, replied to that in my response to v2.

>>>  {
>>>    /* 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.
> 
> No.  I think it's better to leave the base class empty to give full freedom
> to targets.

Do you have an actual case of btrace target that wouldn't use these
fields? My stance on it is: this is all internal GDB code, it's not an
API we expose.  I don't think we should complicate things for
hypothetical scenarios, when we always have the freedom to re-work
things later if need be.  I think it makes more sense to keep things
simple for what we have today.

Simon
  

Hello Simon,

>>>>    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?
>>
>> It would be leaked.  Since tracing couldn't be disabled, it would still
>> be in use, so leaking it seems better than deleting an in-use object
>> and risking either crash or data corruption.
>
>I'm not sure I follow.  Even if something fails when trying to disable
>btrace (like a call to the OS fails for an unknown reason), GDB can
>still decide it doesn't want to do tracing / consume btrace data and get
>rid of the object.

In the remote case, when the disable request fails with an error, I wanted
to allow GDB to try to disable again.


>> Now, that cannot happen today, since both linux_disable_pt and
>> linux_disable_bts return BTRACE_ERR_NONE.
>
>Ok, well if these operations can't fail I think they should not return
>an error code (for the same reason I wrote below about hypothetical
>scenarios).

This code is used in GDB and in gdbserver and, historically, the gdbserver
target ops used error codes everywhere, whereas the GDB target ops didn't.

IMHO it is better to build in error handling right from the beginning, even if
the first implementation wouldn't really have any error condition.  It is much
harder to add it afterwards.  Note that those were C days and you had to
propagate unhandled errors upwards.

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