Issue 14271 in v8: Incorrect bignumber.js behavior with Maglev on x86_64

[kusc… via monorail]

Updates:
Cc: les...@chromium.org

Comment #2 on issue 14271 by kus...@google.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c2

Hey Leszek, sorry to send this out of the blue but a customer sent this over and it seems vaguely V8 and thought you may have a clue who this should be ideally routed to and/or know what the heck is going on

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[lesz… via monorail]

Updates:
Components: Compiler>Maglev
Owner: les...@chromium.org

Comment #3 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c3

Good routing, this should actually end up on my plate. I'll take a look, looks like a maglev correctness issue.

[lesz… via monorail]

Comment #4 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c4

Looks like there's something wrong with the Math.ceil implementation as used in round, if I comment out that reduction I no longer see the issue.

[lesz… via monorail]

Updates:
Labels: -Pri-3 Priority-1
Status: Assigned

Comment #5 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c5

(No comment was entered for this change.)

[lesz… via monorail]

Comment #6 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c6

No, not Math.ceil reduction, it's something to do with there being a call there at all -- if I keep the Math.ceil reduction and add a `print` after it, then I don't see the issue. In fact, it seems to be something to do with the "get number of digits of n" loop:

```
n = k = xc[ni];

// === If I put a print(i) here I don't reproduce

// Get the number of digits of n.
for ( d = 1; k >= 10; k /= 10, d++ );

// === If I put a print(i) here I do

// Get the index of rd within n.
i %= LOG_BASE;
```

[m8ch… via monorail]

Comment #7 on issue 14271 by m8ch...@gmail.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c7

Hello

I've been looking at this issue using v8 binaries from GoogleChromeLabs/jsvu on Windows 10 x64.

The bug is not present in v8 v11.7.251 and below, but is present in v11.7.252 up to at least v11.8.55, the latest available from jsvu.

I'm using the following code appended to bignumber.js.

```
const a = new BigNumber('0.52');
const b = new BigNumber('567.4');

for (let i = 0; i < 500; i++) {
const c = a.toFixed(2);
const d = b.toFixed(2);
if (d !== '567.40') { // should never be true
console.log(d);
break;
}
}
```

Attachments:
test.js 85.1 KB

[m8ch… via monorail]

Comment #8 on issue 14271 by m8ch...@gmail.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c8

Investigating further, the issue seems to be that the value of `n` is being magically transformed from `40000000000000` to `0` as we leave the scope of the `else` block beginning on line 1431.

```
} else {
ni = mathceil((i + 1) / LOG_BASE);

if (ni >= xc.length) {
// branch not taken
} else {

n = k = xc[ni];

// Get the number of digits of n.

for (d = 1; k >= 10; k /= 10, d++);

// Get the index of rd within n.
i %= LOG_BASE;

// Get the index of rd within n, adjusted for leading zeros.
// The number of leading zeros of n is given by LOG_BASE - d.
j = i - LOG_BASE + d;

// Get the rounding digit at index j of n.
rd = j < 0 ? 0 : n / pows10[d - j - 1] % 10 | 0;

//>>>>>>>>>>>>> n is correctly 40000000000000 here
}
//>>>>>>>>>>>>>> n is also correctly 40000000000000 here
}
//>>>>>>>>>>>>>> n is inexplicably 0 here.
```

[evan.… via monorail]

Comment #9 on issue 14271 by evan....@lightspeedhq.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c9

There's also some discussion happening on the bignumber.js GitHub repo, around what small tweaks will and won't cause the issue to be reproduced: https://github.com/MikeMcl/bignumber.js/issues/354

[m8ch… via monorail]

Comment #10 on issue 14271 by m8ch...@gmail.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c10

By the way, the bug still occurs with my test code if the the "get number of digits of n" loop is replaced with

```
d = 14; for (let whatever = 0; whatever; whatever = 0);
```

Absurdly, the latter `whatever = 0` is necessary to trigger the bug even though the condition cannot be true.

The problem seems to be some kind of memory alignment issue perhaps.

[lesz… via monorail]

Updates:
Owner: ol...@chromium.org

Comment #11 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c11

Oli found the issue, it's a register allocation (specifically, stack slot reuse between float64 and int32 values) issue in maglev. We're preparing a fix that we're hoping to backmerge.

[Git Watcher via monorail]

Comment #12 on issue 14271 by Git Watcher: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c12

The following revision refers to this bug:
https://chromium.googlesource.com/v8/v8/+/ae96bf33709239303f2051d894e0c507b24d7290

commit ae96bf33709239303f2051d894e0c507b24d7290
Author: Olivier Flückiger <ol...@chromium.org>
Date: Tue Aug 22 18:22:12 2023

[maglev] Fix interfering move cycles with double spill slots

The ParallelMoveResolver is instantiated separately for Registers and
DoubleRegisters. Thus we cannot detect cycles between stack slots used
for spilling Registers and DoubleRegisters at the same time. To avoid
getting into this situation we don't share spill slots between the two.

Bug: v8:14271
Change-Id: I68bb239081eb83cb94d3fc7840c3e5d12598a596
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4804203
Commit-Queue: Olivier Flückiger <ol...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#89593}

