Bug 230915

Summary:

DoesGCCheck does not use enough bits for nodeIndex

Product:

WebKit

Reporter:

Saam Barati <saam>

Component:

JavaScriptCore

Assignee:

Saam Barati <saam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ews-watchlist, keith_miller, mark.lam, mcatanzaro, msaboff, tzagallo, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch.	ews-feeder: commit-queue-
Patch	mark.lam: review+
patch for landing	none
[fast-cq] fix test	none

Description Saam Barati 2021-09-28 14:36:36 PDT

...

Comment 1 Saam Barati 2021-09-28 14:37:06 PDT

<rdar://83297515>

Comment 2 Saam Barati 2021-09-28 15:34:06 PDT

Created attachment 439533 [details]
Patch.

Comment 3 Saam Barati 2021-09-28 15:38:03 PDT

Created attachment 439534 [details]
Patch

Comment 4 Mark Lam 2021-09-28 16:09:59 PDT

Comment on attachment 439534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439534&action=review

r=me with fix.

> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:79
> +    unsigned nodeOp() { return u.other & nodeOpMask; }

This should be:
    unsigned nodeOp() { return (u.other >> nodeOpShift) & nodeOpMask; }

Comment 5 Saam Barati 2021-09-28 17:18:48 PDT

Comment on attachment 439534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439534&action=review

>> Source/JavaScriptCore/dfg/DFGDoesGCCheck.h:79
>> +    unsigned nodeOp() { return u.other & nodeOpMask; }
> 
> This should be:
>     unsigned nodeOp() { return (u.other >> nodeOpShift) & nodeOpMask; }

Will fix. Thanks

Comment 6 Saam Barati 2021-09-28 17:39:36 PDT

Created attachment 439549 [details]
patch for landing

Comment 7 EWS 2021-09-28 18:33:09 PDT

Committed r283207 (242249@main): <https://commits.webkit.org/242249@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439549 [details].

Comment 8 Michael Catanzaro 2021-09-29 06:23:35 PDT

Hi, this new test fails on every architecture (x86_64, aarch64, ppc64le, s390x) with the error:

Running stress/verify-can-gc-node-index.js.default
stress/verify-can-gc-node-index.js.default: Exception: RangeError: Maximum call stack size exceeded.
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:7:11
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: f@verify-can-gc-node-index.js:8:6
stress/verify-can-gc-node-index.js.default: ERROR: Unexpected exit code: 3
FAIL: stress/verify-can-gc-node-index.js.default

So it's doing too much recursion. This is using the JSCOnly port and Red Hat's internal CI, which has cloop enabled, but I see the JSC EWS failed in the same way, and I doubt that EWS is using cloop.

Comment 9 Saam Barati 2021-09-29 09:54:25 PDT

Created attachment 439617 [details]
[fast-cq] fix test

Comment 10 EWS 2021-09-29 09:56:13 PDT

Committed r283231 (242273@main): <https://commits.webkit.org/242273@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439617 [details].