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: Simon Marchi <simon dot marchi at polymtl dot ca>
- To: Pedro Alves <palves at redhat dot com>
- Cc: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Sat, 12 Aug 2017 13:29:51 +0200
- Subject: Re: [PATCH 0/3] Create arch_lwp_info class hierarchy
- Authentication-results: sourceware.org; auth=none
- References: <1500892797-7523-1-git-send-email-simon.marchi@ericsson.com> <22f9058d-52de-293a-eef8-6af1572955d0@redhat.com>
On 2017-08-11 22:13, Pedro Alves wrote:
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.
I like the idea. I tried to do this and fell in some traps, maybe you
have some ideas about how to make it better. In linux-nat.c, for
example, I started with this:
#if defined __arm__
# include "arm-linux-nat.h"
#elif defined __i686__ || defined __x86_64__
# include "nat/x86-linux.h"
#else
# error "Missing arch-specific include."
#endif
But then I realized that many arches have linux support, but don't have
arch_lwp_info. So I changed to
...
#else
/* Define a dummy arch_lwp_info for arches that don't define one. */
struct arch_lwp_info [};
#endif
But then I realized that I forgot to include the header for s390, and
the compiler (when building for a s390 host) didn't warn me. This is
dangerous and fragile since we end up with two definitions of
arch_lwp_info (the s390 one and that fallback one), and nothing to warn
about it. So I changed it to listing explicitly the architectures that
don't defined their own arch_lwp_info:
...
#elif defined __alpha__ || defined __powerpc__ || ...
/* Define a dummy arch_lwp_info for arches that don't define one. */
struct arch_lwp_info {};
#else
# error "Missing arch-specific include."
#endif
That would work, but requires listing all the arches that need the
fallback definition of arch_lwp_info, so it gets pretty ugly.
Any idea to make this simple but safe? Otherwise, I'll just go with the
current version of the patch.
Simon