Re: [RFC] Use target vector inheritance for GNU/Linux

[Andrew Cagney <cagney@gnu.org>]

Mark Kettenis wrote:

>    Can you rename saved_xfer_partial to super_xfer_partial_hack and add a 
>    FIXME.  It should be calling super.xfer_partial but that's not available :-(
> 
> Can you explain what you're hinting at here Andrew?  What makes this
> specific saved_xfer_partial so different from the other saved_xxx
> instances that the patch introduces?

Nothing?  Changing those to be consistent with this would be a logical 
next step.

>    +#ifndef FETCH_INFERIOR_REGISTERS
>    +
>    +/* Fetch register REGNUM from the inferior.  */
>    +
>    +static void
>    +fetch_register (int regnum)
>    +{
> 
>    Why is this wrapped in in an #ifdef?
> 
> Some of the Linux target still need the crufty old code to read
> registers using PTRACE_PEEKUSR.  The new inf-ptrace.c doesn't provide
> that functionality, so I guess Daniel inlined that bit of code here.
> This is related to the FIXME below, and of course only temporary.

So a fixme or other comment needs to be added at the point of this macro?

>    +/* Create a generic GNU/Linux target vector.  If T is non-NULL, base
>    +   the new target vector on it.  */
>    +
>    +struct target_ops *
>    +linux_target (struct target_ops *t)
> 
>    Can this be renamed to inf_linux_target (to be consistent with the other 
>    inf_*_target() methods?
> 
> Apparently I don't agre with this since I already introduced
> i386bsd_target and sparc_target; linux_target is consistent with that.

You also added inf_ttrace_target.

>    > A new function, linux_target, is added to linux-nat.c.  Then any GNU/Linux
>    > target can call it, and pass the result to add_target - after specializing
>    > whatever methods it needs to.  Sometimes it's necessary to specialize a
>    > method between inf_ptrace_target and linux_target, so it accepts an optional
>    > argument.  This wouldn't be necessary if all target methods took a
>    > target_ops parameter, so they could call the overridden method.
> 
>    As in this?
> 
>    > +void
>    > +_initialize_i386_linux_nat (void)
>    > +{
>    > +  struct target_ops *t = inf_ptrace_target ();
>    > +
>    > +  /* Override the default ptrace resume method.  */
>    > +  t->to_resume = i386_linux_resume;
>    > +
>    > +  /* Fill in the generic GNU/Linux methods.  */
>    > +  t = linux_target (t);
> 
>    which is violating the inheritance structure.  Can you instead add a 
>    one-of method (deprecated_set_super_linux_resume?) to handle this case? 
>      We can then see about fixing the problem (I'm left wondering if that 
>    method is still needed).
> 
> No it isn't.  At a very low level, all Linux ports are slightly
> different.  Most ports will need to adjust the generic ptrace target
> before it can be inherited by the generic Linux target.  In fact I
> think that when the FETCH_INFERIOR_REGISTERS issue above is sorted
> out, you'll see that *all* Linux ports will need to do this trick of
> adjusting the ptrace target before passing it to linux_target().
> 
> (And yes, I'm fairly certain the method is still needed.  While the
> problem may have been fixed in recent kernels, there are many older
> Linux kernels out there.)

You're on the right track, however the inheritance structure that the C 
code is trying to mimic is:

i386LinuxInferior IS-A LinuxInferior IS-A PtraceInferior

with the i386LinuxInferior.resume method overriding LinuxInferior.resume 
(which overrid PtraceInferior.resume); and not:

LinuxInferior IS-A i386LinuxInferior IS-A PtraceInferior

That the current inferior code doesn't facilitate this is a recognized 
problem, but not one that we should get hung-up over.  Hence my 
suggestion of deprecated_set_super_linux_resume as a workaround until 
that is fixed.

Andrew