[DTrace-devel] [PATCH 1/3] Transition from variable-length string size to 2-byte string size

[Eugene Loh]

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

On 9/2/21 2:38 AM, Kris Van Hees wrote:
> The use of variable-length integers to store string sizes has turned
> out to prohibitively complex for the BPF verifier to handle.  In order
to -> to be (though I suspect you've already fixed that)

Also, (both subject line and commit message), the point isn't so much 
that string sizes are changing as the string-size prefixes to strings are.
> to provide string manipulation functions we have decided to transition
> away from variable-length string size prefix.
>
> This patch provides a transition to 2-byte string sizes stored ahead
> of the actual string data.  A future patch will provide a more
> permanent solution.
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -850,40 +850,31 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>   
>   		return 0;
>   	} else if (dt_node_is_string(dnp)) {
> -		dt_ident_t	*idp;
>   		uint_t		size_ok = dt_irlist_label(dlp);
>   		int		reg = dt_regset_alloc(drp);
>   
>   		off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, kind,
>   				 size, 1, pfp, arg);
>   
> -		/* Retrieve the length of the string.  */
> +		/*
> +		 * Retrieve the length of the string, and adjust for the
> +		 * terminating NUL byte and the length prefix.
> +		 */
>   		dt_cg_strlen(dlp, drp, reg, dnp->dn_reg);
> -
> -		if (dt_regset_xalloc_args(drp) == -1)
> -			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> -
> -		/* Determine the number of bytes used for the length. */
> -		emit(dlp,  BPF_MOV_REG(BPF_REG_1, reg));
> -		idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_vint_size");
> -		assert(idp != NULL);
> -		dt_regset_xalloc(drp, BPF_REG_0);
> -		emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> -
> -		/* Add length of the string (adjusted for terminating byte). */
> -		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, reg, 1));
> -		emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_0, reg));
> -		dt_regset_free(drp, reg);
> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, reg, 1 + DT_STRLEN_BYTES));
>   
>   		/*
>   		 * Copy string data (varint length + string content) to the
Strike the "varint" reference.  Anticipating future changes, one could 
drop the parenthetical comment altogether.
>   		 * output buffer at [%r9 + off].  The amount of bytes copied is
>   		 * the lesser of the data size and the maximum string size.
>   		 */
> +		if (dt_regset_xalloc_args(drp) == -1)
> +			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
>   		emit(dlp,  BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
>   		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> -		emit(dlp,  BPF_MOV_REG(BPF_REG_2, BPF_REG_0));
> -		dt_regset_free(drp, BPF_REG_0);
> +		emit(dlp,  BPF_MOV_REG(BPF_REG_2, reg));
> +		dt_regset_free(drp, reg);
>   		emit(dlp,  BPF_BRANCH_IMM(BPF_JLT, BPF_REG_2, size, size_ok));
>   		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, size));
>   		emitl(dlp, size_ok,
Strictly speaking, I suppose the way the size is clipped is a bug. It 
keeps room for the length prefix, at the expense of dropping the last 
two chars.  In light of anticipated future changes, however, I don't 
think this is worth fixing.