[DTrace-devel] [PATCH 1/2] Add support for strjoin() subroutine
Eugene Loh
eugene.loh at oracle.com
Thu Sep 2 13:25:02 PDT 2021
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
I'm ignoring the similarly named "3/3" patch. Anyhow, some extra
comments as usual...
On 9/2/21 11:33 AM, Kris Van Hees wrote:
> diff --git a/bpf/strjoin.S b/bpf/strjoin.S
> new file mode 100644
> index 00000000..2a19f2eb
> --- /dev/null
> +++ b/bpf/strjoin.S
> @@ -0,0 +1,55 @@
> +// 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_str 45
> +
> +/*
> + * void dt_strjoin(char *dst, const char *s1, const char *s2)
> + */
> + .text
> + .align 4
> + .global dt_strjoin
> + .type dt_strjoin, @function
> +dt_strjoin:
> + mov %r9, %r1
> + mov %r7, %r2
> + mov %r8, %r3
> +
> + mov %r1, %r7
> + call dt_strlen
> + mov %r6, %r0
> + add %r7, DT_STRLEN_BYTES
> +
> + mov %r1, %r8
> + call dt_strlen
> + add %r6, %r0
> + add %r8, DT_STRLEN_BYTES
> +
> + mov %r1, %r6
> + mov %r2, %r9
> + call dt_strlen_store
> + add %r9, DT_STRLEN_BYTES
> +
> + lddw %r6, STRSZ
> + and %r6, 0xffffffff
Okay, but why the &=-1? We're doing a dw ld of some "small" value
(e.g., 256). So the &=-1 is a nop... doesn't even help the BPF verifier.
> +
> + mov %r1, %r9
> + mov %r2, %r6
> + mov %r3, %r7
> + call BPF_FUNC_probe_read_str
> + jslt %r0, 0, .L1
> + jslt %r0, 1, .L2
> + add %r9, %r0
> + sub %r9, 1
> +.L2:
> + mov %r1, %r9
> + mov %r2, %r6
> + mov %r3, %r8
> + call BPF_FUNC_probe_read_str
> +.L1:
> + exit
> + .size dt_strjoin, .-dt_strjoin
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index f9710a5e..b48b9f7b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -753,20 +753,31 @@ dt_cg_strlen(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src)
> size_t size = yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE];
> uint_t lbl_ok = dt_irlist_label(dlp);
>
> + TRACE_REGSET(" strlen:Begin");
> +
> assert(idp != NULL);
> if (dt_regset_xalloc_args(drp) == -1)
> longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> - emit(dlp, BPF_MOV_REG(BPF_REG_1, src));
> - dt_regset_xalloc(drp, BPF_REG_0);
> + if (src != BPF_REG_1)
> + emit(dlp, BPF_MOV_REG(BPF_REG_1, src));
> + if (dst != BPF_REG_0)
> + dt_regset_xalloc(drp, BPF_REG_0);
The src!=%r1 is okay, I guess, but doesn't it make more sense to deal
with this sort of thing in some later optimization phase?
The dst!=%r0 is more important, but shouldn't we be worrying about this
sort of thing more broadly and not just right here? Come to think of
it, why has dst already been allocated? It's exerting unnecessary
register pressure to allocate a register before a function call when the
register is not needed until afterwards.
> emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> dt_regset_free_args(drp);
> emit(dlp, BPF_BRANCH_IMM(BPF_JLE, BPF_REG_0, size, lbl_ok));
> emit(dlp, BPF_MOV_IMM(BPF_REG_0, size));
> - emitl(dlp, lbl_ok,
> - BPF_MOV_REG(dst, BPF_REG_0));
> - dt_regset_free(drp, BPF_REG_0);
> +
> + if (dst != BPF_REG_0) {
> + emitl(dlp, lbl_ok,
> + BPF_MOV_REG(dst, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
> + } else
> + emitl(dlp, lbl_ok,
> + BPF_NOP());
To me, it'd be cleaner to write
emit(dlp, BPF_BRANCH_IMM(BPF_JLE, BPF_REG_0, size, lbl_ok));
emit(dlp, BPF_MOV_IMM(BPF_REG_0, size));
emitl(dlp, lbl_ok,
BPF_NOP());
if (dst != BPF_REG_0) {
emit(dlp, BPF_MOV_REG(dst, BPF_REG_0));
dt_regset_free(drp, BPF_REG_0);
}
> +
> + TRACE_REGSET(" strlen:End ");
> }
>
> static void
> @@ -3089,6 +3100,55 @@ dt_cg_subr_strlen(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> TRACE_REGSET(" subr-strlen:End ");
> }
>
> +static void
> +dt_cg_subr_strjoin(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> + dt_node_t *s1 = dnp->dn_args;
> + dt_node_t *s2 = s1->dn_list;
> + dt_ident_t *idp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_strjoin");
> +
> + assert(idp != NULL);
> +
> + TRACE_REGSET(" subr-strjoin:Begin");
> +
> + dt_cg_node(s1, dlp, drp);
> + dt_cg_check_notnull(dlp, drp, s1->dn_reg);
> + dt_cg_node(s2, dlp, drp);
> + dt_cg_check_notnull(dlp, drp, s2->dn_reg);
> +
> + /*
> + * We can re-use the register from s1 for our result. The result needs
> + * be be a temporary string, so we need to allocate one.
> + */
I'm confused by the "reuse s1 reg" comment. It looks to me like we do not.
> + 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));
> +
> + 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, s1->dn_reg));
> + dt_regset_free(drp, s1->dn_reg);
> + if (s1->dn_tstring)
> + dt_cg_tstring_free(s1->dn_tstring->dn_value);
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, s2->dn_reg));
> + dt_regset_free(drp, s2->dn_reg);
> + if (s2->dn_tstring)
> + dt_cg_tstring_free(s2->dn_tstring->dn_value);
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
> + dt_regset_free_args(drp);
> + dt_regset_free(drp, BPF_REG_0);
> +
> + TRACE_REGSET(" subr-strjoin:End ");
> +}
> +
> typedef void dt_cg_subr_f(dt_node_t *, dt_irlist_t *, dt_regset_t *);
>
> static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
> @@ -3115,7 +3175,7 @@ static dt_cg_subr_f *_dt_cg_subr[DIF_SUBR_MAX + 1] = {
> [DIF_SUBR_GETMAJOR] = NULL,
> [DIF_SUBR_GETMINOR] = NULL,
> [DIF_SUBR_DDI_PATHNAME] = NULL,
> - [DIF_SUBR_STRJOIN] = NULL,
> + [DIF_SUBR_STRJOIN] = dt_cg_subr_strjoin,
> [DIF_SUBR_LLTOSTR] = NULL,
> [DIF_SUBR_BASENAME] = NULL,
> [DIF_SUBR_DIRNAME] = NULL,
> diff --git a/libdtrace/dt_dlibs.c b/libdtrace/dt_dlibs.c
> index 04becfa6..771b7fe0 100644
> --- a/libdtrace/dt_dlibs.c
> +++ b/libdtrace/dt_dlibs.c
> @@ -59,6 +59,7 @@ static const dt_ident_t dt_bpf_symbols[] = {
> DT_BPF_SYMBOL(dt_get_string, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_get_tvar, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_set_tvar, DT_IDENT_SYMBOL),
> + DT_BPF_SYMBOL(dt_strjoin, DT_IDENT_SYMBOL),
> DT_BPF_SYMBOL(dt_strnlen, DT_IDENT_SYMBOL),
> /* BPF maps */
> DT_BPF_SYMBOL(aggs, DT_IDENT_PTR),
> diff --git a/test/unittest/dif/strjoin.d b/test/unittest/dif/strjoin.d
> index e7834eb9..ee70dec8 100644
> --- a/test/unittest/dif/strjoin.d
> +++ b/test/unittest/dif/strjoin.d
> @@ -1,6 +1,10 @@
> -/* @@xfail: dtv2 */
> BEGIN
> {
> trace(strjoin(probeprov, probename));
> exit(0);
> }
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/strjoin/tst.strjoin-nested.d b/test/unittest/funcs/strjoin/tst.strjoin-nested.d
> new file mode 100644
> index 00000000..84ba459c
> --- /dev/null
> +++ b/test/unittest/funcs/strjoin/tst.strjoin-nested.d
> @@ -0,0 +1,34 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Nested strjoin() works
> + *
> + * SECTION: Actions and Subroutines/strjoin()
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + trace(strjoin(
> + strjoin(
> + strjoin(probeprov, ":"),
> + strjoin(probemod, ":")
> + ),
> + strjoin(
> + strjoin(probefunc, ":"),
> + probename
> + )
> + ));
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/strjoin/tst.strjoin-nested.r b/test/unittest/funcs/strjoin/tst.strjoin-nested.r
> new file mode 100644
> index 00000000..748852fb
> --- /dev/null
> +++ b/test/unittest/funcs/strjoin/tst.strjoin-nested.r
> @@ -0,0 +1 @@
> +dtrace:::BEGIN
> diff --git a/test/unittest/funcs/tst.strjoin.d b/test/unittest/funcs/strjoin/tst.strjoin.d
> similarity index 80%
> rename from test/unittest/funcs/tst.strjoin.d
> rename to test/unittest/funcs/strjoin/tst.strjoin.d
> index beec500c..f52443bd 100644
> --- a/test/unittest/funcs/tst.strjoin.d
> +++ b/test/unittest/funcs/strjoin/tst.strjoin.d
> @@ -1,10 +1,9 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2021, Oracle and/or its affiliates. All rights reserved.
> * Licensed under the Universal Permissive License v 1.0 as shown at
> * http://oss.oracle.com/licenses/upl.
> */
> -/* @@xfail: dtv2 */
>
> #pragma D option quiet
>
> diff --git a/test/unittest/funcs/tst.strjoin.r b/test/unittest/funcs/strjoin/tst.strjoin.r
> similarity index 100%
> rename from test/unittest/funcs/tst.strjoin.r
> rename to test/unittest/funcs/strjoin/tst.strjoin.r
More information about the DTrace-devel
mailing list