[DTrace-devel] [PATCH 5/7] Add support for built-in variable args
Eugene Loh
eugene.loh at oracle.com
Fri May 27 16:29:41 UTC 2022
Another one where I do not feel I understand the code that well, but:
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
A few things:
*) There is a typo in dt_dis.c:
"We know that the previous two instruction exists and move" ->
"We know that the previous two instructions exist and move"
*) test/unittest/disasm/tst.ann-bvar.sh is missing uregs.
*) Is it new behavior that syscall probes have typed args?
Is that advertised?
*) Why are there no tests (other than disasm and err)? One argument
could be
because SDT args are not yet populated... patch 6. But syscall
apparently
has typed args. After all,
test/unittest/variables/bvar/err.D_ARGS_IDX.args-too-many.d
requires it.
*) One can slightly simplify things by omitting the unnecessary "quiet"
option
from err.* tests.
*) But I would be in favor of adding
/* @@runtest-opts: " " -e */
to those err tests. That just saves a little bit of time (waiting
for the test
to time out) in the unanticipated case of the script actually running.
(The weird " " is to deal with runtest.sh trying to get "-e" by
running "echo -e".
I can submit a patch to fix runtest.sh if we care.)
*) What is this comment about? I.e., are there other anticipated uses?
+/*
+ * Currently, this code is only used for the args[] and uregs[]
arrays.
+ */
static void
dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
On 5/26/22 11:24, Kris Van Hees via DTrace-devel wrote:
> Signed-off-by: Kris Van Hees<kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 57 +++++++++++++------
> libdtrace/dt_dis.c | 9 +--
> libdtrace/dt_ident.c | 5 ++
> test/unittest/disasm/tst.ann-bvar.r | 34 +++++++++++
> test/unittest/disasm/tst.ann-bvar.sh | 52 +++++++++++++++++
> .../bvar/err.D_ARGS_IDX.args-neg-idx.d | 23 ++++++++
> .../bvar/err.D_ARGS_IDX.args-no-args.d | 23 ++++++++
> .../bvar/err.D_ARGS_IDX.args-too-many.d | 26 +++++++++
> ...rgs.d => err.D_IDENT_BADREF.args-no-idx.d} | 7 +--
> .../err.D_PROTO_ARG.args-non-scalar-idx.d | 23 ++++++++
> 10 files changed, 235 insertions(+), 24 deletions(-)
> create mode 100644 test/unittest/disasm/tst.ann-bvar.r
> create mode 100755 test/unittest/disasm/tst.ann-bvar.sh
> create mode 100644 test/unittest/variables/bvar/err.D_ARGS_IDX.args-neg-idx.d
> create mode 100644 test/unittest/variables/bvar/err.D_ARGS_IDX.args-no-args.d
> create mode 100644 test/unittest/variables/bvar/err.D_ARGS_IDX.args-too-many.d
> rename test/unittest/variables/bvar/{tst.args.d => err.D_IDENT_BADREF.args-no-idx.d} (58%)
> create mode 100644 test/unittest/variables/bvar/err.D_PROTO_ARG.args-non-scalar-idx.d
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index bbddc986..c6e1013b 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3623,23 +3623,27 @@ dt_cg_assoc_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> TRACE_REGSET(" assoc_op: End ");
> }
>
> +/*
> + * Currently, this code is only used for the args[] and uregs[] arrays.
> + */
> static void
> dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> {
> - dt_probe_t *prp = yypcb->pcb_probe;
> - uintmax_t saved = dnp->dn_args->dn_value;
> - dt_ident_t *idp = dnp->dn_ident;
> -
> - ssize_t base;
> - size_t size;
> - int n;
> + dt_probe_t *prp = yypcb->pcb_probe;
> + uintmax_t saved = dnp->dn_args->dn_value;
> + dt_ident_t *idp = dnp->dn_ident;
> + dt_ident_t *fidp = dt_dlib_get_func(yypcb->pcb_hdl, "dt_get_bvar");
> + size_t size;
> + int n;
>
> assert(dnp->dn_kind == DT_NODE_VAR);
> - assert(!(idp->di_flags & DT_IDFLG_LOCAL));
> + assert(!(idp->di_flags & (DT_IDFLG_TLS | DT_IDFLG_LOCAL)));
>
> assert(dnp->dn_args->dn_kind == DT_NODE_INT);
> assert(dnp->dn_args->dn_list == NULL);
>
> + assert(fidp != NULL);
> +
> /*
> * If this is a reference in the args[] array, temporarily modify the
> * array index according to the static argument mapping (if any),
> @@ -3657,18 +3661,39 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> dnp->dn_args->dn_value = prp->mapping[saved];
> }
>
> + /*
> + * Mark the identifier as referenced by the current DIFO. If it is an
> + * orphaned identifier, we also need to mark the primary identifier
> + * (global identifier with the same name).
> + *
> + * We can safely replace idp with the primary identifier at this point
> + * because they have the same id.
> + */
> + idp->di_flags |= DT_IDFLG_DIFR;
> + if (idp->di_flags & DT_IDFLG_ORPHAN) {
> + idp = dt_idhash_lookup(yypcb->pcb_hdl->dt_globals, "args");
> + assert(idp != NULL);
> + idp->di_flags |= DT_IDFLG_DIFR;
> + }
> +
> dt_cg_node(dnp->dn_args, dlp, drp);
> dnp->dn_args->dn_value = saved;
> -
> dnp->dn_reg = dnp->dn_args->dn_reg;
>
> - if (idp->di_flags & DT_IDFLG_TLS)
> - base = 0x2000;
> - else
> - base = 0x3000;
> + if (dt_regset_xalloc_args(drp) == -1)
> + longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>
> - idp->di_flags |= DT_IDFLG_DIFR;
> - emit(dlp, BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, base + dnp->dn_ident->di_id));
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_FP, DT_STK_DCTX));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_2, idp->di_id));
> + emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> + dt_regset_xalloc(drp, BPF_REG_0);
> + emite(dlp, BPF_CALL_FUNC(fidp->di_id), fidp);
> + dt_regset_free_args(drp);
> +
> + dt_cg_check_fault(yypcb);
> +
> + emit(dlp, BPF_MOV_REG(dnp->dn_reg, BPF_REG_0));
> + dt_regset_free(drp, BPF_REG_0);
>
> /*
> * If this is a reference to the args[] array, we need to take the
> @@ -3691,7 +3716,7 @@ dt_cg_array_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> n = sizeof(uint64_t) * NBBY - size * NBBY;
>
> emit(dlp, BPF_ALU64_IMM(BPF_LSH, dnp->dn_reg, n));
> - emit(dlp, BPF_ALU64_REG((dnp->dn_flags & DT_NF_SIGNED) ? BPF_ARSH : BPF_RSH, dnp->dn_reg, n));
> + emit(dlp, BPF_ALU64_IMM((dnp->dn_flags & DT_NF_SIGNED) ? BPF_ARSH : BPF_RSH, dnp->dn_reg, n));
> }
>
> /*
> diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> index 9a365a6b..e2f31172 100644
> --- a/libdtrace/dt_dis.c
> +++ b/libdtrace/dt_dis.c
> @@ -346,11 +346,12 @@ dt_dis_bpf_args(const dtrace_difo_t *dp, const char *fn,
> {
> if (strcmp(fn, "dt_get_bvar") == 0) {
> /*
> - * We know that the previous instruction exists and moves
> - * the variable id to a register (because we wrote the code
> - * generator to emit the instructions in this exact order.)
> + * We know that the previous two instruction exists and move
> + * the variable id to a register in the first instruction of
> + * that sequence (because we wrote the code generator to emit
> + * the instructions in this exact order.)
> */
> - in--;
> + in -= 2;
> snprintf(buf, len, "%s",
> dt_dis_varname_id(dp, in->imm, DIFV_SCOPE_GLOBAL, addr));
> return buf;
> diff --git a/libdtrace/dt_ident.c b/libdtrace/dt_ident.c
> index e34f8e8b..a81863bb 100644
> --- a/libdtrace/dt_ident.c
> +++ b/libdtrace/dt_ident.c
> @@ -366,6 +366,11 @@ dt_idcook_args(dt_node_t *dnp, dt_ident_t *idp, int argc, dt_node_t *ap)
> xyerror(D_ARGS_TYPE, "failed to resolve native type for "
> "%s[%lld]\n", idp->di_name, (longlong_t)ap->dn_value);
>
> + if (idp->di_type == CTF_ERR) {
> + idp->di_ctfp = DT_DYN_CTFP(dtp);
> + idp->di_type = DT_DYN_TYPE(dtp);
> + }
> +
> if (dtp->dt_xlatemode == DT_XL_STATIC && (
> nnp == xnp || dt_node_is_argcompat(nnp, xnp))) {
> dnp->dn_ident = dt_ident_create(idp->di_name, idp->di_kind,
> diff --git a/test/unittest/disasm/tst.ann-bvar.r b/test/unittest/disasm/tst.ann-bvar.r
> new file mode 100644
> index 00000000..b514dd99
> --- /dev/null
> +++ b/test/unittest/disasm/tst.ann-bvar.r
> @@ -0,0 +1,34 @@
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg0
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg1
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg2
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg3
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg4
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg5
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg6
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg7
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg8
> +85 0 1 0000 ffffffff call dt_get_bvar ! arg9
> +85 0 1 0000 ffffffff call dt_get_bvar ! args
> +85 0 1 0000 ffffffff call dt_get_bvar ! caller
> +85 0 1 0000 ffffffff call dt_get_bvar ! curcpu
> +85 0 1 0000 ffffffff call dt_get_bvar ! curthread
> +85 0 1 0000 ffffffff call dt_get_bvar ! epid
> +85 0 1 0000 ffffffff call dt_get_bvar ! errno
> +85 0 1 0000 ffffffff call dt_get_bvar ! execname
> +85 0 1 0000 ffffffff call dt_get_bvar ! gid
> +85 0 1 0000 ffffffff call dt_get_bvar ! id
> +85 0 1 0000 ffffffff call dt_get_bvar ! ipl
> +85 0 1 0000 ffffffff call dt_get_bvar ! pid
> +85 0 1 0000 ffffffff call dt_get_bvar ! ppid
> +85 0 1 0000 ffffffff call dt_get_bvar ! probefunc
> +85 0 1 0000 ffffffff call dt_get_bvar ! probemod
> +85 0 1 0000 ffffffff call dt_get_bvar ! probename
> +85 0 1 0000 ffffffff call dt_get_bvar ! probeprov
> +85 0 1 0000 ffffffff call dt_get_bvar ! stackdepth
> +85 0 1 0000 ffffffff call dt_get_bvar ! tid
> +85 0 1 0000 ffffffff call dt_get_bvar ! timestamp
> +85 0 1 0000 ffffffff call dt_get_bvar ! ucaller
> +85 0 1 0000 ffffffff call dt_get_bvar ! uid
> +85 0 1 0000 ffffffff call dt_get_bvar ! ustackdepth
> +85 0 1 0000 ffffffff call dt_get_bvar ! vtimestamp
> +85 0 1 0000 ffffffff call dt_get_bvar ! walltimestamp
> diff --git a/test/unittest/disasm/tst.ann-bvar.sh b/test/unittest/disasm/tst.ann-bvar.sh
> new file mode 100755
> index 00000000..1986de66
> --- /dev/null
> +++ b/test/unittest/disasm/tst.ann-bvar.sh
> @@ -0,0 +1,52 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2022, 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.
> +#
> +
> +dtrace=$1
> +
> +$dtrace $dt_flags -Sen '
> +sdt:task::task_rename
> +{
> + trace(arg0);
> + trace(arg1);
> + trace(arg2);
> + trace(arg3);
> + trace(arg4);
> + trace(arg5);
> + trace(arg6);
> + trace(arg7);
> + trace(arg8);
> + trace(arg9);
> + trace(args[0]);
> + trace(caller);
> + trace(curcpu);
> + trace(curthread);
> + trace(epid);
> + trace(errno);
> + trace(execname);
> + trace(gid);
> + trace(id);
> + trace(ipl);
> + trace(pid);
> + trace(ppid);
> + trace(probefunc);
> + trace(probemod);
> + trace(probename);
> + trace(probeprov);
> + trace(stackdepth);
> + trace(tid);
> + trace(timestamp);
> + trace(ucaller);
> + trace(uid);
> + trace(ustackdepth);
> + trace(vtimestamp);
> + trace(walltimestamp);
> + exit(0);
> +}
> +' 2>&1 | awk '/ call dt_get_bvar/ { sub(/^[^:]+: /, ""); print; }'
> +
> +exit $?
> diff --git a/test/unittest/variables/bvar/err.D_ARGS_IDX.args-neg-idx.d b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-neg-idx.d
> new file mode 100644
> index 00000000..cde98985
> --- /dev/null
> +++ b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-neg-idx.d
> @@ -0,0 +1,23 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: Accessing args[-1] for probes with arguments triggers a fault.
> + *
> + * SECTION: Variables/Built-in Variables/args
> + */
> +
> +#pragma D option quiet
> +
> +write:entry {
> + trace(args[-1]);
> + exit(0);
> +}
> +
> +ERROR {
> + exit(1);
> +}
> diff --git a/test/unittest/variables/bvar/err.D_ARGS_IDX.args-no-args.d b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-no-args.d
> new file mode 100644
> index 00000000..2c8f5041
> --- /dev/null
> +++ b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-no-args.d
> @@ -0,0 +1,23 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: Accessing args[0] for probes without arguments triggers a fault.
> + *
> + * SECTION: Variables/Built-in Variables/args
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN {
> + trace(args[0]);
> + exit(0);
> +}
> +
> +ERROR {
> + exit(1);
> +}
> diff --git a/test/unittest/variables/bvar/err.D_ARGS_IDX.args-too-many.d b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-too-many.d
> new file mode 100644
> index 00000000..35ca3338
> --- /dev/null
> +++ b/test/unittest/variables/bvar/err.D_ARGS_IDX.args-too-many.d
> @@ -0,0 +1,26 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: Accessing args[i] for probes with n arguments fails when i >= n.
> + *
> + * SECTION: Variables/Built-in Variables/args
> + */
> +
> +#pragma D option quiet
> +
> +write:entry {
> + trace(args[0]);
> + trace(args[1]);
> + trace(args[2]);
> + trace(args[3]);
> + exit(0);
> +}
> +
> +ERROR {
> + exit(1);
> +}
> diff --git a/test/unittest/variables/bvar/tst.args.d b/test/unittest/variables/bvar/err.D_IDENT_BADREF.args-no-idx.d
> similarity index 58%
> rename from test/unittest/variables/bvar/tst.args.d
> rename to test/unittest/variables/bvar/err.D_IDENT_BADREF.args-no-idx.d
> index 09536766..9f0bb67c 100644
> --- a/test/unittest/variables/bvar/tst.args.d
> +++ b/test/unittest/variables/bvar/err.D_IDENT_BADREF.args-no-idx.d
> @@ -1,13 +1,12 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2022, 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 */
>
> /*
> - * ASSERTION: The 'args' variable can be accessed and is not -1.
> + * ASSERTION: The 'args' variable must be indexed.
> *
> * SECTION: Variables/Built-in Variables/args
> */
> @@ -16,7 +15,7 @@
>
> BEGIN {
> trace(args);
> - exit(args != -1 ? 0 : 1);
> + exit(0);
> }
>
> ERROR {
> diff --git a/test/unittest/variables/bvar/err.D_PROTO_ARG.args-non-scalar-idx.d b/test/unittest/variables/bvar/err.D_PROTO_ARG.args-non-scalar-idx.d
> new file mode 100644
> index 00000000..3b3eed0e
> --- /dev/null
> +++ b/test/unittest/variables/bvar/err.D_PROTO_ARG.args-non-scalar-idx.d
> @@ -0,0 +1,23 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, 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: The index of args[] must be a scalar.
> + *
> + * SECTION: Variables/Built-in Variables/args
> + */
> +
> +BEGIN
> +{
> + trace(args["a"]);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
More information about the DTrace-devel
mailing list