[DTrace-devel] [PATCH 4/4] Remove the string length prefix
Kris Van Hees
kris.van.hees at oracle.com
Fri Jan 28 23:58:56 UTC 2022
On Fri, Jan 28, 2022 at 04:22:18PM -0500, Eugene Loh via DTrace-devel wrote:
> 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?
Yeah, should remove it. I'll add that to this patch.
> 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
Thanks.
> > 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.
Oops, yes I deleted a bit too much there. Thanks.
> > */
> > - 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.
Hm, I thought I had done that. May have done it, then needed to rework some
stuff and accidentally when I reset, lost it. Thanks for catching these
things.
>
> > 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.
Thanks.
> > @@ -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.
Good point.
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list