[DTrace-devel] [PATCH CORRECTED] Implement normalize() and denormalize()

[Kris Van Hees]

On Fri, Jan 15, 2021 at 12:17:40PM -0800, Eugene Loh wrote:
> Actually, I have another question here.  For dt_cg_[de]normalize(), you 
> introduce an argc to count the number of arguments and check if it's the 
> expected value.
> 
> At least for denormalize(), however, that doesn't make sense.  If the 
> number of arguments is wrong, you'll never get to this new 
> dt_cg_denormalize() code.  You will already have failed with PROTO_LEN.  
> In fact, you've added new tests to check just that.

Yes, which means the implementation is even easier than it already is.  I did
a copy'n'paste from normalize() to write denormalize() and actually didn't
pay attention to this.  (And when I wrote the tests, I didn't bother to check
the code of course - my bad when doing multiple things at once.)

Thanks for pointing this out.

> For normalize(), I guess(?) the new argc code is okay.  The case of "too 
> few args" seems to be caught by the new code, even if I do not 
> understand why PROTO_LEN hasn't complained.

Well, the problem is that the prototype is defined with the 2nd argument being
optional while the implementation of normalize() (in all legacy versions)
clearly shows that both arguments are required.

So, I think that the correct action is to actually get rid of NORMALIZE_PROTO
as an error tag and let the prototype checking code do its job and flag
PROTO_LEN here as error.  I can easily do this by making the 2nd argument not
optional since it clearly shouldn't be.  And since we have perfectly fine
prototype checking code, we should use it.

It introduces another small backwards compatibility break but I think it is
certainly for the better and I doubt there are users of this code that actually
care about the specific error tag used for this type of error.

So, I will change the prototype to actually reflect the correct expectation,
and get rid of unnecessary action specific code.

> On 01/14/2021 09:04 AM, Kris Van Hees wrote:
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -609,33 +609,36 @@ dt_cg_act_commit(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >   static void
> >   dt_cg_act_denormalize(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >   {
> > -	dt_node_t *anp;
> > -	dt_ident_t *aid;
> > -	char n[DT_TYPE_NAMELEN];
> > +	dt_node_t	*anp;
> > +	int		argc = 0;
> > +	char		n[DT_TYPE_NAMELEN];
> > +	dt_ident_t	*aid;
> > +
> > +	/* Count the arguments. */
> > +	for (anp = dnp->dn_args; anp != NULL; anp = anp->dn_list)
> > +		argc++;
> > +
> > +	if (argc != 1)
> > +		dnerror(dnp, D_NORMALIZE_PROTO,
> > +		    "%s( ) prototype mismatch: %d args passed, %d expected\n",
> > +		    dnp->dn_ident->di_name, argc, 1);
> >   
> > @@ -730,6 +733,43 @@ dt_cg_act_jstack(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >   static void
> >   dt_cg_act_normalize(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind)
> >   {
> > +	dt_node_t	*anp, *normal;
> > +	int		argc = 0;
> > +	char		n[DT_TYPE_NAMELEN];
> > +	dt_ident_t	*aid;
> > +
> > +	/* Count the arguments. */
> > +	for (anp = dnp->dn_args; anp != NULL; anp = anp->dn_list)
> > +		argc++;
> > +
> > +	if (argc != 2)
> > +		dnerror(dnp, D_NORMALIZE_PROTO,
> > +		    "%s( ) prototype mismatch: %d args passed, %d expected\n",
> > +		    dnp->dn_ident->di_name, argc, 2);
> > +
> > diff --git a/test/unittest/aggs/err.D_NORMALIZE_PROTO.bad.d ...
> > deleted file mode 100644
> > diff --git a/test/unittest/aggs/err.D_NORMALIZE_PROTO.norm_too_few_args.d ...
> > new file mode 100644
> > diff --git a/test/unittest/aggs/err.D_PROTO_LEN.denorm_too_few_args.d ...
> > new file mode 100644
> > diff --git a/test/unittest/aggs/err.D_PROTO_LEN.denorm_too_many_args.d ...
> > new file mode 100644
> > diff --git a/test/unittest/aggs/err.D_PROTO_LEN.norm_too_many_args.d ...
> > new file mode 100644
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel