[DTrace-devel] [PATCH 3/5] Fix size for typecast tuple (and aggregation key) components

[Kris Van Hees]

On Thu, Sep 08, 2022 at 02:10:25PM -0400, Eugene Loh via DTrace-devel wrote:
> I think of this fix slightly differently.  The point is not that "size"
> needs to be updated;  rather, it was never computed correctly in the first
> place.  With this patch, "size" is computed once (incorrectly, but never
> really used) and then overwritten with the correct value.  It would be
> cleaner just to compute size correctly in the first place.  Instead of
> inserting the new correction, just go up a dozen lines and:
> 
> -               size = t.dtdt_size;
> +               size = dt_node_sizeof(&isp->dis_args[i]);

But then you invalidate the earlier test on whether the size of the node
is 0 (and if it is, ignore it that node).  I am not 100% certain under what
conditions that can happen, but since the legacy version did deem it needed
to put it there, I am not ready to remove it at this point.

> Also, the patch should have a test.  Here is a suggestion:

Test added (different from the one you suggested because a single case is quite
sufficient).

v2 posted.

> BEGIN {
>   @a[11111111, 22222222,        0x7fff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +1);
>   @a[11111111, 22222222, (int  )0x7fff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +2);
>   @a[11111111, 22222222, (short)0x7fff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +4);
>   @a[11111111, 22222222, (long )0x7fff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +8);
>   @a[11111111, 22222222,        0x7fffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +16);
>   @a[11111111, 22222222, (int  )0x7fffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +32);
>   @a[11111111, 22222222, (short)0x7fffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +64);
>   @a[11111111, 22222222, (long )0x7fffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +128);
>   @a[11111111, 22222222,        0xffff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +256);
>   @a[11111111, 22222222, (int  )0xffff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +512);
>   @a[11111111, 22222222, (short)0xffff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +1024);
>   @a[11111111, 22222222, (long )0xffff7fff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +2048);
>   @a[11111111, 22222222,        0xffffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +4096);
>   @a[11111111, 22222222, (int  )0xffffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +8192);
>   @a[11111111, 22222222, (short)0xffffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +16384);
>   @a[11111111, 22222222, (long )0xffffffff, 44444444, 55555555] =
> lquantize(3, 0, 10, 1, +32768);
>   exit(0);
> }
> 
> On 9/6/22 21:36, Kris Van Hees via DTrace-devel wrote:
> > Components of a tuple (or aggregation key) were being typecast where
> > needed but if narrowing or widening took place, the storage size to store
> > the value was never updated.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_cg.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index ffab2443..fae32516 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2765,6 +2765,9 @@ empty_args:
> >   		dt_cg_typecast(dnp, &isp->dis_args[i], dlp, drp);
> >   		isp->dis_args[i].dn_reg = -1;
> > +		/* The typecast may have changed the size. */
> > +		size = dt_node_sizeof(&isp->dis_args[i]);
> > +
> >   		if (dt_node_is_scalar(dnp) || dt_node_is_float(dnp)) {
> >   			assert(size > 0 && size <= 8 &&
> >   			       (size & (size - 1)) == 0);
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel