[DTrace-devel] [PATCH 4/4] Make sure assignment expressions work correctly with tstrings

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with minor comments.

On 10/30/21 1:41 AM, Kris Van Hees wrote:
> The node kinds that can hold a tstring has been expanded from just
> DT_NOT_DUNC to DT_NODE_OP1, DT_NODE_OP2, DT_NODE_OP3, DT_NODE_DEXPR.

s/DT_NOT_DUNC/DT_NODE_FUNC/

> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index ddc4c0d7..0196b114 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2303,8 +2310,13 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
>   	}
>   }
>   
> +/*
> + * dnp = node of the assignment
> + *   dn_left = identifier node for the destination (idp = identifier)
> + *   dn_right = value to store
> + */
>   static void
> -dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
> +dt_cg_store_var(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>   		dt_ident_t *idp)
>   {
>   	uint_t	varid;
> @@ -2327,7 +2339,7 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>   			emit(dlp, BPF_LOAD(BPF_DW, reg, reg, DCTX_GVARS));
>   
>   		/* store by value or by reference */
> -		if (src->dn_flags & DT_NF_REF) {
> +		if (dnp->dn_flags & DT_NF_REF) {
>   			size_t		srcsz;
>   
>   			emit(dlp, BPF_ALU64_IMM(BPF_ADD, reg, idp->di_offset));
> @@ -2337,23 +2349,22 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>   			 * the lesser of the size of the identifier and the
>   			 * size of the data being copied in.
>   			 */
> -			srcsz = dt_node_type_size(src->dn_right);
> -			if (dt_node_is_string(src))
> +			srcsz = dt_node_type_size(dnp->dn_right);
> +			if (dt_node_is_string(dnp))
>   				srcsz += DT_STRLEN_BYTES;
>   			size = MIN(srcsz, idp->di_size);
>   
> -			dt_cg_memcpy(dlp, drp, reg, src->dn_reg, size);
> +			dt_cg_memcpy(dlp, drp, reg, dnp->dn_reg, size);
>   		} else {
>   			size = idp->di_size;
>   
>   			assert(size > 0 && size <= 8 &&
>   			       (size & (size - 1)) == 0);
>   
> -			emit(dlp, BPF_STORE(ldstw[size], reg, idp->di_offset, src->dn_reg));
> +			emit(dlp, BPF_STORE(ldstw[size], reg, idp->di_offset, dnp->dn_reg));
>   		}
>   
>   		dt_regset_free(drp, reg);
> -		dt_cg_tstring_free(yypcb, src);
>   		return;
>   	}

Did you also want s/srcsz/dnpsz/?  (Of course "it does not matter.")

> @@ -2370,7 +2381,7 @@ dt_cg_store_var(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp,
>   	 * the disassembler expects this sequence in support for annotating
>   	 * the disassembly with variables names.
>   	 */
> -	emit(dlp,  BPF_MOV_REG(BPF_REG_2, src->dn_reg));
> +	emit(dlp,  BPF_MOV_REG(BPF_REG_2, dnp->dn_reg));
>   	emit(dlp,  BPF_MOV_IMM(BPF_REG_1, varid));
>   	dt_regset_xalloc(drp, BPF_REG_0);
>   	emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> @@ -2830,6 +2841,35 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   
>   	emitl(dlp, lbl_post,
>   		   BPF_NOP());
> +
> +	/*
> +	 * Strings complicate things a bit because dn_left and dn_right might
> +	 * actually be temporary strings (tstring) *and* in different slots.
> +	 * We need to allocate a new tstring to hold the result, and copy the
> +	 * value into the new tstring (and free any tstrings in dn_left and
> +	 * dn_right).
> +	 */
> +	if (dt_node_is_string(dnp)) {
> +		/*
> +		 * At this point, dnp->dn_reg holds a pointer to the string we
> +		 * need to copy.  But we want to copy it into a tstring which
> +		 * location is to be stored in dnp->dn_reg.  So, we need to
> +		 * shuffle things a bit.
> +		 */
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> +		dt_cg_tstring_alloc(yypcb, dnp);
> +
> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_DCTX));
> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, dnp->dn_reg, DCTX_MEM));
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
> +
> +		dt_cg_memcpy(dlp, drp, dnp->dn_reg, BPF_REG_0,
> +			     DT_STRLEN_BYTES +
> +			     yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE]);
> +
> +		dt_cg_tstring_free(yypcb, dnp->dn_left);
> +		dt_cg_tstring_free(yypcb, dnp->dn_right);
> +	}
>   }

Would it not be safer to allocate a register than to hardwire %r0?

Should the memcpy not include an extra byte for the NULL terminator?

> diff --git a/test/unittest/codegen/tst.tstring_asgn_expr.d b/test/unittest/codegen/tst.tstring_asgn_expr.d
> new file mode 100644
> index 00000000..6b0d7d73
> --- /dev/null
> +++ b/test/unittest/codegen/tst.tstring_asgn_expr.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 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.
> + */
> +
> +/*
> + * DESC
> + */

DESC?

> diff --git a/test/unittest/codegen/tst.tstring_ternary.d b/test/unittest/codegen/tst.tstring_ternary.d
> new file mode 100644
> index 00000000..c59d8c2e
> --- /dev/null
> +++ b/test/unittest/codegen/tst.tstring_ternary.d
> @@ -0,0 +1,28 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 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.
> + */
> +
> +/*
> + * DESC
> + */
DESC?