This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]