[DTrace-devel] [PATCH] Minor edits

[Kris Van Hees]

On Mon, Feb 28, 2022 at 03:21:48PM -0500, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> Leftover review comments from
> e04248378e3a Implement strlen() based on bpf_probe_read_str()

Well, yes and no :)  (See below...)  I'd suggest instead having the commit msg
reflect that this adding optimization for zero length source strings.  The
addition of typoe fixes does not need mention in the commit msg.  You could add
a mention of the harmless dn_reg copy being removed.

> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  bpf/substr.S      | 8 ++++----
>  libdtrace/dt_cg.c | 1 -
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/bpf/substr.S b/bpf/substr.S
> index 8d501faa..886a7200 100644
> --- a/bpf/substr.S
> +++ b/bpf/substr.S
> @@ -41,9 +41,9 @@ dt_substr :
>  
>  	/*
>  	 * Get the source string length and validate it.
> -	 * If the return value of probe_read_str) is less than 0, an error
> -	 * occured, and the result will be the empty string.
> -	 * If the length is 0, the result is the empty string.
> +	 * If the return value of probe_read_str() is less than 0, an error
> +	 * occurred, and the result will be the empty string.
> +	 * If the length is 1, the result is the empty string.

I do not see any previous comment about these changes.  Sure, there are typos
there but we generally do not submit patches to correct things like that with
specific patches.

>  	 * If the length is less than the maximum string length (STRSZ), use
>  	 * the length we got.  (The length is initialized in %r8 as the default
>  	 * string length.)
> @@ -55,7 +55,7 @@ dt_substr :
>  					 * len = probe_read_str(dst, STRSZ + 1,
>  					 *			src);
>  					 */
> -	jslt	%r0, 1, .Lempty		/* if (len < 1) goto Lempty; */
> +	jsle	%r0, 1, .Lempty		/* if (len <= 1) goto Lempty; */

I did not take the suggestion because I felt the change caused more confusion,
but with the suggested comment change to the comment block I do agree it is OK
now.

>  	sub	%r0, 1			/* len--; */
>  	mov	%r4, %r8		/* %r4 = STRSZ (previously in %r8) */
>  	mov	%r8, %r0		/* %r8 = len */
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 59c4fdc5..d4a58667 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3989,7 +3989,6 @@ dt_cg_subr_strlen(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  
>  	dt_cg_node(str, dlp, drp);
>  	dt_cg_check_notnull(dlp, drp, str->dn_reg);
> -	dnp->dn_reg = str->dn_reg;		/* re-use register */

Yes, I missed that one (though it is harmless).

>  	if (dt_regset_xalloc_args(drp) == -1)
>  		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel