Bug 9678 – fmod function of MS CRT returns NaN for x / Inf

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Björn Graf (boki)]

JavaScriptCore expects fmod to return Inf for finite dividend and infinite divisor , but the MS CRT version of fmod returns NaN in these cases.

[Björn Graf (boki)]

Created attachment 9128 [details]
MS CRT fmod bug workaround where x % (1/0) = NaN, not x.

The workaround checks the arguments for infinity and only calls fmod when the divisor is not infinite. Otherwise it returns the dividend.

[Darin Adler]

Comment on attachment 9128 [details]
MS CRT fmod bug workaround where x % (1/0) = NaN, not x.

I'd like to see this workaround in MathExtras.h rather than inline in the JavaScript-specific files. We just need a name for the fmod with the workaround, and then we can use that. It can be an inline that expands to a call to fmod on platforms other than Win32.

Also, we do COMPILER(MSVC) rather than _MSC_VER for these kinds of ifdefs.

I don't think we need PLATFORM(WIN) at all -- probably COMPILER(MSVC) is the whole check needed.

[Björn Graf (boki)]

Created attachment 9183 [details]
Workaround for MSVCRTs fmod function

This patch implements the necessary workaround in wtf/MathExtras.h.

Defining fmod inside the KJS namespace would have worked, if all call sites where in the namespace explicitly. However, being implicitly declared in the namespace and defined in it via the using namespace KJS; directive requires the #define workaround.

Needed to change isinf() because NaN is not finite.

[Maciej Stachowiak]

I think Darin's suggestion was to just use an alternate function name besides fmod at the call sites, and for non-MSVC compilers it could just expand inline to fmod. It could just be called _fmod, or fremainder or something. I actually think this solution is fine though, so I'll just r+ it.

[Darin Adler]

(In reply to comment #4)
> I think Darin's suggestion was to just use an alternate function name besides
> fmod at the call sites, and for non-MSVC compilers it could just expand inline
> to fmod. It could just be called _fmod, or fremainder or something. I actually
> think this solution is fine though, so I'll just r+ it.

Maciej's right about what I was suggesting. But I think it's also OK to use a trick so it can still be fmod at the call site. I would not have called the other function _fmod, though, because those identifiers are specifically reserved for the implementation! Maybe fmodWithWorkaround or something like that.

Also, if we are doing a macro, a macro that takes parameters is safer than one that just does the bare name. But I also think this is OK as-is, so I'll just leave Maciej's review flag intact. We can always enhance the technique used in MathExtras.h when and if we run into trouble.

[Darin Adler]

Created attachment 9193 [details]
my alternate version of the fmod patch

[Darin Adler]

Created attachment 9198 [details]
my alternate version of the fmod patch

[Maciej Stachowiak]

Comment on attachment 9198 [details]
my alternate version of the fmod patch

This looks ok too, but I kinda like MathExtras.h being transparent and letting you use the normal function names. Saying wtf_fmod all over (w/ potential to mistakenly say fmod) seems worse to me than a macro solution. Agreed though that a macro with parameters would be better.

[Maciej Stachowiak]

Comment on attachment 9198 [details]
my alternate version of the fmod patch

This looks ok too, but I kinda like MathExtras.h being transparent and letting you use the normal function names. Saying wtf_fmod all over (w/ potential to mistakenly say fmod) seems worse to me than a macro solution. Agreed though that a macro with parameters would be better.

[Darin Adler]

Committed revision 15155.