[DTrace-devel] [PATCH] Allow dt_cg_arglist() to handle the NULL case
Kris Van Hees
kris.van.hees at oracle.com
Tue Aug 16 05:05:19 UTC 2022
On Mon, Aug 15, 2022 at 07:35:37PM -0700, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
Thanks.
> That said, this patch bewilders me with its complexity. The previous patch
> (return register) seemed quite a bit simpler. I do not understand the
> rationale for the increased complexity.
Yes, but the previous patch was a bit of a hack I think. One option could
have been to rewrite dt_cg_arglist() to always return the register holding
the tuple pointer and not store it in args->dn_reg at all. That would
require the callers to be modified but that was certainly an option. But
it would make dt_cg_arglist() quite different from most other codegen funcs
that generate code for the node tree they are given, decorating the given
node with the register that holds the resulting value.
I opted to stick with the typical way codegen funcs are implemented.
> A few more comments embedded...
>
>
> On 8/15/22 11:45, Kris Van Hees via DTrace-devel wrote:
>
> Even if an argument is empty, we might want to call dt_cg_arglist() to
> construct a pointer to a tuple.
>
> Callers can use the following pseudo-code to accomplish this:
>
>
> Given the trickiness, it's good to see an explanation of how to use arglist()
> in the NULL case. I would think, however, that the explanation should go in a
> comment block for dt_cg_arglist() rather than in the commit history. The
> former is a much easier place for users to find such info.
It is here in the commit message because adding it as a comment would be rather
overkill, especially once the next patch goes in which adds the code that makes
use of this functionality. So there will be a sample of how to use it right in
the code anyway.
>       dt_node_t   noargs = {
>                         dnp->dn_ctfp,
>                         dtp->dt_type_void,
>                   };
>
>       ...
>
>       if (dnp->dn_aggtup == NULL)
>             dnp->dn_aggtup = &noargs;
>
>
> Why is dnp->dn_aggtup being set to anything (and restored later)? It's not
> being used. Or, is the point that you did not mean to hardwire noargs into the
> arglist call? Maybe you meant something like
The sample I am using prefigures the following patch that modifies the way
aggregations are implemented. I didn't want to put too much extra code in,
so by using dnp->dn_aggtup I convey what node we are working with. It is
pseudo code after all.
> dt_node_t *args = dnp->dn_aggtup ? dnp->dn_aggtup : &noargs;
> dt_cg_arglist(aid, args, dlp, drp);
> // use args->dn_reg
>
>
>       dt_cg_arglist(aid, &noargs, dlp, drp);
>
>       ... <use dnp->dn_aggtup->dn_reg> ...
>
>       if (dnp->aggtup == &noargs)
>             dnp->aggtup = NULL;
>
>       ... <dnp->dn_aggtup->dn_reg is not used anympre> ...
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_bpf.c | 18 +++++++++++-------
> libdtrace/dt_cg.c | 10 +++++++---
> libdtrace/dt_impl.h | 1 +
> libdtrace/dt_open.c | 4 +++-
> 4 files changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> index 4cb0eec6..f3f8c79d 100644
> --- a/libdtrace/dt_bpf.c
> +++ b/libdtrace/dt_bpf.c
> @@ -510,15 +510,19 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
>            dvarc = dtp->dt_options[DTRACEOPT_DYNVARSIZE] /
>                  dtp->dt_maxdvarsize;
>
> -     if (dvarc > 0 &&
> -      create_gmap(dtp, "dvars", BPF_MAP_TYPE_HASH,
> -                 sizeof(uint64_t), dtp->dt_maxdvarsize, dvarc) == -1)
> +     if (dvarc > 0) {
> +           if (create_gmap(dtp, "dvars", BPF_MAP_TYPE_HASH,
> +                       sizeof(uint64_t), dtp->dt_maxdvarsize,
> +                       dvarc) == -1)
>                  return -1;  /* dt_errno is set for us */
>
> -     if (dtp->dt_maxtuplesize > 0 &&
> -      create_gmap(dtp, "tuples", BPF_MAP_TYPE_HASH,
> -                 dtp->dt_maxtuplesize, sizeof(uint64_t), dvarc) == -1)
> -           return -1;        /* dt_errno is set for us */
> +           assert(dtp->dt_maxtuplesize > 0);
> +
> +           if (create_gmap(dtp, "tuples", BPF_MAP_TYPE_HASH,
> +                  dtp->dt_maxtuplesize, sizeof(uint64_t),
> +                  dvarc) == -1)
> +                 return -1;  /* dt_errno is set for us */
> +     }
>
>      /* Populate the 'cpuinfo' map. */
>      dt_bpf_map_update(ci_mapfd, &key, dtp->dt_conf.cpus);
>
> Do the dt_bpf.c changes really belong in this patch? I'm fine with the
> changes, but surprised they are included here.
It could go in its own patch as well, but in a sense it is related to the
change here because it expands the notion of a tuple beyond dynamic variables,
which means that we may have a max tuple size without using dynamic variables.
That would result in a tuples map being created with a maximum number of
entries of 0 (dvarc = 0) and that triggers a map creation failure. Since that
is directly related to this patch, the fix for it is included in it.
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 175c4b95..0963f202 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2600,13 +2600,17 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args,
> dt_irlist_t *dlp,
>      const dt_idsig_t  *isp = idp->di_data;
>      dt_node_t         *dnp;
>      dt_ident_t        *maxtupsz = dt_dlib_get_var(dtp, "TUPSZ");
> -     int               i;
> +     int               i = 0;
>      int               treg, areg;
>      uint_t                  tuplesize;
>
>      TRACE_REGSET(" arglist: Begin");
>
> -     for (dnp = args, i = 0; dnp != NULL; dnp = dnp->dn_list, i++) {
> +     /* A 'void' args node indicates an empty argument list. */
> +     if (dt_node_is_void(args))
> +           goto empty_args;
> +
> +     for (dnp = args; dnp != NULL; dnp = dnp->dn_list, i++) {
>            /* Bail early if we run out of tuple slots. */
>            if (i > dtp->dt_conf.dtc_diftupregs)
>                  longjmp(yypcb->pcb_jmpbuf, EDT_NOTUPREG);
> @@ -2623,6 +2627,7 @@ dt_cg_arglist(dt_ident_t *idp, dt_node_t *args,
> dt_irlist_t *dlp,
>            dt_regset_free(drp, BPF_REG_0);
>      }
>
> +empty_args:
>      TRACE_REGSET(" arglist: Stack");
>
>      /*
>
>
> I have a question about the looping (which we do not see in the patch text).
> The first loop computes values. The second loop stores the values. In the
> "NULL" case, we have an "if" statement that jumps over the first loop. In
> contrast, the second loop gets executed. Its first iteration short circuits
> due to size==0. Then the second iteration exits. I *think* that's what's
> going on and should work, but would not mind a sanity check since it was not at
> first clear to me what was going on.
That is indeed what happens.
>
> @@ -6669,7 +6674,6 @@ dt_cg_agg(dt_pcb_t *pcb, dt_node_t *dnp, dt_irlist_t
> *dlp, dt_regset_t *drp)
>            dnerror(dnp->dn_aggtup, D_ARR_BADREF, "indexing is not "
>                  "supported yet: @%s\n", dnp->dn_ident->di_name);
>
> -
>      assert(fid->di_id >= DT_AGG_BASE && fid->di_id < DT_AGG_HIGHEST);
>
>      dt_cg_clsflags(pcb, DTRACEACT_AGGREGATION, dnp);
> diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h
> index c1ff6256..85a1e7c9 100644
> --- a/libdtrace/dt_impl.h
> +++ b/libdtrace/dt_impl.h
> @@ -329,6 +329,7 @@ struct dtrace_hdl {
>      ctf_id_t dt_type_stack; /* cached CTF identifier for stack type */
>      ctf_id_t dt_type_symaddr; /* cached CTF identifier for _symaddr type
> */
>      ctf_id_t dt_type_usymaddr; /* cached CTF ident. for _usymaddr type */
> +     ctf_id_t dt_type_void;  /* cached CTF identifier for void type */
>      dtrace_epid_t dt_nextepid; /* next enabled probe ID to assign */
>      size_t dt_maxprobe;     /* max enabled probe ID */
>      dtrace_datadesc_t **dt_ddesc; /* probe data descriptions */
> diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c
> index f5cbd499..581172e2 100644
> --- a/libdtrace/dt_open.c
> +++ b/libdtrace/dt_open.c
> @@ -1055,10 +1055,12 @@ dt_vopen(int version, int flags, int *errp,
>      dtp->dt_type_usymaddr = ctf_add_typedef(dmp->dm_ctfp, CTF_ADD_ROOT,
>       "_usymaddr", ctf_lookup_by_name(dmp->dm_ctfp, "void"));
>
> +     dtp->dt_type_void = ctf_lookup_by_name(dmp->dm_ctfp, "void");
> +
>      if (dtp->dt_type_func == CTF_ERR || dtp->dt_type_fptr == CTF_ERR ||
>       dtp->dt_type_str == CTF_ERR || dtp->dt_type_dyn == CTF_ERR ||
>       dtp->dt_type_stack == CTF_ERR || dtp->dt_type_symaddr == CTF_ERR
> ||
> -      dtp->dt_type_usymaddr == CTF_ERR) {
> +      dtp->dt_type_usymaddr == CTF_ERR || dtp->dt_type_void == CTF_ERR)
> {
>            dt_dprintf("failed to add intrinsic to D container: %s\n",
>             ctf_errmsg(ctf_errno(dmp->dm_ctfp)));
>            return set_open_errno(dtp, errp, EDT_CTF);
> --
> 2.34.1
>
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
>
> _______________________________________________
> 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