[DTrace-devel] [PATCH v2 3/5] Include aggregates in the DIFO vartab

[Kris Van Hees]

On Mon, Nov 30, 2020 at 11:56:18AM -0800, Eugene Loh wrote:
> I'm curious about two things:
> 
> 
> On 11/24/2020 01:50 PM, Kris Van Hees wrote:
> > Aggregates used to be handled as special identifiers.  Under the new
> > design they will be handled more like variables and it is therefore
> > warranted to include them in the variable table.
> >
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   include/dtrace/dif_defines.h |  1 +
> >   libdtrace/dt_as.c            | 24 +++++++++++++++++++-----
> >   libdtrace/dt_dis.c           |  3 +++
> >   3 files changed, 23 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/dtrace/dif_defines.h b/include/dtrace/dif_defines.h
> > index b983c45c..216bce42 100644
> > --- a/include/dtrace/dif_defines.h
> > +++ b/include/dtrace/dif_defines.h
> > @@ -275,6 +275,7 @@ typedef uint32_t	dif_instr_t;
> >   
> >   #define DIFV_KIND_ARRAY		0
> >   #define DIFV_KIND_SCALAR	1
> > +#define DIFV_KIND_AGGREGATE	2
> >   
> >   #define DIFV_SCOPE_GLOBAL	0
> >   #define DIFV_SCOPE_THREAD	1
> > diff --git a/libdtrace/dt_as.c b/libdtrace/dt_as.c
> > index 3169cf66..437a2d86 100644
> > --- a/libdtrace/dt_as.c
> > +++ b/libdtrace/dt_as.c
> > @@ -61,7 +61,9 @@ dt_countvar(dt_idhash_t *dhp, dt_ident_t *idp, void *data)
> >   	size_t *np = data;
> >   
> >   	if (idp->di_flags & (DT_IDFLG_DIFR | DT_IDFLG_DIFW))
> > -		(*np)++; /* include variable in vartab */
> > +		(*np)++;		/* include variable in vartab */
> > +	else if (idp->di_kind == DT_IDENT_AGG)
> > +		(*np)++;		/* include variable in vartab */
> 
> How about collapsing those to be:
>          if (idp->di_flags & (DT_IDFLG_DIFR | DT_IDFLG_DIFW))||
>              idp->di_kind == DT_IDENT_AGG)
>                  (*np)++;        /* include variable in vartab */
> 
> That would be more compact and would also mimic the expression in 
> dt_copyvar(), seen immediately following.

I actually think the better action is to split up the one in dt_copyvar().
That way it is more clear because one conditional is on di_flags and the
other is on di_kind.

> >   	return (0);
> >   }
> > @@ -76,8 +78,9 @@ dt_copyvar(dt_idhash_t *dhp, dt_ident_t *idp, dtrace_hdl_t *dtp)
> >   	ssize_t		stroff;
> >   	dt_node_t	dn;
> >   
> > -	if (!(idp->di_flags & (DT_IDFLG_DIFR | DT_IDFLG_DIFW)))
> > -		return (0); /* omit variable from vartab */
> > +	if (!(idp->di_flags & (DT_IDFLG_DIFR | DT_IDFLG_DIFW)) &&
> > +	    idp->di_kind != DT_IDENT_AGG)
> > +		return 0;		/* omit variable from vartab */
> >   
> >   	dvp = &pcb->pcb_difo->dtdo_vartab[pcb->pcb_asvidx++];
> >   	stroff = dt_strtab_insert(dtp->dt_ccstab, idp->di_name);
> > @@ -91,8 +94,17 @@ dt_copyvar(dt_idhash_t *dhp, dt_ident_t *idp, dtrace_hdl_t *dtp)
> >   	dvp->dtdv_id = idp->di_id;
> >   	dvp->dtdv_flags = 0;
> >   
> > -	dvp->dtdv_kind = (idp->di_kind == DT_IDENT_ARRAY) ?
> > -	    DIFV_KIND_ARRAY : DIFV_KIND_SCALAR;
> > +	switch (idp->di_kind) {
> > +	case DT_IDENT_AGG:
> > +		dvp->dtdv_kind = DIFV_KIND_AGGREGATE;
> > +		break;
> > +	case DT_IDENT_ARRAY:
> > +		dvp->dtdv_kind = DIFV_KIND_ARRAY;
> > +		break;
> > +	default:
> > +		dvp->dtdv_kind = DIFV_KIND_SCALAR;
> > +	}
> > +
> >   
> >   	if (idp->di_flags & DT_IDFLG_LOCAL)
> >   		dvp->dtdv_scope = DIFV_SCOPE_LOCAL;
> > @@ -358,6 +370,7 @@ fail:
> >   	 * table we insert the corresponding variable names into the strtab.
> >   	 */
> >   	dt_idhash_iter(dtp->dt_tls, dt_countvar, &n);
> > +	dt_idhash_iter(dtp->dt_aggs, dt_countvar, &n);
> >   	dt_idhash_iter(dtp->dt_globals, dt_countvar, &n);
> >   	dt_idhash_iter(pcb->pcb_locals, dt_countvar, &n);
> >   
> > @@ -369,6 +382,7 @@ fail:
> >   			longjmp(pcb->pcb_jmpbuf, EDT_NOMEM);
> >   
> >   		dt_idhash_iter(dtp->dt_tls, (dt_idhash_f *)dt_copyvar, dtp);
> > +		dt_idhash_iter(dtp->dt_aggs, (dt_idhash_f *)dt_copyvar, dtp);
> >   		dt_idhash_iter(dtp->dt_globals, (dt_idhash_f *)dt_copyvar, dtp);
> >   		dt_idhash_iter(pcb->pcb_locals, (dt_idhash_f *)dt_copyvar, dtp);
> >   	}
> > diff --git a/libdtrace/dt_dis.c b/libdtrace/dt_dis.c
> > index 342db62b..b10c3867 100644
> > --- a/libdtrace/dt_dis.c
> > +++ b/libdtrace/dt_dis.c
> > @@ -663,6 +663,9 @@ dt_dis_difo(const dtrace_difo_t *dp, FILE *fp)
> >   		char kind[4], scope[4], range[12], flags[16] = { 0 };
> >   
> >   		switch (v->dtdv_kind) {
> > +		case DIFV_KIND_AGGREGATE:
> > +			strcpy(kind, "agg");
> > +			break;
> >   		case DIFV_KIND_ARRAY:
> >   			strcpy(kind, "arr");
> >   			break;
> 
> I realize that this is not yet sufficient to generate disassembly for 
> aggregations, but I'm just wondering what tests we have for this.  
> Actually, hang on, do I not know where to look, or is it the case that 
> we have absolutely no tests of any kind for disassembly? That would seem 
> to be a big omission.

There are no tests for the disassembly output generation.  There never have
been and, well, none were ever added with the implementation on Linux and
the many years since then.

Yes, we should add tests for this and other aspects of disassembly output.
That will have to be a todo item for the (near) future.  It isn't that high
of a priority though since disassembly output is really primarily for
debugging purposes and thus issues with that are commonly discovered well
before a testsuite run takes place.

But yes, good point!