[DTrace-devel] [PATCH] Clean up argument processing (and its testing) for trunc()

[Eugene Loh]

On 2/11/21 4:07 PM, Kris Van Hees wrote:
> On Thu, Jan 21, 2021 at 04:33:20PM -0500, eugene.loh at oracle.com wrote:
>> From: Eugene Loh <eugene.loh at oracle.com>
>>
>> Add some argument vetting to dt_cg_act_trunc() and write the
>> action to the output buffer in anticipation of a future patch
>> that will implement trunc() on the consumer side.
>>
>> Clean up associated testing.  Note these name changes:
>>    err.D_TRUNC_PROTO.badmany.d -> err.D_PROTO_LEN.trunc_too_many_args.d
>>    err.D_TRUNC_PROTO.badnone.d -> err.D_PROTO_LEN.trunc_too_few_args.d
>>    err.D_TRUNC_SCALAR.bad.d    -> err.D_PROTO_ARG.trunc_non_scalar_trunc.d
>>    err.D_TRUNC_AGGARG.bad.d    -> err.D_TRUNC_AGGARG.trunc_no_agg.d
>>
>> Further clean up is warranted.  For example, it appears that earlier
>> argument checking means that dt_cg_act_trunc()'s D_TRUNC_SCALAR check
>> will never fail -- that is, D_TRUNC_SCALAR is obsolete.  Also, none of
>> the D_*_AGGBAD errors are tested.
> Ah, that is a good thing.  Amazing what happens when code makes use of the
> facilities that have been put in place for generic argument checking for
> functions.  I think that making the change to remove D_TRUNC_SCALAR is a good
> choice
Okay.  I'll remove D_TRUNC_SCALAR.
> (I think we did the same for clear(), right).
There apparently was never a D_CLEAR_SCALAR.
> And adding some tests
> for the D_*_AGGBAD errors would be something I would add to this patch as
> well.  It only makes things better.
Presumably you only mean for D_TRUNC_AGGBAD, but not for the other 
D_*_AGGBAD since this patch is trunc().  Yes?

Anyhow, I don't know how to trigger D_*_AGGBAD.  Maybe this is another 
dead error mode.  I try trunc(@a), where @a is undefined, but it fails 
with D_IDENT_UNDEF.  What should trigger D_*_AGGBAD?
>> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
>> @@ -1072,16 +1072,14 @@ dt_cg_act_trace(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
>>   		        "data-recording actions may not follow commit( )\n");
>>   	*cfp |= DT_CLSFLAG_DATAREC;
>>   
>> -	if (dt_node_is_void(arg)) {
>> +	if (dt_node_is_void(arg))
>>   		dnerror(arg, D_TRACE_VOID,
>>   			"trace( ) may not be applied to a void expression\n");
>> -	}
>>   
>> -	if (dt_node_is_dynamic(arg)) {
>> +	if (dt_node_is_dynamic(arg))
>>   		dnerror(arg, D_TRACE_DYN,
>>   			"trace( ) may not be applied to a dynamic "
>>   			"expression\n");
>> -	}
> Hm, did we somehow overlook these in the big coding convention patch?  This
> really should be in its own tiny patch, because it has nothing to do with
> trunc().
I'm sure there are plenty of these left over.  It also seemed to me that 
we routinely used to include such unrelated stylistic changes in our 
patches... the whole reason for doing the big patch.  Anyhow, I'll pull 
to its own patch.