<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<p>Reviewed-by: Eugene Loh <a class="moz-txt-link-rfc2396E" href="mailto:eugene.loh@oracle.com"><eugene.loh@oracle.com></a> <br>
<br>
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.<br>
<br>
A few more comments embedded... <br>
<br>
</p>
<div class="moz-cite-prefix">On 8/15/22 11:45, Kris Van Hees via
DTrace-devel wrote:<br>
</div>
<blockquote type="cite" cite="mid:SN6PR10MB2975EA5A5D9ADFE9B62C2081C2689@SN6PR10MB2975.namprd10.prod.outlook.com">
<style type="text/css" style="display:none;">P {margin-top:0;margin-bottom:0;}</style>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif;
font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<div>Even if an argument is empty, we might want to call
dt_cg_arglist() to<br>
</div>
<div>construct a pointer to a tuple.</div>
<div><br>
</div>
<div>Callers can use the following pseudo-code to accomplish
this:</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite" cite="mid:SN6PR10MB2975EA5A5D9ADFE9B62C2081C2689@SN6PR10MB2975.namprd10.prod.outlook.com">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif;
font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<div> dt_node_t noargs = {</div>
<div> dnp->dn_ctfp,</div>
<div> dtp->dt_type_void,</div>
<div> };</div>
<div><br>
</div>
<div> ...</div>
<div><br>
</div>
<div> if (dnp->dn_aggtup == NULL)</div>
<div> dnp->dn_aggtup = &noargs;</div>
</div>
</blockquote>
<br>
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<br>
<br>
dt_node_t *args = dnp->dn_aggtup ? dnp->dn_aggtup :
&noargs;<br>
dt_cg_arglist(aid, args, dlp, drp);<br>
// use args->dn_reg<br>
<br>
<blockquote type="cite" cite="mid:SN6PR10MB2975EA5A5D9ADFE9B62C2081C2689@SN6PR10MB2975.namprd10.prod.outlook.com">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif;
font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<div> dt_cg_arglist(aid, &noargs, dlp, drp);</div>
<div><br>
</div>
<div> ... <use dnp->dn_aggtup->dn_reg> ...</div>
<div><br>
</div>
<div> if (dnp->aggtup == &noargs)</div>
<div> dnp->aggtup = NULL;</div>
<div><br>
</div>
<div> ... <dnp->dn_aggtup->dn_reg is not used
anympre> ...</div>
<div><br>
</div>
<div>Signed-off-by: Kris Van Hees
<a class="moz-txt-link-rfc2396E" href="mailto:kris.van.hees@oracle.com"><kris.van.hees@oracle.com></a></div>
<div>---</div>
<div> libdtrace/dt_bpf.c | 18 +++++++++++-------</div>
<div> libdtrace/dt_cg.c | 10 +++++++---</div>
<div> libdtrace/dt_impl.h | 1 +</div>
<div> libdtrace/dt_open.c | 4 +++-</div>
<div> 4 files changed, 22 insertions(+), 11 deletions(-)</div>
<div><br>
</div>
<div>diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c</div>
<div>index 4cb0eec6..f3f8c79d 100644</div>
<div>--- a/libdtrace/dt_bpf.c</div>
<div>+++ b/libdtrace/dt_bpf.c</div>
<div>@@ -510,15 +510,19 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)</div>
<div> dvarc =
dtp->dt_options[DTRACEOPT_DYNVARSIZE] /</div>
<div> dtp->dt_maxdvarsize;</div>
<div> </div>
<div>- if (dvarc > 0 &&</div>
<div>- create_gmap(dtp, "dvars", BPF_MAP_TYPE_HASH,</div>
<div>- sizeof(uint64_t), dtp->dt_maxdvarsize,
dvarc) == -1)</div>
<div>+ if (dvarc > 0) {</div>
<div>+ if (create_gmap(dtp, "dvars",
BPF_MAP_TYPE_HASH,</div>
<div>+ sizeof(uint64_t),
dtp->dt_maxdvarsize,</div>
<div>+ dvarc) == -1)</div>
<div> return -1; /* dt_errno is set for us */</div>
<div> </div>
<div>- if (dtp->dt_maxtuplesize > 0 &&</div>
<div>- create_gmap(dtp, "tuples", BPF_MAP_TYPE_HASH,</div>
<div>- dtp->dt_maxtuplesize,
sizeof(uint64_t), dvarc) == -1)</div>
<div>- return -1; /* dt_errno is set for us */</div>
<div>+ assert(dtp->dt_maxtuplesize > 0);</div>
<div>+</div>
<div>+ if (create_gmap(dtp, "tuples",
BPF_MAP_TYPE_HASH,</div>
<div>+ dtp->dt_maxtuplesize,
sizeof(uint64_t),</div>
<div>+ dvarc) == -1)</div>
<div>+ return -1; /* dt_errno is set for us */</div>
<div>+ }</div>
<div> </div>
<div> /* Populate the 'cpuinfo' map. */</div>
<div> dt_bpf_map_update(ci_mapfd, &key,
dtp->dt_conf.cpus);</div>
</div>
</blockquote>
<br>
Do the dt_bpf.c changes really belong in this patch? I'm fine with
the changes, but surprised they are included here.<br>
<br>
<blockquote type="cite" cite="mid:SN6PR10MB2975EA5A5D9ADFE9B62C2081C2689@SN6PR10MB2975.namprd10.prod.outlook.com">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif;
font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<div>diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c</div>
<div>index 175c4b95..0963f202 100644</div>
<div>--- a/libdtrace/dt_cg.c</div>
<div>+++ b/libdtrace/dt_cg.c</div>
<div>@@ -2600,13 +2600,17 @@ dt_cg_arglist(dt_ident_t *idp,
dt_node_t *args, dt_irlist_t *dlp,</div>
<div> const dt_idsig_t *isp = idp->di_data;</div>
<div> dt_node_t *dnp;</div>
<div> dt_ident_t *maxtupsz = dt_dlib_get_var(dtp,
"TUPSZ");</div>
<div>- int i;</div>
<div>+ int i = 0;</div>
<div> int treg, areg;</div>
<div> uint_t tuplesize;</div>
<div> </div>
<div> TRACE_REGSET(" arglist: Begin");</div>
<div> </div>
<div>- for (dnp = args, i = 0; dnp != NULL; dnp =
dnp->dn_list, i++) {</div>
<div>+ /* A 'void' args node indicates an empty argument
list. */</div>
<div>+ if (dt_node_is_void(args))</div>
<div>+ goto empty_args;</div>
<div>+</div>
<div>+ for (dnp = args; dnp != NULL; dnp = dnp->dn_list,
i++) {</div>
<div> /* Bail early if we run out of tuple slots. */</div>
<div> if (i > dtp->dt_conf.dtc_diftupregs)</div>
<div> longjmp(yypcb->pcb_jmpbuf,
EDT_NOTUPREG);</div>
<div>@@ -2623,6 +2627,7 @@ dt_cg_arglist(dt_ident_t *idp,
dt_node_t *args, dt_irlist_t *dlp,</div>
<div> dt_regset_free(drp, BPF_REG_0);</div>
<div> }</div>
<div> </div>
<div>+empty_args:</div>
<div> TRACE_REGSET(" arglist: Stack");</div>
<div> </div>
<div> /*</div>
</div>
</blockquote>
<br>
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.<br>
<br>
<blockquote type="cite" cite="mid:SN6PR10MB2975EA5A5D9ADFE9B62C2081C2689@SN6PR10MB2975.namprd10.prod.outlook.com">
<div style="font-family: Calibri, Arial, Helvetica, sans-serif;
font-size: 12pt; color: rgb(0, 0, 0);" class="elementToProof">
<div>@@ -6669,7 +6674,6 @@ dt_cg_agg(dt_pcb_t *pcb, dt_node_t
*dnp, dt_irlist_t *dlp, dt_regset_t *drp)</div>
<div> dnerror(dnp->dn_aggtup, D_ARR_BADREF,
"indexing is not "</div>
<div> "supported yet: @%s\n",
dnp->dn_ident->di_name);</div>
<div> </div>
<div>-</div>
<div> assert(fid->di_id >= DT_AGG_BASE &&
fid->di_id < DT_AGG_HIGHEST);</div>
<div> </div>
<div> dt_cg_clsflags(pcb, DTRACEACT_AGGREGATION, dnp);</div>
<div>diff --git a/libdtrace/dt_impl.h b/libdtrace/dt_impl.h</div>
<div>index c1ff6256..85a1e7c9 100644</div>
<div>--- a/libdtrace/dt_impl.h</div>
<div>+++ b/libdtrace/dt_impl.h</div>
<div>@@ -329,6 +329,7 @@ struct dtrace_hdl {</div>
<div> ctf_id_t dt_type_stack; /* cached CTF identifier for
stack type */</div>
<div> ctf_id_t dt_type_symaddr; /* cached CTF identifier
for _symaddr type */</div>
<div> ctf_id_t dt_type_usymaddr; /* cached CTF ident. for
_usymaddr type */</div>
<div>+ ctf_id_t dt_type_void; /* cached CTF identifier for
void type */</div>
<div> dtrace_epid_t dt_nextepid; /* next enabled probe ID
to assign */</div>
<div> size_t dt_maxprobe; /* max enabled probe ID */</div>
<div> dtrace_datadesc_t **dt_ddesc; /* probe data
descriptions */</div>
<div>diff --git a/libdtrace/dt_open.c b/libdtrace/dt_open.c</div>
<div>index f5cbd499..581172e2 100644</div>
<div>--- a/libdtrace/dt_open.c</div>
<div>+++ b/libdtrace/dt_open.c</div>
<div>@@ -1055,10 +1055,12 @@ dt_vopen(int version, int flags,
int *errp,</div>
<div> dtp->dt_type_usymaddr =
ctf_add_typedef(dmp->dm_ctfp, CTF_ADD_ROOT,</div>
<div> "_usymaddr", ctf_lookup_by_name(dmp->dm_ctfp,
"void"));</div>
<div> </div>
<div>+ dtp->dt_type_void =
ctf_lookup_by_name(dmp->dm_ctfp, "void");</div>
<div>+</div>
<div> if (dtp->dt_type_func == CTF_ERR ||
dtp->dt_type_fptr == CTF_ERR ||</div>
<div> dtp->dt_type_str == CTF_ERR ||
dtp->dt_type_dyn == CTF_ERR ||</div>
<div> dtp->dt_type_stack == CTF_ERR ||
dtp->dt_type_symaddr == CTF_ERR ||</div>
<div>- dtp->dt_type_usymaddr == CTF_ERR) {</div>
<div>+ dtp->dt_type_usymaddr == CTF_ERR ||
dtp->dt_type_void == CTF_ERR) {</div>
<div> dt_dprintf("failed to add intrinsic to D
container: %s\n",</div>
<div> ctf_errmsg(ctf_errno(dmp->dm_ctfp)));</div>
<div> return set_open_errno(dtp, errp, EDT_CTF);</div>
<div>-- </div>
<div>2.34.1</div>
<br>
</div>
<br>
<fieldset class="moz-mime-attachment-header"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
DTrace-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:DTrace-devel@oss.oracle.com">DTrace-devel@oss.oracle.com</a>
<a class="moz-txt-link-freetext" href="https://oss.oracle.com/mailman/listinfo/dtrace-devel">https://oss.oracle.com/mailman/listinfo/dtrace-devel</a>
</pre>
</blockquote>
</body>
</html>