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

Negative zero broken on ≥ v10.4.0 #25221

Closed
TimothyGu opened this issue Dec 26, 2018 · 11 comments
Closed

Negative zero broken on ≥ v10.4.0 #25221

TimothyGu opened this issue Dec 26, 2018 · 11 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@TimothyGu
Copy link
Member

TimothyGu commented Dec 26, 2018

  • Version: ≥ v10.4.0 < v12.0.0
  • Platform: Linux 4.9.0-8-amd64 SMP Debian 4.9.130-2 (2018-10-27) x86_64 GNU/Linux
  • Subsystem: v8

V8 versions between 6.7 and 7.0 (inclusive) have a bug where

[...[]];
console.log(Object.is(-0, 0));

prints true rather than false. This is reproducible with all Node.js versions after 10.4.0, though it is fixed on master which uses V8 7.1. We should find the V8 commit that fixes this and backport it to LTS at the very least.

/cc @devsnek, who helped triage this bug
/cc @nodejs/v8

@TimothyGu TimothyGu added v8 engine Issues and PRs related to the V8 dependency. v10.x labels Dec 26, 2018
TimothyGu added a commit to engine262/engine262 that referenced this issue Dec 26, 2018
@devsnek
Copy link
Member

devsnek commented Dec 26, 2018

to be clear, the issue is that -0 evaluates to 0, it's not a bug with Object.is or console.log.

@bnoordhuis
Copy link
Member

bnoordhuis commented Dec 27, 2018

If it helps, this bug seems to persist after the function is optimized by TurboFan and therefore it's likely not an Ignition bug.

@ryzokuken
Copy link
Contributor

@TimothyGu this seems to be fixed on v11.5.0 as well:

> $ node -v
v11.5.0

> $ node -e "console.log(Object.is(-0, 0))"
false

@TimothyGu
Copy link
Member Author

@ryzokuken You have to use the spread operator prior to the evaluation of -0 to make the bug surface.

@ryzokuken
Copy link
Contributor

ryzokuken commented Dec 27, 2018

@TimothyGu I'd already tried that:

I see. Will try to investigate.

> $ node 
> [...[]]; console.log(Object.is(-0, 0));
true
undefined

@targos
Copy link
Member

targos commented Dec 28, 2018

This was fixed as a side-effect in v8/v8@1c48d52.

That's a big change that we probably won't be able to backport.

@BridgeAR
Copy link
Member

@targos there do not seem to be a lot of conflicts in that commit when backprorting it to v11.

@nodejs/v8 @psmarshall @bmeurer @hashseed would you be so kind and check what is required to backport the necessary commit?

@ryzokuken
Copy link
Contributor

@BridgeAR I think what @targos means is that since this isn't an isolated fix but a huge change, it won't be great idea to backport the whole commit. Had it been a smaller commit, I guess it would have been simpler.

@mikermcneil

This comment has been minimized.

@BridgeAR

This comment has been minimized.

@targos targos removed the v11.x label Jul 20, 2019
@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

This appears to be fixed in 10.x. Closing

@jasnell jasnell closed this as completed Jun 26, 2020
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

8 participants