Bug 214289 – returnEarlyFromInfiniteLoopsForFuzzing and validateDoesGC may fail when used together in the FTL

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

<rdar://problem/65272138>

[Saam Barati]

Created attachment 404199 [details]
patch

[Saam Barati]

Created attachment 404200 [details]
patch

[Keith Miller]

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.

[Saam Barati]

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

[Saam Barati]

Created attachment 404202 [details]
patch for landing

[Saam Barati]

Created attachment 404204 [details]
patch for landing

[EWS]

Committed r264330: <https://trac.webkit.org/changeset/264330>

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

[Michael Catanzaro]

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.

[Saam Barati]

(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”?

[Michael Catanzaro]

(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.

[Saam Barati]

(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).