[DTrace-devel] [PATCH v3 1/3] Introduce temporary string space for string manipulation functions

[Eugene Loh]

A few questions/comments...

On 9/3/21 3:52 PM, Kris Van Hees wrote:
> Functions must request a temporary string slot and have it associated
> with their return value using a statement like:
>      dt_cg_tstring_alloc(pcb, dnp);

Just to clarify your earlier response... if a function needs scratch 
space for computation (but not for the output result), that's beyond the 
scope of this patch.  Did I get that right?

> Functions that may be receiving a temporary string as argument (which
> is any function or action that accept string arguments) must free the
> temporary string once it is no longer needed.  This should be done
> using code like:
>
>      if (dnp->dn_tstring)
>          dt_cg_tstring_free(pcb, dnp);

How about putting that dnp->dn_tstring check inside of 
dt_cg_tstring_free()?  That would save some boilerplate code.

> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> @@ -276,20 +269,25 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>   
>   	/*
>   	 * The size of the map value (a byte array) is the sum of:
> -	 *	- size of the DTrace context, rounded up to the nearest
> +	 *	- size of the DTrace machine state, rounded up to the nearest
>   	 *	  multiple of 8
>   	 *	- 8 bytes padding for trace buffer alignment purposes
>   	 *	- maximum trace buffer record size, rounded up to the
>   	 *	  multiple of 8
>   	 *	- the greater of:
>   	 *		+ the maximum stack trace size
> -	 *		+ three times the maximum string size
> +	 *		+ four times the maximum string size (incl. length)
> +	 *		  plus the maximum string size (to accomodate the BPF
> +	 *		  verifier)
>   	 */
>   	memsz = roundup(sizeof(dt_mstate_t), 8) +
>   		8 +
>   		roundup(dtp->dt_maxreclen, 8) +
>   		MAX(sizeof(uint64_t) * dtp->dt_options[DTRACEOPT_MAXFRAMES],
> -		    3 * dtp->dt_options[DTRACEOPT_STRSIZE]);
> +		    DT_TSTRING_SLOTS *
> +			(DT_STRLEN_BYTES + dtp->dt_options[DTRACEOPT_STRSIZE] +
> +		    dtp->dt_options[DTRACEOPT_STRSIZE]
> +		));

Thanks for the clarification, but I'm still a little puzzled.  Do you 
want the extra strsize overall or per slot?  The code above seems to 
have it per slot.

> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -2841,6 +2912,8 @@ dt_cg_asgn_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   			dt_cg_arglist(idp, dnp->dn_left->dn_args, dlp, drp);
>   
>   		dt_cg_store_var(dnp, dlp, drp, idp);
> +		if (dnp->dn_right->dn_kind == DT_NODE_FUNC)
> +			dt_cg_tstring_free(yypcb, dnp->dn_right);
>   	} else {

I don't understand this stuff, but the usual ->dn_tstring check is not 
performed here.  Is that okay?  Of course, no problem if the check ends 
up being moved into tstring_free().