[DTrace-devel] [PATCH 5/7] Add support for built-in variable args
Kris Van Hees
kris.van.hees at oracle.com
Fri May 27 18:32:47 UTC 2022
On Fri, May 27, 2022 at 09:29:41AM -0700, Eugene Loh via DTrace-devel wrote:
> 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"
Thanks.
> *) test/unittest/disasm/tst.ann-bvar.sh is missing uregs.
That is because uregs isn't supported yet. But since this test is never
actually executing the generated BPF program, I guess I could include it
anyhow.
> *) Is it new behavior that syscall probes have typed args?
> Is that advertised?
That actually has always been the case since I implemented syscall probes
in this new DTrace implementation. But it could not be accessed because we
did not have args[] support.
> *) 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.
Well, as you point out, there are tests. And there are no tests to actually
test the proper functioning in this patch because this patch is not actually
providing any implementation of a provider that makes use of args[].
Arguably, the variables/bvar/tst.args-match-argN.d test could be moved into
this patch, if you prefer that.
> *) One can slightly simplify things by omitting the unnecessary "quiet"
> option
> from err.* tests.
Sure, but it hardly matters. And it avoids getting output we do not care
about.
> *) 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.)
Nick has a patch for this already posted for review.
> *) 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)
That code seems to imply that it can support things other than args and uregs,
but there is nothing that calls it for any other arrays. Hence to comment
to explain that.
> 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);
> > +}
>
> _______________________________________________
> 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