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

[Simon Marchi <simon.marchi@polymtl.ca>]

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