[DTrace-devel] [PATCH 4/6] Fix bit-field operations
Kris Van Hees
kris.van.hees at oracle.com
Wed Mar 31 19:50:21 PDT 2021
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
... with comments below
On Fri, Mar 19, 2021 at 01:45:32PM -0400, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Fix bugs in the dt_cg_field_[set|get] functions.
>
> Add tests for the functionality. Note that, due to an earlier patch
> ("6d9c2398 Provide a more consistent implementation of the trace() action"),
> results for signed bit fields with a nonzero sign bit differ from the
> incorrect results of DTv1. Current results match GCC.
This is confusing. I don't think we need to make any reference to DTrace v1
(the original DTrace port to Linux that we did years ago). The tests do not
exist in the earlier version, so there is no need to compare. You could
mention that you verified the behaviour with this patch and that it matches
the legacy implementation or something like that.
> (Also note that bit fields are not packed together as efficiently as
> possible. Rather, following legacy DTrace behavior, each bit field is
> aligned to the size of the next largest integer type. Thus, simplifications
> in BPF code generation can be imagined. Or, bit fields should be packed more
> tightly together, in a departure from earlier DTrace implementations but
> bringing behavior in line with common user expectations.)
One consideration here os whether this has an impact on accessing kernel
structures. If a kernel structure contains a bit field and we use that
datatype (or the structure) to declare a D variable, can we access the
content of the bit field in the kernel structure correctly? And also any
other data, e.g. other bit fields? If GCC packs them together, but D is
expecting the datatype to have them stored differently we will run into
trouble. This is something you may want to test on the legacy implementation
because I don't think we can really access the content of kernel structures
just yet. But knowing whether there is an issue here would help fine tune
the above paragraph in the commit message.
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
> libdtrace/dt_cg.c | 65 ++++++++-----------
> test/unittest/bitfields/tst.BasicSigned.d | 54 +++++++++++++++
> test/unittest/bitfields/tst.BasicSigned.r | 3 +
> test/unittest/bitfields/tst.BasicUnsigned.d | 54 +++++++++++++++
> test/unittest/bitfields/tst.BasicUnsigned.r | 3 +
> .../bitfields/tst.BitFieldPromotion.d | 3 +-
> 6 files changed, 143 insertions(+), 39 deletions(-)
> create mode 100644 test/unittest/bitfields/tst.BasicSigned.d
> create mode 100644 test/unittest/bitfields/tst.BasicSigned.r
> create mode 100644 test/unittest/bitfields/tst.BasicUnsigned.d
> create mode 100644 test/unittest/bitfields/tst.BasicUnsigned.r
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index da3e9ca6..27b32c87 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1698,20 +1698,7 @@ dt_cg_ptrsize(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
>
> /*
> * If the result of a "." or "->" operation is a bit-field, we use this routine
> - * to generate an epilogue to the load instruction that extracts the value. In
> - * the diagrams below the "ld??" is the load instruction that is generated to
> - * load the containing word that is generating prior to calling this function.
> - *
> - * Epilogue for unsigned fields: Epilogue for signed fields:
> - *
> - * ldu? [r1], r1 lds? [r1], r1
> - * setx USHIFT, r2 setx 64 - SSHIFT, r2
> - * srl r1, r2, r1 sll r1, r2, r1
> - * setx (1 << bits) - 1, r2 setx 64 - bits, r2
> - * and r1, r2, r1 sra r1, r2, r1
> - *
> - * The *SHIFT constants above changes value depending on the endian-ness of our
> - * target architecture. Refer to the comments below for more details.
> + * to generate an epilogue to the load instruction that extracts the value.
> */
> static void
> dt_cg_field_get(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> @@ -1737,10 +1724,13 @@ dt_cg_field_get(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> * must subtract (ctm_offset % NBBY + cte_bits) from the size in bits
> * we used for the load. The size of our load in turn is found by
> * rounding cte_bits up to a byte boundary and then finding the
> - * nearest power of two to this value (see clp2(), above). These
> - * properties are used to compute shift as USHIFT or SSHIFT, below.
> + * nearest power of two to this value (see clp2(), above).
> */
> if (dnp->dn_flags & DT_NF_SIGNED) {
> + /*
> + * r1 <<= 64 - shift
> + * r1 >>= 64 - bits
> + */
> #ifdef _BIG_ENDIAN
> shift = clp2(P2ROUNDUP(e.cte_bits, NBBY) / NBBY) * NBBY -
> mp->ctm_offset % NBBY;
> @@ -1750,35 +1740,26 @@ dt_cg_field_get(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> emit(dlp, BPF_ALU64_IMM(BPF_LSH, r1, 64 - shift));
> emit(dlp, BPF_ALU64_IMM(BPF_ARSH, r1, 64 - e.cte_bits));
> } else {
> + /*
> + * r1 >>= shift
> + * r1 &= (1 << bits) - 1
> + */
> #ifdef _BIG_ENDIAN
> shift = clp2(P2ROUNDUP(e.cte_bits, NBBY) / NBBY) * NBBY -
> (mp->ctm_offset % NBBY + e.cte_bits);
> #else
> shift = mp->ctm_offset % NBBY;
> #endif
> - emit(dlp, BPF_ALU64_IMM(BPF_LSH, r1, shift));
> + emit(dlp, BPF_ALU64_IMM(BPF_RSH, r1, shift));
> emit(dlp, BPF_ALU64_IMM(BPF_AND, r1, (1ULL << e.cte_bits) - 1));
> }
> }
>
> /*
> - * If the destination of a store operation is a bit-field, we use this routine
> - * to generate a prologue to the store instruction that loads the surrounding
> - * bits, clears the destination field, and ORs in the new value of the field.
> - * In the diagram below the "st?" is the store instruction that is generated to
> - * store the containing word that is generating after calling this function.
> - *
> - * ld [dst->dn_reg], r1
> - * setx ~(((1 << cte_bits) - 1) << (ctm_offset % NBBY)), r2
> - * and r1, r2, r1
> - *
> - * setx (1 << cte_bits) - 1, r2
> - * and src->dn_reg, r2, r2
> - * setx ctm_offset % NBBY, r3
> - * sll r2, r3, r2
> - *
> - * or r1, r2, r1
> - * st? r1, [dst->dn_reg]
> + * If the destination of a store operation is a bit-field, we first use this
> + * routine to load the surrounding bits, clear the destination field, and OR
> + * in the new value of the field. After this routine, we will store the
> + * generated word.
> *
> * This routine allocates a new register to hold the value to be stored and
> * returns it. The caller is responsible for freeing this register later.
> @@ -1838,11 +1819,21 @@ dt_cg_field_set(dt_node_t *src, dt_irlist_t *dlp,
> fmask = (1ULL << e.cte_bits) - 1;
> cmask = ~(fmask << shift);
>
> - /* FIXME: Does not handled signed or userland */
> + /*
> + * r1 = [dst->dn_reg]
> + * r2 = cmask
> + * r1 &= r2
> + * r2 = fmask
> + * r2 &= src->dn_reg
> + * r2 <<= shift
> + * r1 |= r2
> + */
> + /* FIXME: Does not handle userland */
> emit(dlp, BPF_LOAD(dt_cg_load(dst, fp, m.ctm_type), r1, dst->dn_reg, 0));
> - emit(dlp, BPF_ALU64_IMM(BPF_AND, r1, cmask));
> - dt_cg_setx(dlp, r2, fmask);
> + dt_cg_setx(dlp, r2, cmask);
> emit(dlp, BPF_ALU64_REG(BPF_AND, r1, r2));
> + dt_cg_setx(dlp, r2, fmask);
> + emit(dlp, BPF_ALU64_REG(BPF_AND, r2, src->dn_reg));
> emit(dlp, BPF_ALU64_IMM(BPF_LSH, r2, shift));
> emit(dlp, BPF_ALU64_REG(BPF_OR, r1, r2));
> dt_regset_free(drp, r2);
> diff --git a/test/unittest/bitfields/tst.BasicSigned.d b/test/unittest/bitfields/tst.BasicSigned.d
> new file mode 100644
> index 00000000..00c0441d
> --- /dev/null
> +++ b/test/unittest/bitfields/tst.BasicSigned.d
> @@ -0,0 +1,54 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2021, 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: Bit-field store and load work properly.
> + *
> + * SECTION: Structs and Unions/Bit-Fields
> + */
> +
> +#pragma D option quiet
> +
> +struct bitRecord{
> + int a : 3;
> + int b : 7;
> + int c : 14;
> + long d : 35;
> + int e : 22;
> + int f : 32;
> + int g : 16;
> + int h : 8;
> +} var;
> +
> +BEGIN
> +{
> + /* sign bit is 0 */
> + var.a = 3;
> + var.b = 0x3f;
> + var.c = 0x1fff;
> + var.d = 0x3ffffffff;
> + var.e = 0x1fffff;
> + var.f = 0x7fffffff;
> + var.g = 0x7fff;
> + var.h = 0x7f;
> + printf("%d %d %d %d %d %d %d %d\n",
> + var.a, var.b, var.c, var.d, var.e, var.f, var.g, var.h);
> +
> + /* sign bit is 1 */
> + var.a = 7;
> + var.b = 0x75;
> + var.c = 0x3555;
> + var.d = 0x755555555;
> + var.e = 0x355555;
> + var.f = 0xf5555555;
> + var.g = 0xf555;
> + var.h = 0xf5;
> + printf("%d %d %d %d %d %d %d %d\n",
> + var.a, var.b, var.c, var.d, var.e, var.f, var.g, var.h);
> +
> + exit(0);
> +}
> diff --git a/test/unittest/bitfields/tst.BasicSigned.r b/test/unittest/bitfields/tst.BasicSigned.r
> new file mode 100644
> index 00000000..074bc249
> --- /dev/null
> +++ b/test/unittest/bitfields/tst.BasicSigned.r
> @@ -0,0 +1,3 @@
> +3 63 8191 17179869183 2097151 2147483647 32767 127
> +-1 -11 -2731 -2863311531 -699051 -178956971 -2731 -11
> +
> diff --git a/test/unittest/bitfields/tst.BasicUnsigned.d b/test/unittest/bitfields/tst.BasicUnsigned.d
> new file mode 100644
> index 00000000..e60d9b1b
> --- /dev/null
> +++ b/test/unittest/bitfields/tst.BasicUnsigned.d
> @@ -0,0 +1,54 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2021, 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: Bit-field store and load work properly.
> + *
> + * SECTION: Structs and Unions/Bit-Fields
> + */
> +
> +#pragma D option quiet
> +
> +struct bitRecord{
> + unsigned int a : 3;
> + unsigned int b : 7;
> + unsigned int c : 14;
> + unsigned long d : 35;
> + unsigned int e : 22;
> + unsigned int f : 32;
> + unsigned int g : 16;
> + unsigned int h : 8;
> +} var;
> +
> +BEGIN
> +{
> + /* sign bit is 0 */
> + var.a = 3;
> + var.b = 0x3f;
> + var.c = 0x1fff;
> + var.d = 0x3ffffffff;
> + var.e = 0x1fffff;
> + var.f = 0x7fffffff;
> + var.g = 0x7fff;
> + var.h = 0x7f;
> + printf("%d %d %d %d %d %d %d %d\n",
> + var.a, var.b, var.c, var.d, var.e, var.f, var.g, var.h);
> +
> + /* sign bit is 1 */
> + var.a = 7;
> + var.b = 0x75;
> + var.c = 0x3555;
> + var.d = 0x755555555;
> + var.e = 0x355555;
> + var.f = 0xf5555555;
> + var.g = 0xf555;
> + var.h = 0xf5;
> + printf("%d %d %d %d %d %d %d %d\n",
> + var.a, var.b, var.c, var.d, var.e, var.f, var.g, var.h);
> +
> + exit(0);
> +}
> diff --git a/test/unittest/bitfields/tst.BasicUnsigned.r b/test/unittest/bitfields/tst.BasicUnsigned.r
> new file mode 100644
> index 00000000..244be657
> --- /dev/null
> +++ b/test/unittest/bitfields/tst.BasicUnsigned.r
> @@ -0,0 +1,3 @@
> +3 63 8191 17179869183 2097151 2147483647 32767 127
> +7 117 13653 31496426837 3495253 4116010325 62805 245
> +
> diff --git a/test/unittest/bitfields/tst.BitFieldPromotion.d b/test/unittest/bitfields/tst.BitFieldPromotion.d
> index 535b8f11..56522216 100644
> --- a/test/unittest/bitfields/tst.BitFieldPromotion.d
> +++ b/test/unittest/bitfields/tst.BitFieldPromotion.d
> @@ -1,10 +1,9 @@
> /*
> * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 2021, 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.
> */
> -/* @@xfail: dtv2 */
>
> /*
> * ASSERTION: Bit-field will be automatically promoted to the next largest
> --
> 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