[DTrace-devel] [PATCH v4 22/61] Fix typecast
Kris Van Hees
kris.van.hees at oracle.com
Thu Aug 4 12:52:29 UTC 2022
On Wed, Aug 03, 2022 at 07:12:11PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> 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.
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
... with small changes (mostly code movement) below
... queued for dev
> ---
> libdtrace/dt_cg.c | 44 ++++---
> libdtrace/dt_parser.c | 24 ++--
> test/unittest/arithmetic/tst.cast-exp.d | 157 ++++++++++++++++++++++++
> test/unittest/arithmetic/tst.cast-exp.r | 75 +++++++++++
> 4 files changed, 276 insertions(+), 24 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 f77e239b..8a023729 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -2516,23 +2516,13 @@ dt_cg_typecast(const dt_node_t *src, const dt_node_t *dst,
> 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) &&
> + srcsize = dt_node_type_size(src);
> + dstsize = dt_node_type_size(dst);
This can move into the else-cause of the conditional clause because that is the
only place it is used. So, move the declaration of srcsize, dstsize, and n
into the code block of the sub-clause also.
> +
> + if (dt_node_is_arith(dst) && dt_node_is_pointer(src) &&
> (src->dn_flags & DT_NF_ALLOCA)) {
> int mst;
>
> @@ -2545,8 +2535,30 @@ 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
> - emit(dlp, BPF_MOV_REG(dst->dn_reg, src->dn_reg));
> + } else {
> + int cast = 1;
> + int srcsigned = src->dn_flags & DT_NF_SIGNED;
> + int dstsigned = dst->dn_flags & DT_NF_SIGNED;
With the above, this becomes:
int srcsigned = src->dn_flags & DT_NF_SIGNED;
int dstsigned = dst->dn_flags & DT_NF_SIGNED;
size_t srcsize = dt_node_type_size(src);
size_t dstsize = dt_node_type_size(dst);
int n = (sizeof(uint64_t) - dstsize) * NBBY;
int cast = 1;
> +
> + if (dst->dn_reg != src->dn_reg)
> + emit(dlp, BPF_MOV_REG(dst->dn_reg, src->dn_reg));
> +
> + n = sizeof(uint64_t) * NBBY - dstsize * NBBY;
Not needed - done above.
> + if (n == 0) {
> + cast = 0;
> + } else if (dstsize > srcsize) {
> + if (dstsigned || !srcsigned)
> + cast = 0;
> + } else if (dstsize == srcsize) {
> + if (dstsigned == srcsigned)
> + cast = 0;
> + }
> +
> + if (cast) {
> + emit(dlp, BPF_ALU64_IMM(BPF_LSH, dst->dn_reg, n));
> + emit(dlp, BPF_ALU64_IMM(dstsigned ? BPF_ARSH : BPF_RSH, dst->dn_reg, n));
> + }
> + }
> }
>
> /*
> diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> index fd89b2c7..2cac1a71 100644
> --- a/libdtrace/dt_parser.c
> +++ b/libdtrace/dt_parser.c
> @@ -2082,18 +2082,26 @@ dt_node_op2(int op, dt_node_t *lp, dt_node_t *rp)
> dt_node_is_integer(lp)) {
> size_t srcsize = dt_node_type_size(rp);
> size_t dstsize = dt_node_type_size(lp);
> + int n = sizeof(uint64_t) * NBBY - dstsize * NBBY, cast = 1;
Split these in two lines for clarity. And I wuold again use distributive
property of * over +, so n = (sizeof(uint64_t) - dstsize) * NBBY, since we
care about difference in number of bytes in the sizss, and then multiply
by bits-per-byte to get the shift value.
> + int srcsigned = rp->dn_flags & DT_NF_SIGNED;
> + int dstsigned = lp->dn_flags & DT_NF_SIGNED;
> +
> + if (n == 0) {
> + cast = 0;
> + } else if (dstsize > srcsize) {
> + if (dstsigned || !srcsigned)
> + cast = 0;
> + } else if (dstsize == srcsize) {
> + if (dstsigned == srcsigned)
> + cast = 0;
> + }
>
> - if ((dstsize < srcsize) || ((lp->dn_flags & DT_NF_SIGNED) ^
> - (rp->dn_flags & DT_NF_SIGNED))) {
> - int n = dstsize < srcsize ?
> - (sizeof(uint64_t) * NBBY - dstsize * NBBY) :
> - (sizeof(uint64_t) * NBBY - srcsize * NBBY);
> -
> + if (cast) {
> rp->dn_value <<= n;
> - if (lp->dn_flags & DT_NF_SIGNED)
> + if (dstsigned)
> rp->dn_value = (intmax_t)rp->dn_value >> n;
> else
> - rp->dn_value = rp->dn_value >> n;
> + rp->dn_value >>= n;
> }
>
> dt_node_type_propagate(lp, rp);
> diff --git a/test/unittest/arithmetic/tst.cast-exp.d b/test/unittest/arithmetic/tst.cast-exp.d
> new file mode 100644
> index 00000000..25e3fc16
> --- /dev/null
> +++ b/test/unittest/arithmetic/tst.cast-exp.d
> @@ -0,0 +1,157 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Integers are typecast correctly. (explicit)
> + * The .r results file can be generated by copying this file
> + * to a .c file and compiling the resulting C program.
> + *
> + * SECTION: Types, Operators, and Expressions/Arithmetic Operators
> + */
> +/* @@runtest-opts: -qC -DUSE_AS_D_SCRIPT */
> +
> +#ifndef USE_AS_D_SCRIPT
> +#include <stdio.h>
> +#endif
> +
> +signed char c, c0;
> +short s, s0;
> +int i, i0;
> +long long l, l0;
> +unsigned char C, C0;
> +unsigned short S, S0;
> +unsigned int I, I0;
> +unsigned long long L, L0;
> +
> +#ifdef USE_AS_D_SCRIPT
> +#define FMT "%d %d %d %d %d %d %d %d\n"
> +#else
> +#define FMT "%hhd %hd %d %lld %hhu %hu %u %llu\n"
> +#endif
> +
> +#define TEST(x) \
> + c = (signed char)(x); \
> + s = (short)(x); \
> + i = (int)(x); \
> + l = (long long)(x); \
> + C = (unsigned char)(x); \
> + S = (unsigned short)(x); \
> + I = (unsigned int)(x); \
> + L = (unsigned long long)(x); \
> + printf(FMT, c, s, i, l, C, S, I, L)
> +
> +#ifdef USE_AS_D_SCRIPT
> +BEGIN
> +#else
> +int main(int c_unused, char **v_unused)
> +#endif
> +{
> + /* from scalar */
> + TEST(-2);
> + TEST(0xfffffffffffffffe);
> + TEST(0xfffffffe);
> + TEST(0xfffe);
> + TEST(0xfe);
> + TEST(2);
> + TEST(0x55);
> + TEST(0x5555);
> + TEST(0x55555555);
> + TEST(0x5555555555555555);
> +
> + /* from signed char */
> + c0 = -2; TEST(c0);
> + c0 = 0xfe; TEST(c0);
> + c0 = 2; TEST(c0);
> + c0 = 0x55; TEST(c0);
> +
> + /* from short */
> + s0 = -2; TEST(s0);
> + s0 = 0xfffe; TEST(s0);
> + s0 = 2; TEST(s0);
> + s0 = 0x5555; TEST(s0);
> +
> + /* from int */
> + i0 = -2; TEST(i0);
> + i0 = 0xfffffffe; TEST(i0);
> + i0 = 2; TEST(i0);
> + i0 = 0x55555555; TEST(i0);
> +
> + /* from long long */
> + l0 = -2; TEST(l0);
> + l0 = 0xfffffffffffffffe; TEST(l0);
> + l0 = 2; TEST(l0);
> + l0 = 0x5555555555555555; TEST(l0);
> +
> + /* from unsigned char */
> + C0 = -2; TEST(C0);
> + C0 = 0xfe; TEST(C0);
> + C0 = 2; TEST(C0);
> + C0 = 0x55; TEST(C0);
> +
> + /* from unsigned short */
> + S0 = -2; TEST(S0);
> + S0 = 0xfffe; TEST(S0);
> + S0 = 2; TEST(S0);
> + S0 = 0x5555; TEST(S0);
> +
> + /* from unsigned int */
> + I0 = -2; TEST(I0);
> + I0 = 0xfffffffe; TEST(I0);
> + I0 = 2; TEST(I0);
> + I0 = 0x55555555; TEST(I0);
> +
> + /* from unsigned long long */
> + L0 = -2; TEST(L0);
> + L0 = 0xfffffffffffffffe; TEST(L0);
> + L0 = 2; TEST(L0);
> + L0 = 0x5555555555555555; TEST(L0);
> +
> + /* from other constant expressions */
> + TEST((signed char)(-1));
> + TEST((short)(-1));
> + TEST((int)(-1));
> + TEST((long long)(-1));
> + TEST((unsigned char)(-1));
> + TEST((unsigned short)(-1));
> + TEST((unsigned int)(-1));
> + TEST((unsigned long long)(-1));
> +
> + TEST((long long)(signed char)(-1));
> + TEST((long long)(short)(-1));
> + TEST((long long)(int)(-1));
> + TEST((long long)(long long)(-1));
> + TEST((long long)(unsigned char)(-1));
> + TEST((long long)(unsigned short)(-1));
> + TEST((long long)(unsigned int)(-1));
> + TEST((long long)(unsigned long long)(-1));
> +
> + /* from other expressions */
> + l0 = -1;
> + TEST((signed char)l0);
> + TEST((short)l0);
> + TEST((int)l0);
> + TEST((long long)l0);
> + TEST((unsigned char)l0);
> + TEST((unsigned short)l0);
> + TEST((unsigned int)l0);
> + TEST((unsigned long long)l0);
> +
> + TEST((long long)(signed char)l0);
> + TEST((long long)(short)l0);
> + TEST((long long)(int)l0);
> + TEST((long long)(long long)l0);
> + TEST((long long)(unsigned char)l0);
> + TEST((long long)(unsigned short)l0);
> + TEST((long long)(unsigned int)l0);
> + TEST((long long)(unsigned long long)l0);
> +
> +#ifdef USE_AS_D_SCRIPT
> + exit (0);
> +#else
> + return 0;
> +#endif
> +}
> diff --git a/test/unittest/arithmetic/tst.cast-exp.r b/test/unittest/arithmetic/tst.cast-exp.r
> new file mode 100644
> index 00000000..c2601193
> --- /dev/null
> +++ b/test/unittest/arithmetic/tst.cast-exp.r
> @@ -0,0 +1,75 @@
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 4294967294 254 65534 4294967294 4294967294
> +-2 -2 65534 65534 254 65534 65534 65534
> +-2 254 254 254 254 254 254 254
> +2 2 2 2 2 2 2 2
> +85 85 85 85 85 85 85 85
> +85 21845 21845 21845 85 21845 21845 21845
> +85 21845 1431655765 1431655765 85 21845 1431655765 1431655765
> +85 21845 1431655765 6148914691236517205 85 21845 1431655765 6148914691236517205
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +2 2 2 2 2 2 2 2
> +85 85 85 85 85 85 85 85
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +2 2 2 2 2 2 2 2
> +85 21845 21845 21845 85 21845 21845 21845
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +2 2 2 2 2 2 2 2
> +85 21845 1431655765 1431655765 85 21845 1431655765 1431655765
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +2 2 2 2 2 2 2 2
> +85 21845 1431655765 6148914691236517205 85 21845 1431655765 6148914691236517205
> +-2 254 254 254 254 254 254 254
> +-2 254 254 254 254 254 254 254
> +2 2 2 2 2 2 2 2
> +85 85 85 85 85 85 85 85
> +-2 -2 65534 65534 254 65534 65534 65534
> +-2 -2 65534 65534 254 65534 65534 65534
> +2 2 2 2 2 2 2 2
> +85 21845 21845 21845 85 21845 21845 21845
> +-2 -2 -2 4294967294 254 65534 4294967294 4294967294
> +-2 -2 -2 4294967294 254 65534 4294967294 4294967294
> +2 2 2 2 2 2 2 2
> +85 21845 1431655765 1431655765 85 21845 1431655765 1431655765
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +-2 -2 -2 -2 254 65534 4294967294 18446744073709551614
> +2 2 2 2 2 2 2 2
> +85 21845 1431655765 6148914691236517205 85 21845 1431655765 6148914691236517205
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 255 255 255 255 255 255 255
> +-1 -1 65535 65535 255 65535 65535 65535
> +-1 -1 -1 4294967295 255 65535 4294967295 4294967295
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 255 255 255 255 255 255 255
> +-1 -1 65535 65535 255 65535 65535 65535
> +-1 -1 -1 4294967295 255 65535 4294967295 4294967295
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 255 255 255 255 255 255 255
> +-1 -1 65535 65535 255 65535 65535 65535
> +-1 -1 -1 4294967295 255 65535 4294967295 4294967295
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +-1 255 255 255 255 255 255 255
> +-1 -1 65535 65535 255 65535 65535 65535
> +-1 -1 -1 4294967295 255 65535 4294967295 4294967295
> +-1 -1 -1 -1 255 65535 4294967295 18446744073709551615
> +
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list