[modify] https://crrev.com/ae96bf33709239303f2051d894e0c507b24d7290/src/maglev/maglev-regalloc.cc
[modify] https://crrev.com/ae96bf33709239303f2051d894e0c507b24d7290/src/maglev/maglev-regalloc.h

[ol… via monorail]

Updates:
Labels: Merge-Request-11.7 Merge-Request-11.6

Comment #13 on issue 14271 by ol...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c13

(No comment was entered for this change.)

[ol… via monorail]

Updates:
Cc: va...@chromium.org

Comment #14 on issue 14271 by ol...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c14

+lutz, can we request merge from here?

[vora… via monorail]

Comment #17 on issue 14271 by vora...@flowaccount.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c17

Hey guys, is there a way to know is this going to be force updated in 11.7? Also how do we check and notify the users in code to help the transition?

[evan.… via monorail]

Comment #18 on issue 14271 by evan....@lightspeedhq.com: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c18

Is there any consideration for disabling the V8Maglev field trial in Chrome until this fix can go out? We've found it to be a significant problem that undermines our customers' confidence. They were also affected back when the trial was enabled for Chrome 115. It took us weeks just to understand what was happening and get to the point where we could reproduce something and file this issue.

We've identified a workaround for now for this particular case in our code base, but it's concerning that such a low-level, hard-to-reason-about issue exists and could affect other code. I guess the fact that it wasn't discovered until now means it may not be too widespread, but I don't understand the fix well enough to say.

[lesz… via monorail]

Comment #19 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c19

Yes there's definitely consideration for this, we're almost certainly going to move the trial up past the fix for this. It does seem to be an issue that's quite hard to hit (which is also what makes it so hard to debug or initially discover), so I don't expect it to be widespread, but nevertheless I can see it causing issues. I'm sorry for the impact on your customers' confidence, I'm going to try to figure out why this took so long to discover to avoid similar issues in the future.

[Git Watcher via monorail]

Updates:
Labels: merge-merged-11.7

Comment #20 on issue 14271 by Git Watcher: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c20

The following revision refers to this bug:

https://chromium.googlesource.com/v8/v8/+/2d8c033b05c199f10067ae9f79f7876b2f059a93

commit 2d8c033b05c199f10067ae9f79f7876b2f059a93

Author: Olivier Flückiger <ol...@chromium.org>
Date: Tue Aug 22 18:22:12 2023

Merged: [maglev] Fix interfering move cycles with double spill slots

The ParallelMoveResolver is instantiated separately for Registers and
DoubleRegisters. Thus we cannot detect cycles between stack slots used
for spilling Registers and DoubleRegisters at the same time. To avoid
getting into this situation we don't share spill slots between the two.

Bug: v8:14271

(cherry picked from commit ae96bf33709239303f2051d894e0c507b24d7290)

Change-Id: I21911c66d8a31bcab4ee4506d6a45e0abef3e53b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4813326

Commit-Queue: Olivier Flückiger <ol...@chromium.org>
Reviewed-by: Leszek Swirski <les...@chromium.org>

Cr-Commit-Position: refs/branch-heads/11.7@{#14}
Cr-Branched-From: fe608691065e23ae83720392bf61a59e8166bee6-refs/heads/11.7.439@{#1}
Cr-Branched-From: aeb45525398afa173aa91fcf83eb6341c83850ea-refs/heads/main@{#89415}

[modify] https://crrev.com/2d8c033b05c199f10067ae9f79f7876b2f059a93/src/maglev/maglev-regalloc.cc
[modify] https://crrev.com/2d8c033b05c199f10067ae9f79f7876b2f059a93/src/maglev/maglev-regalloc.h

[lesz… via monorail]

Updates:
Status: Fixed

Comment #21 on issue 14271 by les...@chromium.org: Incorrect bignumber.js behavior with Maglev on x86_64
https://bugs.chromium.org/p/v8/issues/detail?id=14271#c21

Marking as fixed since it's fixed on main, changing the trial min-version will follow.

[suri g]

Our project uses bignumber.js and issue was reported  on Chrome on 05 Sept 2023(not sure of chrome browser version at that time). Our dev team tried to replicate it but we could not replicate now on our Chrome browsers , we tried multiple versions (Chrome Versions that we tried 116.0.5845.141- 116.0.5845.187 (Official Build) (x86_64) , JavaScript -V8 11.6.189.20) . 

The help that we are looking from the team is 

1) Which chrome browser version had this issue and in which browser this fix has been applied?

2) Was Maglev field Trail enabled for Chrome 116.xx Browser? If so for which version it was enabled/disabled?

2) According to our understanding Maglev is not enabled in (Version 116.0.5845.187 (Official Build) (x86_64)) , How can we enable Maglev Compiler on our browser? we would like to replicate the issue at first and then apply the fix provided by Bignumber team(https://github.com/MikeMcl/bignumber.js/commit/ee5481b1464969fe2f5116abd6cef08144d44509)  and ensure the fix is working . 

Thanks in advance for your help.