This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Fri, 11 Aug 2017 21:13:15 +0100
- Subject: Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0204C72FC8
- References: <1500892797-7523-1-git-send-email-simon.marchi@ericsson.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