[DTrace-devel] [PATCH 2/2] Add support for substr() subroutine
Kris Van Hees
kris.van.hees at oracle.com
Fri Sep 3 23:10:08 PDT 2021
On Thu, Sep 02, 2021 at 06:34:55PM -0400, Eugene Loh wrote:
> 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.
I don't think that the commit message or the code is the best place to
document the subroutine. That ought to be done in, well, the documentation.
> 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.
Harmless but yes, unnecessary.
> > + 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).
Separation of code. The code in dt_cg_subr_substr() is to set up the
arguments to the dt_substr() call, while the logic implementing substr()
lives in dt_substr(). So, this is the proper place to determine whether
there is an explicitly spcified cnt or not.
> > + 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.
The first instruction in Ladjust_cnt is not absolutely necessary, but when
idx > len, we know the result is the empty string.
> > + 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
I am not trying to go for the most concise code - part of why I wrote it the
way I did was for clarity.
> > +
> > + 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?
Yes. Fixed.
> > 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 ");
> > +}
>
> _______________________________________________
> 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