Bug 230915 - DoesGCCheck does not use enough bits for nodeIndex
Summary: DoesGCCheck does not use enough bits for nodeIndex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-28 14:36 PDT by Saam Barati
Modified: 2021-09-29 09:56 PDT (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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].