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

Saam Barati

Reported 2021-09-28 14:36:36 PDT

...

Attachments
Patch. (12.13 KB, patch) 2021-09-28 15:34 PDT, Saam Barati	ews-feeder: commit-queue-	Details Formatted Diff Diff
Patch (12.19 KB, patch) 2021-09-28 15:38 PDT, Saam Barati	mark.lam: review+	Details Formatted Diff Diff
patch for landing (12.19 KB, patch) 2021-09-28 17:39 PDT, Saam Barati	no flags	Details Formatted Diff Diff
[fast-cq] fix test (937 bytes, patch) 2021-09-29 09:54 PDT, Saam Barati	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Saam Barati

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

<rdar://83297515>

Saam Barati

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

Created attachment 439533 [details] Patch.

Saam Barati

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

Created attachment 439534 [details] Patch

Mark Lam

Comment 4 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; }

Saam Barati

Comment 5 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

Saam Barati

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

Created attachment 439549 [details] patch for landing

EWS

Comment 7 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].

Michael Catanzaro

Comment 8 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.

Saam Barati

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

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

EWS

Comment 10 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].

Note You need to log in before you can comment on or make changes to this bug.