[DTrace-devel] [PATCH] Signed divide and mod are broken
Eugene Loh
eugene.loh at oracle.com
Sat Aug 22 13:22:44 PDT 2020
I meant to mark this as V2. Anyhow, thanks for the feedback. I was
reluctant to use branches due to the complexity, but I agree the code is
more efficient and, most importantly, lighter on register pressure.
On 08/22/2020 01:20 PM, eugene.loh at oracle.com wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> Consider
> trace(-2700 / 1000);
> trace(-2700 % 1000);
>
> versus
> x = -2700;
> y = 1000;
> trace(x / y);
> trace(x % y);
>
> The first correctly gives -2 and -700, while the second incorrectly
> gives -1700 and -1700.
>
> The problem is that we pass obsolete DIF_OP_SDIV and DIF_OP_SREM op
> codes, whose numeric values translate to BPF_ADD operations. There
> are no signed DIV or MOD operations in BPF.
>
> Fix by handling the signs ourselves. Also, clean up tests so that
> such a problem would be discovered.
>
> Orabug: 31783034
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 74 +++++++++++++++++++++++----
> test/unittest/arithmetic/tst.basics.d | 4 +-
> test/unittest/arithmetic/tst.basics.r | 8 +++
> test/unittest/arithmetic/tst.moddiv.d | 26 ++++++++++
> test/unittest/arithmetic/tst.moddiv.r | 1 +
> 5 files changed, 100 insertions(+), 13 deletions(-)
> create mode 100644 test/unittest/arithmetic/tst.basics.r
> create mode 100644 test/unittest/arithmetic/tst.moddiv.d
> create mode 100644 test/unittest/arithmetic/tst.moddiv.r
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index d5532c55..33cff303 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1622,9 +1622,67 @@ dt_cg_arithmetic_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp,
> if (is_ptr_op && lp_is_ptr)
> dt_cg_ptrsize(dnp, dlp, drp, BPF_MUL, dnp->dn_right->dn_reg);
>
> - instr = BPF_ALU64_REG(op, dnp->dn_left->dn_reg, dnp->dn_right->dn_reg);
> + if ((op == BPF_MOD || op == BPF_DIV) &&
> + (dnp->dn_flags & DT_NF_SIGNED)) {
> +
> + /*
> + * BPF_DIV and BPF_MOD are only for unsigned arguments.
> + * For signed arguments, use the following algorithm.
> + * The four right columns indicate the flow of steps.
> + *
> + * l > 0 l > 0 l < 0 l < 0
> + * r > 0 r < 0 r > 0 r < 0
> + * jslt %rl,0,L3 1 1 1 1
> + * jslt %rr,0,L1 2 2
> + * ja L4 3
> + * L1: neg %rr 3
> + * L2: op %rl,%rr 4 4
> + * neg %rl 5 5
> + * ja L5 6 6
> + * L3: neg %rl 2 2
> + * jsge %rr,0,L2 3 3
> + * neg %rr 4
> + * L4: div %rl,%rr 4 5
> + * L5: nop
> + */
> + uint_t lbl_L1 = dt_irlist_label(dlp);
> + uint_t lbl_L2 = dt_irlist_label(dlp);
> + uint_t lbl_L3 = dt_irlist_label(dlp);
> + uint_t lbl_L4 = dt_irlist_label(dlp);
> + uint_t lbl_L5 = dt_irlist_label(dlp);
> +
> + instr = BPF_BRANCH_IMM(BPF_JSLT, dnp->dn_left->dn_reg, 0, lbl_L3);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_BRANCH_IMM(BPF_JSLT, dnp->dn_right->dn_reg, 0, lbl_L1);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_JUMP(lbl_L4);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_NEG_REG(dnp->dn_right->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(lbl_L1, instr));
> + instr = BPF_ALU64_REG(op, dnp->dn_left->dn_reg,
> + dnp->dn_right->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(lbl_L2, instr));
> + instr = BPF_NEG_REG(dnp->dn_left->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_JUMP(lbl_L5);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_NEG_REG(dnp->dn_left->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(lbl_L3, instr));
> + instr = BPF_BRANCH_IMM(BPF_JSGE, dnp->dn_right->dn_reg, 0, lbl_L2);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_NEG_REG(dnp->dn_right->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + instr = BPF_ALU64_REG(op, dnp->dn_left->dn_reg,
> + dnp->dn_right->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(lbl_L4, instr));
> + instr = BPF_NOP();
> + dt_irlist_append(dlp, dt_cg_node_alloc(lbl_L5, instr));
> + } else {
> + instr = BPF_ALU64_REG(op, dnp->dn_left->dn_reg,
> + dnp->dn_right->dn_reg);
> + dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> + }
>
> - dt_irlist_append(dlp, dt_cg_node_alloc(DT_LBL_NONE, instr));
> dt_regset_free(drp, dnp->dn_right->dn_reg);
> dnp->dn_reg = dnp->dn_left->dn_reg;
>
> @@ -2380,14 +2438,12 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> break;
>
> case DT_TOK_DIV_EQ:
> - dt_cg_arithmetic_op(dnp, dlp, drp,
> - (dnp->dn_flags & DT_NF_SIGNED) ? DIF_OP_SDIV : BPF_DIV);
> + dt_cg_arithmetic_op(dnp, dlp, drp, BPF_DIV);
> dt_cg_asgn_op(dnp, dlp, drp);
> break;
>
> case DT_TOK_MOD_EQ:
> - dt_cg_arithmetic_op(dnp, dlp, drp,
> - (dnp->dn_flags & DT_NF_SIGNED) ? DIF_OP_SREM : BPF_MOD);
> + dt_cg_arithmetic_op(dnp, dlp, drp, BPF_MOD);
> dt_cg_asgn_op(dnp, dlp, drp);
> break;
>
> @@ -2495,13 +2551,11 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> break;
>
> case DT_TOK_DIV:
> - dt_cg_arithmetic_op(dnp, dlp, drp,
> - (dnp->dn_flags & DT_NF_SIGNED) ? DIF_OP_SDIV : BPF_DIV);
> + dt_cg_arithmetic_op(dnp, dlp, drp, BPF_DIV);
> break;
>
> case DT_TOK_MOD:
> - dt_cg_arithmetic_op(dnp, dlp, drp,
> - (dnp->dn_flags & DT_NF_SIGNED) ? DIF_OP_SREM : BPF_MOD);
> + dt_cg_arithmetic_op(dnp, dlp, drp, BPF_MOD);
> break;
>
> case DT_TOK_LNEG:
> diff --git a/test/unittest/arithmetic/tst.basics.d b/test/unittest/arithmetic/tst.basics.d
> index dceadf48..300a0079 100644
> --- a/test/unittest/arithmetic/tst.basics.d
> +++ b/test/unittest/arithmetic/tst.basics.d
> @@ -5,13 +5,11 @@
> * http://oss.oracle.com/licenses/upl.
> */
>
> -/* @@note: wild negative numbers, tst.basics.d.out does not exist: validate */
> -
> /*
> * ASSERTION:
> * Simple Arithmetic expressions.
> * Call simple expressions and make sure test succeeds.
> - * Match expected output in tst.basics.d.out
> + * Match expected output in tst.basics.r
> *
> * SECTION: Types, Operators, and Expressions/Arithmetic Operators
> *
> diff --git a/test/unittest/arithmetic/tst.basics.r b/test/unittest/arithmetic/tst.basics.r
> new file mode 100644
> index 00000000..d3b6af81
> --- /dev/null
> +++ b/test/unittest/arithmetic/tst.basics.r
> @@ -0,0 +1,8 @@
> +The value of i is 6
> +The value of i is 18
> +The value of i is 72
> +The value of i is 25920
> +The value of i is 935761216
> +The value of i is -91738734
> +The value of i is -91738729
> +
> diff --git a/test/unittest/arithmetic/tst.moddiv.d b/test/unittest/arithmetic/tst.moddiv.d
> new file mode 100644
> index 00000000..d3af21d0
> --- /dev/null
> +++ b/test/unittest/arithmetic/tst.moddiv.d
> @@ -0,0 +1,26 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2020, 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:
> + * Simple mod and div expressions should work correctly,
> + * even for signed operations.
> + *
> + * SECTION: Types, Operators, and Expressions/Arithmetic Operators
> + *
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN {
> + x = 19; y = 5;
> + trace(( x) / ( y)); trace (( x) % ( y));
> + trace(( x) / (-y)); trace (( x) % (-y));
> + trace((-x) / ( y)); trace ((-x) % ( y));
> + trace((-x) / (-y)); trace ((-x) % (-y));
> + exit(0)
> +}
> diff --git a/test/unittest/arithmetic/tst.moddiv.r b/test/unittest/arithmetic/tst.moddiv.r
> new file mode 100644
> index 00000000..3fc9bc14
> --- /dev/null
> +++ b/test/unittest/arithmetic/tst.moddiv.r
> @@ -0,0 +1 @@
> +34-3-4-3-434
More information about the DTrace-devel
mailing list