Bug 214491 - REGRESSION(r264330): infinite loop in JSC stress tests using cloop
Summary: REGRESSION(r264330): infinite loop in JSC stress tests using cloop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-07-17 12:00 PDT by Michael Catanzaro
Modified: 2020-07-21 11:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2020-07-17 12:44 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (1.42 KB, patch)
2020-07-21 06:14 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2020-07-17 12:00:18 PDT
r264330 added an infinite loop that was supposed to be broken by the --returnEarlyFromInfiniteLoopsForFuzzing=true flag. However, it doesn't work when the tests are run with cloop. This has caused JSC stress tests to run forever only on x86_64 (with cloop enabled). Instead of skipping the tests on particular architectures, let's run the loops only if $vm.useJIT(), suggested by Yusuke.

I think there is a problem with architecture detection for arm64, because the tests are still running fine on this architecture.
Comment 1 Michael Catanzaro 2020-07-17 12:16:06 PDT
(In reply to Michael Catanzaro from comment #0)
> I think there is a problem with architecture detection for arm64, because
> the tests are still running fine on this architecture.

Seems I touched it last, in r261862. My changes look correct, though....
Comment 2 Michael Catanzaro 2020-07-17 12:44:56 PDT
Created attachment 404582 [details]
Patch
Comment 3 Michael Catanzaro 2020-07-18 12:17:13 PDT
Looks like I broke the test on jsc-armv7. No clue why, unless maybe this feature depends on a higher JIT tier than the baseline JIT...? I'm not sure what else could be going wrong.

The solution would be to not remove the skip line, so if nobody else has a guess, that's what I'll do.
Comment 4 Yusuke Suzuki 2020-07-18 17:10:29 PDT
Comment on attachment 404582 [details]
Patch

r=me
Comment 5 Michael Catanzaro 2020-07-19 07:46:18 PDT
(In reply to Michael Catanzaro from comment #3)
> Looks like I broke the test on jsc-armv7. No clue why, unless maybe this
> feature depends on a higher JIT tier than the baseline JIT...? I'm not sure
> what else could be going wrong.
> 
> The solution would be to not remove the skip line, so if nobody else has a
> guess, that's what I'll do.

Yusuke, are you OK with that "solution?" (The current version of the patch breaks jsc-armv7, and that would avoid doing so... but it doesn't seem great.)
Comment 6 Saam Barati 2020-07-20 12:43:24 PDT
Comment on attachment 404582 [details]
Patch

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

> JSTests/stress/validate-does-gc-with-return-early-from-infinite-loop.js:3
> +if ($vm.useJIT()) {

this looks wrong in combination of removing the above skip. I didn't implement it on 32-bit.
Comment 7 Michael Catanzaro 2020-07-20 13:04:01 PDT
So to confirm, this test should run only (a) on 64-bit platforms, and (b) only if JIT is enabled. Yes? In that case, my patch would be fine if I leave the Skip line unchanged, right?
Comment 8 Saam Barati 2020-07-20 21:32:38 PDT
(In reply to Michael Catanzaro from comment #7)
> So to confirm, this test should run only (a) on 64-bit platforms, and (b)
> only if JIT is enabled. Yes? In that case, my patch would be fine if I leave
> the Skip line unchanged, right?

Yes, I believe that is the way to go.
Comment 9 Michael Catanzaro 2020-07-21 06:14:21 PDT
Created attachment 404814 [details]
Patch
Comment 10 EWS 2020-07-21 11:54:30 PDT
Committed r264668: <https://trac.webkit.org/changeset/264668>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404814 [details].
Comment 11 Radar WebKit Bug Importer 2020-07-21 11:55:17 PDT
<rdar://problem/65895487>