[DTrace-devel] [PATCH 2/2] Add support for substr() subroutine

[Eugene Loh]

For now, my main comment is that it would be helpful (for review but 
someday also for our users) if the semantics we're supporting would be 
described in the patch (commit msg or even better in the code, like in 
bpf/substr.S).  The exotic cases of idx and cnt do not seem to be 
described in the documentation (which is another problem).  High-level 
comments in bpf/substr.S saying what is intended would help someone 
understand whether the code is operating correctly.

Meanwhile, ...

On 9/2/21 11:34 AM, Kris Van Hees wrote:
> diff --git a/bpf/substr.S b/bpf/substr.S
> new file mode 100644
> index 00000000..9e032441
> --- /dev/null
> +++ b/bpf/substr.S
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#define DT_STRLEN_BYTES		2
> +
> +#define BPF_FUNC_probe_read	4
> +
> +/*
> + * void dt_substr(char *dst, const char *src, int32_t idx, int32_t cnt,
> + *		  uint64_t argc)
> + */
> +	.text
> +	.align	4
> +	.global	dt_substr
> +dt_substr :
> +	stxdw	[%fp+-8], %r1		/* Spill dst */

AFAICT, it is unnecessary to spill dst to the stack.  It lives in %r9 
anyhow.

> +	mov	%r9, %r1
> +	stxdw	[%fp+-16], %r2		/* Spill src */
> +	lsh	%r3, 32			/* Sign-extend idx */
> +	arsh	%r3, 32
> +	mov	%r6, %r3		/* %r6 = idx */
> +
> +	lddw	%r8, STRSZ		/* %r8 = STRSZ (temporary) */
> +	jgt	%r5, 2, .Lhave_cnt
> +	mov	%r4, %r8		/* cnt = STRSZ */
> +.Lhave_cnt:

This "argc" interface strikes me as clumsy.  Just have 
dt_cg_subr_substr() figure cnt out if the user does not supply a value.  
That simplifies both it (the caller) and this code (the callee).

> +	lsh	%r4, 32			/* Sign-extend cnt */
> +	arsh	%r4, 32
> +	mov	%r7, %r4		/* %r7 = cnt */
> +
> +	/*
> +	 * Get the source string length and validate it.  If the length is 0,
> +	 * the result is the empty string.  If the length is greater than the
> +	 * maximum string length (STRSZ), cap it at that value.
> +	 */
> +	ldxdw	%r1, [%fp+-16]
> +	call	dt_strlen		/* len = dt_strlen(src) */
> +	jeq	%r0, 0, .Lempty
> +	mov	%r1, %r8		/* %r1 = STRSZ */

I'm confused why you need %r1=STRSZ (namely, why you need the first 
instruction in Ladjust_cnt), but I've seen crazier stuff with the BPF 
verifier so I cannot claim this is unneeded.  Does look odd, though.

> +	jle	%r0, %r8, .Llen_ok
> +	mov	%r0, %r8		/* len = STRSZ */
> +.Llen_ok:
> +	mov	%r8, %r0		/* %r8 = len */

You don't need the value to be also in %r0.  So how about replacing the 
above four lines with

         jgt     %r0, %r8, .Lfoobar
         mov     %r8, %r0
     .Lfoobar:

or even just
         jgt     %r0, %r8, 1
         mov     %r8, %r0


