[DTrace-devel] [PATCH] Fix typecast

[Kris Van Hees]

On Mon, Aug 01, 2022 at 02:40:28PM -0700, Eugene Loh via DTrace-devel wrote:
> 
> On 8/1/22 12:15, Kris Van Hees via DTrace-devel wrote:
> > DTrace internally keeps integers basically in a 64-bit format.  Shorter,
> > signed integers are already sign-extended.
> > 
> > The current logic makes an error when an integer is extended and the
> > signedness changes.  It truncates the source and then extends.  E.g.,
> > consider a 1-byte source and 2-byte destination:
> > 
> >      signed?
> >      src dst  input  expected  currently
> > 
> >       N   Y    0xff   0x00ff       -1
> >       Y   N    0xff   0xffff      0xff
> > 
> > Note:
> > 
> > *) In the first case, no casting is even necessary.  If the integer
> > is being extended and it is unsigned, then higher-order bits can simply
> > be left alone.
> > 
> > *) In the second case, the signed input is supposed to be sign-extended
> > before truncating to the new size.  We currently truncate first, at the
> > smaller size.
> > 
> > Fix.  Add tests.
> > 
> > Notice that:
> > 
> > *) Casting constants like "var = (type) constant" is handled
> >     in the parser.
> > 
> > *) Casting variables like "var1 = (type) var2" is handled
> >     in the code generator.
> > 
> > This patch is a modified version of an earlier patch by Eugene Loh.
> > 
> > Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_cg.c                       |  34 ++++----
> >   libdtrace/dt_parser.c                   |  14 ++--
> >   test/unittest/arithmetic/tst.cast-exp.d | 101 ++++++++++++++++++++++++
> >   test/unittest/arithmetic/tst.cast-exp.r |  43 ++++++++++
> >   4 files changed, 167 insertions(+), 25 deletions(-)
> >   create mode 100644 test/unittest/arithmetic/tst.cast-exp.d
> >   create mode 100644 test/unittest/arithmetic/tst.cast-exp.r
> > 
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 3e53c4bc..b40c9e48 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -2515,30 +2515,19 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >   {
> >   	size_t srcsize;
> >   	size_t dstsize;
> > -	int n;
> >   	/* If the destination type is '@' (any type) we need not cast. */
> >   	if (dst->dn_ctfp == NULL && dst->dn_type == CTF_ERR)
> >   		return;
> > -	srcsize = dt_node_type_size(src);
> > -	dstsize = dt_node_type_size(dst);
> > -
> > -	if (dstsize < srcsize)
> > -		n = sizeof(uint64_t) * NBBY - dstsize * NBBY;
> > -	else
> > -		n = sizeof(uint64_t) * NBBY - srcsize * NBBY;
> > -
> >   	if (!dt_node_is_scalar(dst))
> >   		return;
> > -	if (n != 0 && (dstsize < srcsize ||
> > -	    (src->dn_flags & DT_NF_SIGNED) ^ (dst->dn_flags & DT_NF_SIGNED))) {
> > -		emit(dlp, BPF_MOV_REG(dst->dn_reg, src->dn_reg));
> > -		emit(dlp, BPF_ALU64_IMM(BPF_LSH, dst->dn_reg, n));
> > -		emit(dlp, BPF_ALU64_IMM((dst->dn_flags & DT_NF_SIGNED) ? BPF_ARSH : BPF_RSH, dst->dn_reg, n));
> > -	} else if (dt_node_is_arith(dst) && dt_node_is_pointer(src) &&
> > -		   (src->dn_flags & DT_NF_ALLOCA)) {
> > +	srcsize = dt_node_type_size(src);
> > +	dstsize = dt_node_type_size(dst);
> > +
> > +	if (dt_node_is_arith(dst) && dt_node_is_pointer(src) &&
> > +	    (src->dn_flags & DT_NF_ALLOCA)) {
> >   		int mst;
> >   		if ((mst = dt_regset_alloc(drp)) == -1)
> > @@ -2550,6 +2539,19 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> >   		emit(dlp,  BPF_LOAD(BPF_DW, dst->dn_reg, mst, DMST_SCALARIZER));
> >   		dt_regset_free(drp, mst);
> > +	} else {
> > +		int srcsigned = src->dn_flags & DT_NF_SIGNED;
> > +		int dstsigned = dst->dn_flags & DT_NF_SIGNED;
> > +
> > +		if (dst->dn_reg != src->dn_reg)
> > +			emit(dlp, BPF_MOV_REG(dst->dn_reg, src->dn_reg));
> > +
> > +		if ((!srcsigned && dstsigned) && srcsize == dstsize) {
> > +			int n = (sizeof(uint64_t) - dstsize) * NBBY;
> > +
> > +			emit(dlp, BPF_ALU64_IMM(BPF_LSH, dst->dn_reg, n));
> > +			emit(dlp, BPF_ALU64_IMM(BPF_ARSH, dst->dn_reg, n));
> > +		}
> >   	}
> >   }
> 
> So if the src is signed and the dst unsigned, we leave the upper bits
> alone?  That suggests that we could have upper nonzero bits that will
> pollute the result unless we automatically store the result to truncate
> those upper bits.  So, yes, the tests need to be expanded.  I'll post a
> revised patch (with updated tests).

That is what I thought indeed,

> PS  No need for LSH/[A]RSH if n==0.

Yes, I forgot to add that when I moved code around.

> PPS  a long time ago, I thought you didn't like "if (dstreg!=srcreg)
> BPF_MOV_REG(dstreg, srcreg)" since you expected that sort of optimization to
> be done later in some code-optimization phase.

No, I didn't like making the assumption that src and dst *are* the same
register.  But I do think that it makes sense to ensure that we code the
ecurrently unused) case of srcreg != dstreg.

> Anyhow, I've been working on the revised tests.  The results match a gcc
> test program.  My patch passes with the expanded tests; this version of the
> patch does not.  I'm not convinced I fully understand what's going on,
> however, and hence the delay.  This has taken so long, so I figured i'd
> update you what's going on.

I think we should actually include the gcc programs somewhere, if that is our
reference.  Just having result files without any indication on how they were
created makes it a lot more difficult to ensure that the results *are* indeed
the correct output we should expect.