Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Math.atan() behavior change #56796

Open
anthonyroach opened this issue Jan 27, 2025 · 9 comments
Open

Math.atan() behavior change #56796

anthonyroach opened this issue Jan 27, 2025 · 9 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@anthonyroach
Copy link

anthonyroach commented Jan 27, 2025

Version

v20.18.1

Platform

Darwin foo 24.2.0 Darwin Kernel Version 24.2.0: Fri Dec  6 19:01:59 PST 2024; root:xnu-11215.61.5~2/RELEASE_ARM64_T6000 arm64

Subsystem

Math

What steps will reproduce the bug?

Math.atan(2.859624123917431)

How often does it reproduce? Is there a required condition?

It happens all the time on Mac OS X.

What is the expected behavior? Why is that the expected behavior?

1.2343920821908787

This is what older versions of node.js returned, and what all versions of node.js on Linux x86 return.

What do you see instead?

1.234392082190879

Additional information

1.234392082190879 is actually the more correct result and agrees with Python and Wolfram Alpha. But the concerning thing is that the result changed between v20.18.0 and v20.18.1 on Mac OS X arm64 only. On Linux x86 all versions return the less correct result of 1.2343920821908787. It would be nice if node.js on all platforms returned the same value. The difference is small but it is causing some headaches in our unit tests between platforms and node versions.

Here's is what Wolfram Alpha shows with more significant digits:

1.23439208219087881390254524370678138808146132211060418165008615225322232

@juanarbol juanarbol added the v8 engine Issues and PRs related to the V8 dependency. label Jan 28, 2025
@juanarbol
Copy link
Member

juanarbol commented Jan 28, 2025

Hi, thanks for the report. From our side there's not much we can do as Math global is managed by V8. We may have to backport or revert something in our V8 folder.

cc @nodejs/v8

@anthonyroach
Copy link
Author

One interesting thing is node -p process.versions.v8 prints 11.3.244.8-node.23 on both node 20.18.0 and 20.18.1. So both seem to be using the same v8 version.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 28, 2025

This change in behavior is reproducing for me in the same way as #56762 (more specifically #56762 (comment), but now I'm not questioning #55056).

@juanarbol
Copy link
Member

juanarbol commented Jan 28, 2025

Should we also include #56737 ?

I ran a bisect, It did not worked. But I found something, could it be a "build-server" issue (dependency)?

$ node -v
v20.17.0

$ ./node -v
v20.17.0

$ node -e 'console.log(Math.atan(2.859624123917431))'
1.2343920821908787

$ ./node -e 'console.log(Math.atan(2.859624123917431))'
1.234392082190879

@targos
Copy link
Member

targos commented Jan 28, 2025

could it be a "build-server" issue (dependency)?

I think this corresponds to the update from macOS 11 to macOS 13 in the release infra.

@bnoordhuis
Copy link
Member

I'm reasonably sure I encountered a similar bug in quickjs-ng, see quickjs-ng/quickjs#268 (comment).

V8 has its own implementation of atan in deps/v8/src/base/ieee754.cc, it doesn't use libm's. The fix in our case was to either make the doubles volatile, or compile with -mno-fma to disable the fused multiply-adds that affected the precision of intermediate results.

@subfuzion
Copy link

I'm reasonably sure I encountered a similar bug in quickjs-ng, see quickjs-ng/quickjs#268 (comment).

FWIW, the QuickJS Date.UTC test failing for node-test is how I noticed the problem I reported in #56762.

@srl295
Copy link
Member

srl295 commented Jan 29, 2025

Should we also include #56737 ?

How is that issue related?

@juanarbol
Copy link
Member

How is that issue related?

My bad, I don't think they are related at all. It is just this idea of "wrong format, then ICU problem". Sorry, I'll try to be a bit more careful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

7 participants