> +
> +	jsge	%r6, 0, .Ladjust_cnt
> +
> +.Lidx_neg:
> +	add	%r6, %r8		/* idx += len */
> +	jsge	%r6, 0, .Ladjust_cnt
> +	mov	%r0, 0
> +	sub32	%r0, %r6		/* neg messes up the verifier */
> +	jsle	%r7, %r0, .Ladjust_cnt
> +	add32	%r7, %r6		/* cnt += idx */
> +	mov	%r6, 0			/* idx = 0 */
> +
> +.Ladjust_cnt:
> +	jsge	%r6, %r1, .Lempty
> +	jsge	%r6, %r8, .Lempty
> +	jslt	%r6, 0, .Lempty
> +	jsge	%r7, 0, .Lcnt_pos
> +	mov	%r0, %r8
> +	sub32	%r0, %r6
> +	add32	%r7, %r0		/* cnt += len - idx */
> +	lsh	%r7, 32
> +	arsh	%r7, 32
> +	jsle	%r7, 0, .Lempty
> +.Lcnt_pos:
> +	sub	%r1, %r6
> +	jsge	%r1, %r7, .Lcopy
> +	mov	%r7, %r1		/* cnt = STRSZ - idx */
> +
> +.Lcopy:
> +	ldxdw	%r8, [%fp+-16]
> +	add	%r8, DT_STRLEN_BYTES
> +	add	%r8, %r6		/* %r8 = src + DT_STRLEN_BYTES + idx */
> +
> +	mov	%r1, %r7
> +	ldxdw	%r2, [%fp+-8]
> +	mov	%r9, %r2		/* %r9 = dst */
> +	call	dt_strlen_store		/* plen = dt_strlen_store(cnt, dst) */
> +	add	%r9, DT_STRLEN_BYTES	/* %r9 = dst + DT_STRLEN_BYTES */
> +
> +	mov	%r1, %r9
> +	mov	%r2, %r7
> +	mov	%r3, %r8
> +	call	BPF_FUNC_probe_read
> +
> +	add	%r9, %r7
> +	stb	[%r9+0],0
> +	exit
> +
> +.Lempty:
> +	/* Store the empty string in the destination. */
> +	stb	[%r9+0], 0
> +	stb	[%r9+1], 0
> +	exit

I guess .Lempty is okay.  It might be cleaner to use dt_strlen_store(), 
but I suppose all that is going away soon enough.

Do you also want a terminating NULL?


> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index b48b9f7b..749324e4 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3149,6 +3149,63 @@ dt_cg_subr_strjoin(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>   	TRACE_REGSET("    subr-strjoin:End  ");
>   }
>   
> +static void
> +dt_cg_subr_substr(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dt_node_t	*str = dnp->dn_args;
> +	dt_node_t	*idx = str->dn_list;
> +	dt_node_t	*cnt = idx->dn_list;
> +	dt_ident_t	*idp;
> +
> +	TRACE_REGSET("    subr-substr:Begin");
> +
> +	dt_cg_node(str, dlp, drp);
> +	dt_cg_check_notnull(dlp, drp, str->dn_reg);
> +	dt_cg_node(idx, dlp, drp);
> +	if (cnt != NULL)
> +		dt_cg_node(cnt, dlp, drp);
> +
> +	/*
> +	 * Allocate the result register and associate it with a temporary
> +	 * string slot.
> +	 */
> +	dnp->dn_reg = dt_regset_alloc(drp);
> +	if (dnp->dn_reg == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +	dt_node_tstring(dnp, dt_cg_tstring_alloc());
> +
> +        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));

Tabs instead of spaces?

> +
> +	if (dt_regset_xalloc_args(drp) == -1)
> +		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> +
> +	emit(dlp, BPF_MOV_REG(BPF_REG_1, dnp->dn_reg));
> +	emit(dlp, BPF_MOV_REG(BPF_REG_2, str->dn_reg));
> +	dt_regset_free(drp, str->dn_reg);
> +	if (str->dn_tstring)
> +		dt_cg_tstring_free(str->dn_tstring->dn_value);
> +	emit(dlp, BPF_MOV_REG(BPF_REG_3, idx->dn_reg));
> +	dt_regset_free(drp, idx->dn_reg);
> +	if (cnt != NULL) {
> +		emit(dlp, BPF_MOV_REG(BPF_REG_4, cnt->dn_reg));
> +		dt_regset_free(drp, cnt->dn_reg);
> +		emit(dlp, BPF_MOV_IMM(BPF_REG_5, 3));
> +	} else {
> +		emit(dlp, BPF_MOV_IMM(BPF_REG_4, 0));
> +		emit(dlp, BPF_MOV_IMM(BPF_REG_5, 2));
> +	}

Again, drop the %r5 stuff.  Just, for cnt==NULL, use 
dt_options[DTRACEOPT_STRSIZE] instead of 0.

> +	dt_regset_xalloc(drp, BPF_REG_0);
> +	idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_substr");
> +	assert(idp != NULL);
> +	emite(dlp,  BPF_CALL_FUNC(idp->di_id), idp);
> +	dt_regset_free_args(drp);
> +	dt_regset_free(drp, BPF_REG_0);
> +
> +	TRACE_REGSET("    subr-substr:End  ");
> +}