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

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

On 2017-08-14 13:53, Pedro Alves wrote:
> On 08/12/2017 12:29 PM, Simon Marchi wrote:
> 
>> 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.
> 
> Not sure whether that's really such a bad problem.  It seems like the
> sort of thing that'd crash quite visibly/quickly if you actually try to
> run the resulting binary.  It's not like we're adding ports
> every day.  :-)

How would it crash visibly?  I thought it would most likely crash or 
corrupt memory seemingly randomly.  That's irrelevant because of you 
suggestion below, but I'm asking just for the sake of the discussion.

>> 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.
> 
> There's a 3rd, much simpler option.
> 
> The problem we ran into is one of blurred division of responsibility: 
> the
> arch-specific code is responsible for allocating the arch-specific
> arch_lwp_info, which it must be, because it's not possible to allocate 
> an
> incomplete type (must know its size), but we free the object in common 
> code,
> where the type is opaque.
> 
> We can solve the original xfree poisoning problem by simply making the
> arch-specific code responsible for releasing arch_lwp_info too, where 
> the
> type is known, just like it is responsible for allocating it.
> 
> See below a proof of concept doing that for the x86 port.  Making other
> archs do the same should be trivial.  Using new/delete/cdtors/in-class
> initialization for arch_lwp_info would be a separate, orthogonal change
> (and so would class-ification of linux_nat_new_thread, 
> linux_target_ops,
> etc.).

Yes, that makes sense.  I'll try that.  Thanks for the prototype.

Simon