Re: [DTrace-devel] [PATCH 5/8] Fix stddev() carryover computation

Kris Van Hees
2024-08-19T23:13:29Z

On Tue, Jun 04, 2024 at 02:00:05PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@oracle.com>
>
> The stddev() aggregation function squares 64-bit data values. A
> value is split into 32-bit high and low values. Then, (high + low)
> is squared to produce high*high, 2*high*low, and low*low. Each is
> managed in its own 64-bit register, with the final result residing
> in two 64-bit registers.
>
> When the 2*high*low portion is combined with the low*low portion,
> care is exercised in case the combination has a carryover portion to
> the higher bits. This check was broken in the case where low==0.
> That is, data values whose lowest 32 bits were 0 resulted in
> outrageously bad stddev() results.
>
> Fix the check and add a test for such cases.
>
> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@oracle.com>

> ---
> libdtrace/dt_cg.c | 2 +-
> test/unittest/aggs/tst.stddev2.d | 45 ++++++++++++++++++++++++++++++++
> test/unittest/aggs/tst.stddev2.r | 13 +++++++++
> 3 files changed, 59 insertions(+), 1 deletion(-)
> create mode 100644 test/unittest/aggs/tst.stddev2.d
> create mode 100644 test/unittest/aggs/tst.stddev2.r
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index a1c24e37..fc2cebf0 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -8290,7 +8290,7 @@ dt_cg_agg_stddev(dt_pcb_t *pcb, dt_ident_t *aid, dt_node_t *dnp,
> /* Add low value part from mid to lowreg */
> emit(dlp, BPF_ALU64_REG(BPF_ADD, lowreg, lmdreg));
> /* Handle the overflow/carry case */
> - emit(dlp, BPF_BRANCH_REG(BPF_JLT, lmdreg, lowreg, Lncy));
> + emit(dlp, BPF_BRANCH_REG(BPF_JLE, lmdreg, lowreg, Lncy));
> emit(dlp, BPF_ALU64_IMM(BPF_ADD, hi_reg, 1)) /* account for carry */;
>
> /* Sum high value; no overflow expected nor accounted for */
> diff --git a/test/unittest/aggs/tst.stddev2.d b/test/unittest/aggs/tst.stddev2.d
> new file mode 100644
> index 00000000..994bc3e2
> --- /dev/null
> +++ b/test/unittest/aggs/tst.stddev2.d
> @@ -0,0 +1,45 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2024, 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: Positive stddev() test
> + *
> + * SECTION: Aggregations/Aggregations
> + *
> + * NOTES: This is a simple verifiable positive test of the stddev() function.
> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> + @a = stddev( 0); @a = stddev( 0);
> + @b = stddev( 0x10); @b = stddev( 0x20);
> + @c = stddev( 0x100); @c = stddev( 0x200);
> + @d = stddev( 0x1000); @d = stddev( 0x2000);
> + @e = stddev( 0x10000); @e = stddev( 0x20000);
> + @f = stddev( 0x100000); @f = stddev( 0x200000);
> + @g = stddev( 0x1000000); @g = stddev( 0x2000000);
> + @h = stddev( 0x10000000); @h = stddev( 0x20000000);
> + @i = stddev( 0x20000000); @i = stddev( 0x40000000);
> + @j = stddev( 0x40000000); @j = stddev( 0x80000000);
> + @k = stddev( 0x80000000); @k = stddev(0x100000000);
> + @l = stddev(0x100000000); @l = stddev(0x200000000);
> + printa("%9@x\n", @a);
> + printa("%9@x\n", @b);
> + printa("%9@x\n", @c);
> + printa("%9@x\n", @d);
> + printa("%9@x\n", @e);
> + printa("%9@x\n", @f);
> + printa("%9@x\n", @g);
> + printa("%9@x\n", @h);
> + printa("%9@x\n", @i);
> + printa("%9@x\n", @j);
> + printa("%9@x\n", @k);
> + printa("%9@x\n", @l);
> + exit(0);
> +}
> diff --git a/test/unittest/aggs/tst.stddev2.r b/test/unittest/aggs/tst.stddev2.r
> new file mode 100644
> index 00000000..16e17736
> --- /dev/null
> +++ b/test/unittest/aggs/tst.stddev2.r
> @@ -0,0 +1,13 @@
> +
> + 0
> + 8
> + 80
> + 800
> + 8000
> + 80000
> + 800000
> + 8000000
> + 10000000
> + 20000000
> + 40000000
> + 80000000
> --
> 2.18.4
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel