Summary: | returnEarlyFromInfiniteLoopsForFuzzing and validateDoesGC may fail when used together in the FTL | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Saam Barati <saam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Saam Barati <saam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | benjamin, fpizlo, ggaren, gskachkov, guijemont, jsc32, keith_miller, mark.lam, mcatanzaro, msaboff, rmorisset, ross.kirsling, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=214491 | ||||||||||||
Attachments: |
|
Description
Saam Barati
2020-07-13 17:50:04 PDT
Created attachment 404199 [details]
patch
Created attachment 404200 [details]
patch
Comment on attachment 404200 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=404200&action=review r=me with nit. > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14743 > + jit.store32(CCallHelpers::TrustedImm32(DoesGCCheck::encode(true, DoesGCCheck::Special::Uninitialized)), CCallHelpers::Address(GPRInfo::regT0)); Nit: I think this can be DoesGCCheck(). But maybe I'm missing something. Comment on attachment 404200 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=404200&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:14743 >> + jit.store32(CCallHelpers::TrustedImm32(DoesGCCheck::encode(true, DoesGCCheck::Special::Uninitialized)), CCallHelpers::Address(GPRInfo::regT0)); > > Nit: I think this can be DoesGCCheck(). But maybe I'm missing something. we don't have an accessor to read out the bits as is, that's why I'm calling encode Created attachment 404202 [details]
patch for landing
Created attachment 404204 [details]
patch for landing
Committed r264330: <https://trac.webkit.org/changeset/264330> All reviewed patches have been landed. Closing bug and clearing flags on attachment 404204 [details]. Hi, this causes JSC stress tests to run forever on x86_64 when cloop is enabled. I think we need a different skip condition since --returnEarlyFromInfiniteLoopsForFuzzing is not implemented for cloop, right? Is there a way to skip tests depending on whether JIT is enabled? Concerning: the tests are still passing an aarch64, meaning this test must be skipped despite the skip if $architecture != "arm64". That indicates we probably aren't detecting aarch64 correctly. (In reply to Michael Catanzaro from comment #9) > Hi, this causes JSC stress tests to run forever on x86_64 when cloop is > enabled. I think we need a different skip condition since > --returnEarlyFromInfiniteLoopsForFuzzing is not implemented for cloop, > right? Is there a way to skip tests depending on whether JIT is enabled? > > Concerning: the tests are still passing an aarch64, meaning this test must > be skipped despite the skip if $architecture != "arm64". That indicates we > probably aren't detecting aarch64 correctly. I can fix tomorrow or Monday. We probably should add $vm.isCloop() if it doesn’t exist already, and then return early from the test. Is the ARM64 bot cloop? Does it invoke run-JSC-stress-tests with “—arch arm64”? (In reply to Saam Barati from comment #10) > I can fix tomorrow or Monday. We probably should add $vm.isCloop() if it > doesn’t exist already, and then return early from the test. Thanks. See: bug #214491. I forgot to leave a link to that he re. > Is the ARM64 bot cloop? Yes. (Our internal testers actually only test cloop. The public bots are good enough to keep the JIT working. :) > Does it invoke run-JSC-stress-tests with “—arch arm64”? No, none of our runners use the --arch option. (Didn't know about that option.) It's determined by determineArchitectureFromELFBinary() in run-jsc-stress-tests, which I touched last in r261862. I might have to try to get access to a binary file to see if that function is working properly. (In reply to Michael Catanzaro from comment #11) > (In reply to Saam Barati from comment #10) > > I can fix tomorrow or Monday. We probably should add $vm.isCloop() if it > > doesn’t exist already, and then return early from the test. > > Thanks. See: bug #214491. I forgot to leave a link to that he re. > > > Is the ARM64 bot cloop? > > Yes. (Our internal testers actually only test cloop. The public bots are > good enough to keep the JIT working. :) > > > Does it invoke run-JSC-stress-tests with “—arch arm64”? > > No, none of our runners use the --arch option. (Didn't know about that > option.) It's determined by determineArchitectureFromELFBinary() in > run-jsc-stress-tests, which I touched last in r261862. I might have to try > to get access to a binary file to see if that function is working properly. Yeah, if you're not passing "--arch", it's pretty likely that's the issue. I've ensured many times that these checks work in the tests themselves (because I've added many of them). |