[DTrace-devel] [PATCH 4/4] Remove the string length prefix

[Eugene Loh]

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

First (and I think this could have been done a while ago), shouldn't the 
dt_strlen() in dt_string.[c|h] be removed?

On 1/26/22 11:29 AM, Kris Van Hees via DTrace-devel wrote:
> This is the final stage of discontinuing the string length prefix code.
> It removes the actual storage allocation for the 2-byte string length
> prefix, and updates all code that adjusted pointers because of the
> prefix.  The dt_strlen_store() function is gone.
>
> This change affects the offset calculation for variab;e storage when a

variab;e

> string is involved.  Test results for gvar/tst.undecl-offset.r are
> updated to reflect this.
>
> diff --git a/bpf/strjoin.S b/bpf/strjoin.S
> @@ -22,43 +20,26 @@ dt_strjoin:
>   	lddw	%r6, STRSZ
>   	add	%r6, 1			/* cnt = STRSZ + 1 */
>   
> -	add	%r1, DT_STRLEN_BYTES	/* dst is in %r1 already */
>   	mov	%r2, %r6
>   	mov	%r3, %r7
> -	add	%r3, DT_STRLEN_BYTES
> -	call	BPF_FUNC_probe_read_str	/*
> -					 * rc = probe_read_str(
> -					 *	  &dst[DT_STRLEN_BYTES],
> -					 *	  cnt,
> -					 *	  &s1[DT_STRLEN_BYTES]);
> -					 */
> +	call	BPF_FUNC_probe_read_str	/* rc = probe_read_str(dst, cnt, s1); */
>   	jsle	%r0, 0, .Lexit		/* if (rc s<= 0) goto .Lexit; */
>   	mov	%r7, %r0		/* len = rc */
> -	jeq	%r7, %r6, .Lset_len	/* if (len == cnt) goto .Lset_len; */
> +	jeq	%r7, %r6, .Lexit	/* if (len == cnt) goto .Lexit; */
>   	sub	%r7, 1			/* len-- */
>   
>   	mov	%r1, %r9
> -	add	%r1, DT_STRLEN_BYTES
>   	add	%r1, %r7
>   	mov	%r2, %r6
>   	sub	%r2, %r7
>   	mov	%r3, %r8
> -	add	%r3, DT_STRLEN_BYTES
>   	call	BPF_FUNC_probe_read_str	/*
> -					 * rc = probe_read_str(
> -					 *	  &dst[DT_STRLEN_BYTES + len],
> -					 *	  cnt - len,
> -					 *	  &s2[DT_STRLEN_BYTES]);
> +					 * rc = probe_read_str(dst, cnt - len,
> +					 *		       s2);


I don't think that comment is quite right.  You're copying the second 
string to dst+len, not to dst.

>   					 */
> -	jsle	%r0, 0, .Lexit		/* if (rc s<= 0) goto .Lset_len; */
> +	jsle	%r0, 0, .Lexit		/* if (rc s<= 0) goto .Lexit */
>   	add	%r7, %r0		/* len += rc */
>   
> -.Lset_len:
> -	sub	%r7, 1			/* len-- */
> -	mov	%r1, %r7
> -	mov	%r2, %r9
> -	call	dt_strlen_store		/* dt_strlen_store(len - 1, dst) */
> -
>   .Lexit:
>   	exit
>   	.size	dt_strjoin, .-dt_strjoin
> diff --git a/bpf/strrchr.S b/bpf/strrchr.S
> @@ -71,10 +65,9 @@ dt_strrchr :
>   .Lloop:
>   	sub	%r8, 1			/* r8-- */
>   	mov	%r4, %r8
> -	add	%r4, DT_STRLEN_BYTES
>   	ldxdw	%r3, [%fp+-8]
>   	add	%r3, %r4
> -	ldxb	%r3, [%r3+0]		/* r3 = src[DT_STRLEN_BYTES+ r8] */
> +	ldxb	%r3, [%r3+0]		/* r3 = src[r8] */
>   	lsh	%r3, 56			/* r3 <<= 56 */
>   	jeq	%r3, %r5, .Lfound	/* if (r3 == c) goto Lfound */
>   	jgt	%r8, 0, .Lloop		/* if (r8 > 0) goto Lloop */

Perhaps DT_STRLEN_BYTES should have been handled differently originally, 
but now it is definitely out of the picture.  So, with the 
simplification, %r4 is not needed.  No need to "mov %r4 %r8". Just "add 
%r3 %r8" directly.

> diff --git a/bpf/strtok.S b/bpf/strtok.S
> @@ -291,8 +289,6 @@ dt_strtok_flip:
>    * void dt_strtok(char *dst, char *str, const char *del, char *tmp)
>    * {
>    *     // discard delimiter length prefix; we look for the NULL terminator anyhow
> - *     del += DT_STRLEN_BYTES
> - *
>    *     // len = roundup(STRSZ + 1, 8)
>    *     len = ((STRSZ + 1) + 7) & -8
>    *

Since the "del+=DT_STRLEN_BYTES" is removed, its comment should also be 
removed.

> @@ -459,23 +451,18 @@ dt_strtok:
>   	mov	BGN, 0
>   
>   	/* copy str + bgn to destination */
> -	mov	%r1, LEN
> -	ldxdw	%r2, DST_X
> -	call	dt_strlen_store		/* dt_strlen_store(len, dst) */
> -
>   	ldxdw	%r1, DST_X
> -	add	%r1, DT_STRLEN_BYTES
>   	mov	%r2, LEN
>   	mov	%r3, STR
>   	add	%r3, 8
>   	add	%r3, BGN
> -	call	BPF_FUNC_probe_read	/* bpf_probe_read(dst + DT_STRLEN_BYTES, len, str + 8 + bgn) */
> +	call	BPF_FUNC_probe_read	/* bpf_probe_read(dst, len, str + 8 + bgn) */
>   
>   	mov	%r1, 0
>   	ldxdw	%r2, DST_X
>   	add	%r2, LEN
> -	stxb	[%r2+DT_STRLEN_BYTES], %r1
> -					/* dst[DT_STRLEN_BYTES + len] = '\0' */
> +	stxb	[%r2+0], %r1
> +					/* dst[len] = '\0' */
>   
>   	/* update the 8-byte prefix (strtok offset) */
>   	add	BGN, LEN		/* bgn += len */

The stxb instruction is now short enough that its comment can be on the 
same line as it is.