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

[Mark Kettenis <kettenis@gnu.org>]

   Date: Sun, 12 Dec 2004 12:51:29 -0500
   From: Andrew Cagney <cagney@gnu.org>

   > -LONGEST 
   > -ia64_linux_xfer_unwind_table (struct target_ops *ops,
   > -			      enum target_object object,
   > -			      const char *annex,
   > -			      void *readbuf, const void *writebuf,
   > -			      ULONGEST offset, LONGEST len)
   > +LONGEST (*saved_xfer_partial) (struct target_ops *, enum target_object,
   > +			       const char *, void *, const void *,
   > +			       ULONGEST, LONGEST);
   > +
   > +static LONGEST 
   > +ia64_linux_xfer_partial (struct target_ops *ops,
   > +			 enum target_object object,
   > +			 const char *annex,
   > +			 void *readbuf, const void *writebuf,
   > +			 ULONGEST offset, LONGEST len)
   >  {
   > -  return syscall (__NR_getunwind, readbuf, len);
   > +  if (object == TARGET_OBJECT_UNWIND_TABLE && writebuf == NULL && offset == 0)
   > +    return syscall (__NR_getunwind, readbuf, len);
   > +
   > +  return saved_xfer_partial (ops, object, annex, readbuf, writebuf,
   > +			     offset, len);
   > +}

   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?

   +#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.

I'm working on something that will help resolving this problem, by
re-adding PT_READ_U support back into inf-ptrace.c in a clean fashion.
In the meantime, this hack allows us to go forwards.

   +/* 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.

   > 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.)

   > +  /* FIXME drow/2004-12-04: For now, these functions must keep the standard
   > +     names because only some GNU/Linux targets use inf-ptrace.o.  When all
   > +     have been converted, these functions can be renamed and made static.  */

   Is this still true?  I thought the patch was so large because it touched 
   all the linux targets.

Yes it is true.  Daniels patch touches most (if not all) Linux
targets, but doesn't resolve all the issues.  Some more work is
necessary.  However, most of that work is non-mechanical.  Given the
fact that many Linux targets are not actively maintained making the
acceptance of this patch depend on this patch depend on that work
would probably mean it will be dropped.

Mark