Re: [PATCH 0/3] Create arch_lwp_info class hierarchy

[Pedro Alves <palves@redhat.com>]

On 07/24/2017 11:39 AM, Simon Marchi wrote:
> I have the goal of "poisoning" the XNEW/xfree-family of functions, so
> that we catch their usages with non-POD types.  A few things need to be
> fixed in the mean time, this is one.
> 
> The common lwp code in linux-nat.c and gdbserver/linux-low.c xfrees the
> private lwp data of type arch_lwp_info.  However, that type is opaque
> from its point of view, as its defined differently in each arch-specific
> implementation.  This trips on the std::is_pod<T> check, since the
> compiler can't tell whether the type is POD or not if it doesn't know
> about it.
> 
> I think the right fix going forward is to make a hierachy out of these
> structs, so that they all inherit from a common base.  That's what patch
> 3 does.  Patches 1 and 2 simply C++ify the GDB and GDBserver lwp_info
> structures so that it's possible to use unique_ptr fields.
> 
> Simon Marchi (3):
>   gdb lwp_info: Add destructor, initialize fields, use new/delete
>   gdbserver lwp_info: Initialize fields, use new/delete
>   Create arch_lwp_info class hierarchy

Looks fine to me as well.

Let me just lay down some thoughts:

We don't really need to make these types have vtable pointers / don't
really need to polymorphic, since there's only ever going to be one
arch type in a build.  So we could instead move the arch-specific 
definitions to arch-specific headers, still name the arch-specific types
and then have linux-low.c etc. include it.  That way, we'd use
arch_lwp_info throughout just like today.  arch_lwp_info would just
be different types defined in different headers depending on arch.

I.e., e.g., in a linux-arm-low.h:

struct arch_lwp_info : public arch_lwp_info_base
{
  // arm bits.
};

and then in linux-low.h we'd have

#ifdef __arm__
# include "linux-arm-low.h"
#elif defined __i686__
# include "linux-x86-low.h"
#elif ...
...
#endif

A follow up thing that we could do is have arch_lwp_info inherit
from lwp_info and always allocate arch_lwp_info objects.

Or for clarity, rename lwp_info to lwp_info_base and make the
arch version be The lwp_info type.  I.e., e.g., in linux-arm-low.h:

struct lwp_info : public lwp_info_base
{
  // arm bits.
};

This would avoid the double/separate allocation of
lwp_info + arch_lwp_info.

Anyway, just ideas.  I'm really fine with what you have.

Thanks,
Pedro Alves