Re: [DTrace-devel] [PATCH v4 3/5] cg: add argument mapping in the trampoline

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Nov 01, 2024 at 03:57:10PM +0000, Nick Alcock via DTrace-devel wrote:
> Before now, argument mapping 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.
> 
> But existing DTrace testcases suggest that arg mapping is also applied to
> argN for USDT, even though this is inconsistent with SDT and arguably less
> flexible than having argN be the unmapped arguments in all cases.  Add a new
> function dt_cg_tramp_map_args(), which can be called from trampolines to
> apply mappings (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 mapping, so this is not yet called, but it's about to be.
> 
> Signed-off-by: Nick Alcock <nick.alcock@oracle.com>
> ---
>  libdtrace/dt_cg.c | 36 +++++++++++++++++++++++++++++++++++-
>  libdtrace/dt_cg.h |  1 +
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 39c27ab0ec0b..b90757bcb5cb 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -831,6 +831,35 @@ dt_cg_tramp_restore_args(dt_pcb_t *pcb)
>  	}
>  }
>  
> +/*
> + * Move arguments according to the mappings in the array of arguments given.
> + * The original arguments are overwritten if necessary (but saved).  In effect,
> + * this changes the native arguments: the caller should adjust the mappings
> + * array accordingly, and shuffle the native arg types to match.
> + *
> + * The caller must ensure that %r7 contains the value set by the
> + * dt_cg_tramp_prologue*() functions.
> + */
> +void
> +dt_cg_tramp_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs)
> +{
> +	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 map into different positions.
> +	 */
> +	dt_cg_tramp_save_args(pcb);

I still maintain that this should be done in the caller.  (See patch 4/5 for
more info on that.)  As mentioned before, you can mention in the comment before
this function that the caller must save the arguments, just like you already
mention that %r7 must contain a value set by dt_cg_tramp_prologue*().

> +
> +	for (i = 0; i < nargs; i++) {
> +		if (args[i].mapping != i) {
> +			emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_7, DMST_ORIG_ARG(args[i].mapping)));
> +			emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(i), BPF_REG_0));
> +		}
> +	}
> +}
> +
>  typedef struct {
>  	dt_irlist_t	*dlp;
>  	dt_activity_t	act;
> @@ -5060,7 +5089,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.

You dropped the end of a sentence without replacing it with anything?

> +	 *
> +	 * If dt_cg_tramp_map_args is in use, you should apply the mapping at
> +	 * the probe_info stage, since the effective "native" arg positions will
> +	 * be changed.

This does not belong here.  If anything, clauses get compiled separete from
trampolines and a given clause (compiled once) may get used in more than one
BPF program, i.e. it may get associated with more than one trampoline.  So
you can't really depend on what a specific trampoline does when generating code
for a clause.

> +	 *
> +	 * If this is a userland variable, note that we need to copy it in.
>  	 */
>  	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..fb26c125adbc 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_map_args(dt_pcb_t *pcb, dt_argdesc_t *args, size_t nargs);
>  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
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel