UB in various hypot() implementations (left-shifting a negative number)

Sat Nov 16 18:02:29 UTC 2019

The patch for hypotl() is likely unneeded as there is no shifting
of a negative integer in the code being changed.  Does csqrt_test.c
pass without Jeff's patch?

This is the original code

     u_int32_t high;
     t1 = 1.0;
     GET_HIGH_WORD(high,t1);
     SET_HIGH_WORD(t1,high+DESW(k));

high + DESW(k) = high + k
               = 16383 + k

and this is the code after the patch

     t1 = 0.0;
     SET_HIGH_WORD(t1,ESW(k));

ESW(k) = MAX_EXP - 1 + k
       = LDBL_MAX_EXP - 1 + k
       = 16384 - 1 + k
       = 16383 + k

So, in principle there is no functional change.

-- 
steve

On Sat, Nov 16, 2019 at 11:03:19PM +0800, Li-Wen Hsu wrote:
> I did a quick test, it seems one regression test fails:
> 
> lwhsu at x1c:/usr/tests/lib/msun > kyua debug csqrt_test:main
> 1..18
> ok 1 - csqrt
> ok 2 - csqrt
> ok 3 - csqrt
> ok 4 - csqrt
> ok 5 - csqrt
> ok 6 - csqrt
> ok 7 - csqrt
> ok 8 - csqrt
> ok 9 - csqrt
> ok 10 - csqrt
> ok 11 - csqrt
> ok 12 - csqrt
> ok 13 - csqrt
> ok 14 - csqrt
> ok 15 - csqrt
> ok 16 - csqrt
> Assertion failed: (creall(result) == ldexpl(14 * 0x1p-4, exp / 2)),
> function test_overflow, file
> /usr/home/lwhsu/freebsd-src/lib/msun/tests/csqrt_test.c, line 236.
> Process with PID 18700 exited with signal 6 and dumped core;
> attempting to gather stack trace
> [New LWP 101179]
> Core was generated by `/usr/tests/lib/msun/csqrt_test'.
> Program terminated with signal SIGABRT, Aborted.
> #0  thr_kill () at thr_kill.S:3
> 3       RSYSCALL(thr_kill)
> #0  thr_kill () at thr_kill.S:3
> #1  0x00000008004543a4 in __raise (s=6) at /usr/src/lib/libc/gen/raise.c:52
> #2  0x00000008003c3029 in abort () at /usr/src/lib/libc/stdlib/abort.c:67
> #3  0x0000000800440e71 in __assert (func=<optimized out>,
> file=<optimized out>, line=<optimized out>, failedexpr=<optimized
> out>) at /usr/src/lib/libc/gen/assert.c:51
> #4  0x00000000002040bd in test_overflow (maxexp=16384) at
> /usr/home/lwhsu/freebsd-src/lib/msun/tests/csqrt_test.c:236
> #5  0x0000000000202a91 in main () at
> /usr/home/lwhsu/freebsd-src/lib/msun/tests/csqrt_test.c:363
> GDB exited successfully
> Files left in work directory after failure: csqrt_test.core
> csqrt_test:main  ->  broken: Received signal 6
> 
> I haven't checked the details, but I definitely need experts in this
> field to help.
> 
> Thanks,
> Li-Wen
> 
> 
> On Thu, Nov 14, 2019 at 7:22 AM Steve Kargl
> <sgk at troutmask.apl.washington.edu> wrote:
> >
> > Looks good to me.  Just need to convince someone to commit it.
> >
> > --
> > steve
> >
> >
> > On Wed, Nov 13, 2019 at 02:43:04PM -0800, Jeff Walden wrote:
> > >
> > > Just wanted to note here that I filed https://reviews.freebsd.org/D22354 to fix a bit of undefined behavior in the hypot() function implementations.
> > >
> > > `hypot(x, y)` computes `sqrt(x*x + y*y)`, with IEEE-754-aware precision.  For very large or small `x` or `y`, the naive implementation would lose precision -- so in such cases the calculation is performed after multiplying the numbers by a very large (or very small) power of two, then the multiplication is undone at end.
> > >
> > > Undoing the multiplication involves multiplying a quantity `w` by `2**k`, where `k` (which may be positive or negative) was computed depending on the particular `x` and `y` values provided.  Current algorithms generally take `t1=0.0`, extract the high word, add `k<<20` or `k<<23` to it to appropriately bump the exponent, then overwrite `t1`'s high word.  But it seems equally effective to take `t1=0.0`, then write `(1023+k)<<20` or `(127+k)<<23` to it for identical effect -- and `k` is never so negative to compute a negative value.  My changes do this.  (I wish there were named constants I could use for all these numbers, but as there don't seem to be any, magic numbers seems like the best I can do.)
> > >
> > > Errors in these changes would most likely produce a power of two off by a factor of two, so *probably* testing any inputs that would happen to invoke these code paths should be adequate testing.  I'm fixing this in order to upstream a likely fix in the SpiderMonkey JavaScript engine for this, so the (double, double) algorithm/change is the only one I have (purely manually) tested.  Eyeballs on the other functions' changes especially appreciated!
> > >
> > > Jeff
> > > _______________________________________________
> > > freebsd-numerics at freebsd.org mailing list
> > > https://lists.freebsd.org/mailman/listinfo/freebsd-numerics
> > > To unsubscribe, send any mail to "freebsd-numerics-unsubscribe at freebsd.org"
> >
> > --
> > Steve
> > 20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
> > 20161221 https://www.youtube.com/watch?v=IbCHE-hONow
> > _______________________________________________
> > freebsd-numerics at freebsd.org mailing list
> > https://lists.freebsd.org/mailman/listinfo/freebsd-numerics
> > To unsubscribe, send any mail to "freebsd-numerics-unsubscribe at freebsd.org"

-- 
Steve
20170425 https://www.youtube.com/watch?v=VWUpyCsUKR4
20161221 https://www.youtube.com/watch?v=IbCHE-hONow