[DTrace-devel] [PATCH 3/4] Fix fbt:::return and pid:::return arg1

[Kris Van Hees]

On Wed, May 04, 2022 at 09:18:32PM -0400, Eugene Loh via DTrace-devel wrote:
> On 5/4/22 2:09 AM, Kris Van Hees wrote:
> 
> > On Wed, May 04, 2022 at 01:01:43AM -0400, eugene.loh--- via DTrace-devel wrote:
> > > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > >   /*
> > > - * Copy arguments from a dt_pt_regs structure referenced by the 'rp' argument.
> > > + * Copy arguments for entry probes.
> > > + *
> > > + * A dt_pt_regs structure is referenced by the 'rp' argument.
> > >    *
> > >    * The caller must ensure that %r7 contains the value set by the
> > >    * dt_cg_tramp_prologue*() functions.
> > >    */
> > >   void
> > > -dt_cg_tramp_copy_args_from_regs(dt_pcb_t *pcb, int rp)
> > > +dt_cg_tramp_copy_entry_args(dt_pcb_t *pcb, int rp)
> > Sorry, but no.  This function exists specifically to copy the values passed
> > for a probe firing into the mstate.  It does not matter whether this is for
> > entry probes or not.  In fact, various providers do not use entry and return
> > probes.  Let's not sacrifice the common case in favour of a specific case.
> I wanted to check in on this.  Yes we're copying args, but we get them where
> we must, whether from regs or the stack.  So to me the "from regs" seems
> misleading.  But I do not insist on fixing the name.  Just checking in.

Yes, but I couldn't come up with a better name and in a sense it is not wrong
either because even the retrieval of arguments from the stack runs through the
registers because we need to use the sp register to access the stack.  So,
even in that case the arg value is obtained based on the register set.

> Also, when we discussed dt_cg_subr_path_helper() in the past, you suggested
> naming the function narrowly, per existing usage.  If the usage would expand
> -- which it has, to dt_cg_subr_arg_to_tstring() -- we could rename it
> accordingly.  This time, we are only using the tramp-copy-args function for
> copying some entry-probe args.  So, the same rationale does not apply?  Name
> the function for its actual use:  copying function args to become probe args
> for some entry probes?  If the usage changes in the future, rename then?

Well, there are a few different things going on here.  First of all, that
function to copy args from regs was being used for more than just entry probes
prior to your proposed patches.  If your patches cause it to become used for
entry probes only at this point, then I don't think that necessitates renaming
it.  Its name still accurate reflects what it does.  Furthermore, it is a
utility function available to all providers and it does not restrict its use
for probe trampolines either by implementation or naming.

In the case of dt_cg_subr_path_helper(), usage did change towards a more
generic case, which would waarant changing the name to reflect that it is of
more generic use.  If we were to somehow change the calling code to no longer
need it for that more generic use, I wouldn't suggest renaming it back to its
old name.

Similarly, if the *_copy_args_from_regs() had been introduced just for entry
probes, it might have been named based on that.  But it was introduced in an
already more generic case, hence the name.

However, even beyond all this, the design here reflects the underlying
principle that probes are presented (in the DTrace design) as equivalent to
function calls.  The central dtrace_probe() function that implements the
generic probe execution entry point takes arguments (passed by registers and
on the stack for overflow arguments) and retrieves them indiscriminantly for
all probe calls.  This design principle is retained in the current version,
although there are some deviations where needed because of the underlying
tracing infrastructure (at the kernel level).