[DTrace-devel] [PATCH] Fix typecast

[Eugene Loh]

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).

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

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.

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.