[DTrace-devel] [PATCH 4/6] cg: add argument remapping in the trampoline

[Kris Van Hees]

On Fri, Oct 18, 2024 at 08:58:06PM +0100, Nick Alcock wrote:
> Before now, argument remapping was restricted to the args[] array, and was
> implemented in dt_cg_array_op in the same place where we decide whether
> to automatically copyin() userspace strings.

remapping -> mapping (everywhere in this patch, and similarly, remap -> map)

> But existing DTrace testcases suggest that remapping is also applied to argN
> for USDT, and it would definitely be more *convenient* if it were (so that
> argN and arg[N] always contain the same content, and only their types
> differ).  Add a new function dt_cg_tramp_remap_args(), which can be
> called from trampolines to apply remappings (destructively, but it keeps a
> copy of the old args in the save area so you can unapply them if you need
> to).  No existing providers do any remapping, so this is not yet called, but
> it's about to be.

I think that the preceding paragraph makes some wild assumptions that are not
really in line with what the anticipated behaviour is.  For one, there is only
a single existing DTrace testcase that involves argument mapping, and it is
uncertain whether that reflects a conscious design decision.  It is certainly
not consistent with the SDT implementation where argN reflects native values
and args[] is to be used for translated arguments.

The documentation states:

  args[]
  The typed arguments, if any, to the current probe. The args[] array is
  accessed using an integer index, but each element is defined to be the type
  corresponding to the given probe argument. For information about any typed
  arguments, use dtrace -l with the verbose option -v and check Argument Types.

  int64_t arg0, ..., arg9

  The first ten input arguments to a probe, represented as raw 64-bit integers.
  Values are meaningful only for arguments defined for the current probe.

Now, granted, this is a bit vague and does not entirely apply to SDT probes
because there we actually have types for probe arguments anyway.  But I think
a valid case can be made for argN to *always* be a raw 64-bit integer value
(which you could explicitly cast to the type it really is) and to *always* be
the raw arguments of the probe, i.e. what traditionally was passed as the
argument to the dtrace_probe() function.  That would correspond to the native
arguments for SDT and USDT.

Anyway, rather than making assumptions on what would be better or implying
that there is sufficient evidence for one versus the other implementation,
you should simply document that a test in the testsuite implies that the
expectation is that argNM and args[N] are the same for USDT probes, and that
you are adding a function to do apply this mapping, for use in trampoline code.

I also disagree that the function performs the saving of arguments itself.
I think that the caller ought to take care of that, because the caller has a
much better idea about when/if to save arguments, and when/if to restore them.

> The remapping info is still propagated up out of per-provider state so that
> the parser can use it to tell what types the remapped arguments are and
> to apply array-op remapping: users of dt_cg_tramp_remap_args() should set
> the probe remapping to 1:1 and shuffle the native types correspondingly,
> to match what dt_cg_tramp_remap_args() has done (in effect, calling it has
> moved the native types around).

I would drop this paragraph.  It is unnecessary and speculative at best about
what other code should or should not do.

However, I think it might be a good idea to have this function actually rewrite
the mapping data to become a pure i -> i mapping.  That seems appropriate
given that it applying the mapping and thereby should ensure that the mapping
will not be applied again by any other code.

> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>  libdtrace/dt_cg.c | 35 ++++++++++++++++++++++++++++++++++-
>  libdtrace/dt_cg.h |  1 +
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 39c27ab0ec0b..9709667d97fb 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -831,6 +831,34 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
>  	}
>  }
>  
> +/*
> + * Remap arguments according to the array of remappings given.  The original

Remap -> Map
remapping -> mapping

> + * arguments are overwritten (but saved).

Caller should save.

> + *
> + * The caller must ensure that %r7 contains the value set by the
> + * dt_cg_tramp_prologue*() functions.
> + */
> +void
> +dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings)

remap -> map
remappings -> mapping (or argmap)
nremappings -> mapc (or xargc)
> +{
> +	dt_irlist_t	*dlp = &pcb->pcb_ir;
> +	int		i;
> +
> +	/*
> +	 * Work from saved args so we don't need to worry about overwriting regs
> +	 * we're about to remap.
> +	 */
> +	dt_cg_tramp_save_args(pcb);

Caller should save.

> +
> +	for (i = 0; i < nremappings; i++) {
> +		if (remappings[i] != i) {
> +			dt_dprintf("Remap: loading from %i, storing to %i\n", remappings[i], i);

I would remove this debugging statement.  We don't really report debugging data
at this level anywhere else, really.

> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(remappings[i])));
> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));

I would add:
			argmap[i] = i;
to convert the mapping into a pure i -> i mapping, so no other code will apply
argument mapping again.

> +		}
> +	}
> +}
> +
>  typedef struct {
>  	dt_irlist_t	*dlp;
>  	dt_activity_t	act;
> @@ -5060,7 +5088,12 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  	 * array index according to the static argument mapping (if any),
>  	 * unless the argument reference is provided by a dynamic translator.
>  	 * If we're using a dynamic translator for args[], then just set dn_reg
> -	 * to an invalid reg and return: DIF_OP_XLARG will fetch the arg later.
> +	 *
> +	 * If dt_cg_tramp_remap_args is in use, you should apply the remapping
> +	 * at the probe_info stage, since the effective "native" arg positions
> +	 * will be changed.
> +	 *
> +	 * If this is a userland variable, note that we need to copy it in.

Not needed.  The original comment is sufficient.

>  	 */
>  	if (idp->di_id == DIF_VAR_ARGS) {
>  		if ((idp->di_kind == DT_IDENT_XLPTR ||
> diff --git a/libdtrace/dt_cg.h b/libdtrace/dt_cg.h
> index 0a7c7ba6a8c5..4c6d0f5dac6a 100644
> --- a/libdtrace/dt_cg.h
> +++ b/libdtrace/dt_cg.h
> @@ -34,6 +34,7 @@ extern void dt_cg_tramp_get_var(dt_pcb_t *pcb, const char *name, int isstore,
>  extern void dt_cg_tramp_del_var(dt_pcb_t *pcb, const char *name);
>  extern void dt_cg_tramp_save_args(dt_pcb_t *pcb);
>  extern void dt_cg_tramp_restore_args(dt_pcb_t *pcb);
> +extern void dt_cg_tramp_remap_args(dt_pcb_t *pcb, uint8_t *remappings, size_t nremappings);

dt_cg_tramp_map_args

>  extern void dt_cg_tramp_call_clauses(dt_pcb_t *pcb, const dt_probe_t *prp,
>  				     dt_activity_t act);
>  extern void dt_cg_tramp_return(dt_pcb_t *pcb);
> -- 
> 2.46.0.278.g36e3a12567
>