[DTrace-devel] [PATCH] Allow dt_cg_arglist() to handle the NULL case
Eugene Loh
eugene.loh at oracle.com
Tue Aug 16 02:35:37 UTC 2022
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
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.
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.
> 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
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.
> 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.
> @@ -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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://oss.oracle.com/pipermail/dtrace-devel/attachments/20220815/2a99cf2b/attachment-0001.html>
More information about the DTrace-devel
mailing list