[DTrace-devel] [PATCH 2/6] Support storing variables by reference

[Kris Van Hees]

Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>

... with changes as indicated below

On Fri, Mar 19, 2021 at 01:45:30PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Initial support for storing variables by reference -- that is,
> by copying them via the dt_memcpy() routine.  Ultimately, this
> support will have to be expanded to include kernel and userspace
> memory.
> 
> Note that dt_memcpy() is currently limited to 256 bytes.
> 
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  bpf/Build                               |  3 +-
>  libdtrace/dt_cg.c                       | 43 +++++++++++++++++--------
>  test/unittest/struct/tst.StructInside.d |  3 +-
>  3 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/bpf/Build b/bpf/Build
> index f8d736e7..5133a2f2 100644
> --- a/bpf/Build
> +++ b/bpf/Build
> @@ -25,8 +25,9 @@ bpf_dlib_SOURCES = \
>  	agg_lqbin.c agg_qbin.c \
>  	get_bvar.c \
>  	get_tvar.c set_tvar.c \
> +	memcpy.c \
>  	probe_error.c \
> -	memcpy.c strnlen.c
> +	strnlen.c
>  
>  bpf-check: $(objdir)/include/.dir.stamp
>  	$(BPFC) $(BPFCPPFLAGS) $(bpf_dlib_CPPFLAGS) $(BPFCFLAGS) -S \
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 9243f0a7..ab3fe761 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -656,6 +656,26 @@ dt_cg_fill_gap(dt_pcb_t *pcb, int gap)
>  		emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, off, 0));
>  }
>  
> +static void
> +dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
> +{
> +	dt_ident_t *idp;
> +
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_1, dst));
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_2, src));
> +	emit(dlp,  BPF_MOV_IMM(BPF_REG_3, size));
> +	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_memcpy");
> +	assert(idp != NULL);
> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> +	dt_regset_free_args(drp);
> +	/* FIXME: check BPF_REG_0 for error? */
> +	dt_regset_free(drp, BPF_REG_0);
> +}
> +
>  static void
>  dt_cg_spill_store(int reg)
>  {
> @@ -1851,19 +1871,9 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
>  	else
>  		size = dt_node_type_size(src);
>  
> -	if (src->dn_flags & DT_NF_REF) {
> -#ifdef FIXME
> -		if ((reg = dt_regset_alloc(drp)) == -1)
> -			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> -		dt_cg_setx(dlp, reg, size);
> -		instr = DIF_INSTR_COPYS(src->dn_reg, reg, dst->dn_reg);
> -		dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> -		dt_regset_free(drp, reg);
> -#else
> -		xyerror(D_UNKNOWN, "internal error -- cg cannot store "
> -			"values passed by ref\n");
> -#endif
> -	} else {
> +	if (src->dn_flags & DT_NF_REF)
> +		dt_cg_memcpy(dlp, drp, dst->dn_reg, src->dn_reg, size);
> +	else {
>  		uint8_t	sz;
>  
>  		if (dst->dn_flags & DT_NF_BITFIELD)
> @@ -1919,7 +1929,12 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>  			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>  		emit(dlp, BPF_LOAD(BPF_DW, reg, BPF_REG_FP, DT_STK_DCTX));
>  		emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_GVARS));
> -		emit(dlp, BPF_STORE(BPF_DW, reg, idp->di_offset, src->dn_reg));
> +		if ((src->dn_flags & DT_NF_REF) == 0)
> +			emit(dlp, BPF_STORE(BPF_DW, reg, idp->di_offset, src->dn_reg));
> +		else {
> +			emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
> +			dt_cg_memcpy(dlp, drp, reg, src->dn_reg, idp->di_size);
> +		}

I would favour reversing the cases here, to avoid the == 0 (or what would have
been better, a negation), so:

		if (src->dn_flags & DT_NF_REF) {
			emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
			dt_cg_memcpy(dlp, drp, reg, src->dn_reg, idp->di_size);
		} else
			emit(dlp, BPF_STORE(BPF_DW, reg, idp->di_offset, src->dn_reg));

>  		dt_regset_free(drp, reg);
>  		return;
>  	}
> diff --git a/test/unittest/struct/tst.StructInside.d b/test/unittest/struct/tst.StructInside.d
> index 71b2a839..2e1a8218 100644
> --- a/test/unittest/struct/tst.StructInside.d
> +++ b/test/unittest/struct/tst.StructInside.d
> @@ -1,10 +1,9 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
>   * Licensed under the Universal Permissive License v 1.0 as shown at
>   * http://oss.oracle.com/licenses/upl.
>   */
> -/* @@xfail: dtv2 */
>  
>  /*
>   * ASSERTION:
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel