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

[Kris Van Hees]

On Thu, Feb 11, 2021 at 05:44:23PM -0500, Eugene Loh wrote:
> 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.

Ah ok, it mist have been another function for which I got rid of a similar
custom error code that no longer gets triggered because we catch it in the
proto-verification.

> > 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?

Oh yes, my bad.  I am not sure we can actually trigger AGGBAD anymore, so
let's ignore it for now that we do not have tests for it.  We may end up
getting rid of it as an obsolte error condition, but it is too early to tell
right now because the code that would trigger it is territory we haven't
touched yet.

> >> 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.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel