Open Bug 1584399 Opened 5 years ago Updated 2 years ago

Add tests that exercise the `scalbn` path in e_pow.cpp, the `copysign` path in `scalbn`, and `__ldexp_exp` in `k_exp.cpp`

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

People

(Reporter: Waldo, Unassigned)

References

(Blocks 1 open bug)

Details

Reviewbot nags in bug 1583958 patches indicate that s_scalbn.cpp, s_copysign.cpp, and k_exp.cpp are all un-test-covered code. The code is live and called in our source code and callable in actual builds. But it looks like we just don't invoke the right things, the right way, in any tests to trigger them.

s_scalbn.cpp contains a scalbn function that is only ever invoked in e_pow.cpp, whose function is invoked by Math.exp(n). I don't know enough numeric analysis and can't quickly grok the code/algorithm/comments enough to figure out what input will hit this code, but it's gotta be a one-liner.

s_copysign.cpp contains a copysign that is only invoked from within scalbn. However, it's invoked in a whole bunch of places -- all commented with "overflow" or "underflow" -- so I expect it's not terribly difficult to figure out what inputs to scalbn would hit any of those cases. (Whether it's easy to get those inputs to be produced by Math.exp(n) is less clear.)

k_exp.cpp contains a __ldexp_exp function that is invoked by fdlibm's sinh/cosh implementations. These calling locations are invoked by Math.sinh(710) and Math.cosh(710), so adding a test that does those would at least give some coverage of k_exp.cpp. Note, however, that the sinh/cosh implementations only use __ldexp_exp for a very specific range of input numbers:

    /* |x| in [22, log(maxdouble)] return half*exp(|x|) */
	if (ix < 0x40862E42)  return half*__ieee754_exp(fabs(x));

    /* |x| in [log(maxdouble), overflowthresold] */
	if (ix<=0x408633CE)
	    return __ldexp_exp(fabs(x), -1);

which seems to mean (Math.log(Number.MAX_VALUE) being 709.782712893384 ) that inputs from 709ish to 720 -- so we're hardly using __ldexp_exp to its full extent and it's at least conceivable there are lurking bugs in parts we don't currently use.

It is definitely good-first-bug territory to add a test ensuring coverage of k_exp.cpp. Someone with some numerical analysis/mathematical chops could probably make relatively short work of the other two -- but it's enough that it is unlikely to be good-first-bug if you don't have those non-programming skills.

Summary: Add a test that exercises the `scalbn` path in e_pow.cpp → Add tests that exercise the `scalbn` path in e_pow.cpp, the `copysign` path in `scalbn`, and `__ldexp_exp` in `k_exp.cpp`
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.