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

[Eugene Loh]

Thanks for the explanations.
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>


On 11/30/2020 12:39 PM, Kris Van Hees wrote:
> 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!