Bug 193938 – We should add code to validate expected GC activity modelled by doesGC() against what the runtime encounters.

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

[Mark Lam]

For example, in a debug build (or maybe a ASAN build, when we expect everything to be slow), we should try to do the following:

1. JIT compile time verification

   Have a list of DFG operations that are expected to not have GC activity.
   At compile time, maybe in DFG's SpeculativeJIT or FTL LowerTOB3, assert that the expected GC activity modeled by doesGC() matches the operations we're going to call.  This is 

   The reliability of this verification relies on us declaring correctly if a DFG operation is expected to not GC.

2. Runtime verification

   For any operations that are not expected to GC:
   a. Before the operation call, set a flag in the VM.
   b. Modify the GC to clear that flag.
   c. After the operation call, check the flag to make sure that it is still set.
      If the flag is not set, report the issue and crash.
      Else, clear the flag.

   The reliability of this verification relies on us having sufficient code coverage in our test suite to be able to trigger GCs if they are possible in the relevant code paths we are asserting.

[Mark Lam]

z

[Radar WebKit Bug Importer]

<rdar://problem/47616277>

[Mark Lam]

There's an easier way to do this.  In DFG::SpeculativeJIT::compile() and FTL::LowerDFGToB3::compileNode(), before emitting code / B3IR for each DFG node, I can emit a write to a VM flag to declare whether doesGC() expects GCs to be possible or not.  And in the runtime, in allocateCell(), and functions that can resolve rope, we assert that that flag agrees that GC is possible.

[Mark Lam]

Created attachment 362432 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 362432 [details]
proposed patch.

Attachment 362432 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11210307

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 362457 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

[Robin Morisset]

Comment on attachment 362432 [details]
proposed patch.

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

LGTM

> Source/JavaScriptCore/ChangeLog:22
> +        To ensure that Heap.h is #include'd for all files that needs to so this validation

typo: "to so this validation" => "to do this validation"

> Source/JavaScriptCore/heap/Heap.h:308
> +#else

Would it be possible to put RELEASE_ASSERT_NOT_REACHED in these three functions in the else case? It looks like an error to ever call them when DFG_DOES_GC_VALIDATION is 0.

[Michael Saboff]

Comment on attachment 362432 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/runtime/JSString.h:575
> +    if (allocationAllowed) {
> +        if (validateDFGDoesGC)
> +            RELEASE_ASSERT(vm()->heap.expectDoesGC());

Shouldn't these checks and assert be inside the isRope() then code?  If this string isn't a rope, we don't care about these checks.

[Mark Lam]

Comment on attachment 362432 [details]
proposed patch.

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

Thanks for the reviews.

>> Source/JavaScriptCore/ChangeLog:22
>> +        To ensure that Heap.h is #include'd for all files that needs to so this validation
> 
> typo: "to so this validation" => "to do this validation"

Fixed.

>> Source/JavaScriptCore/heap/Heap.h:308
>> +#else
> 
> Would it be possible to put RELEASE_ASSERT_NOT_REACHED in these three functions in the else case? It looks like an error to ever call them when DFG_DOES_GC_VALIDATION is 0.

Good point.  They should all be guarded with a validateDFGDoesGC check, and we don't actually want them to be called.  I'll make this change.

>> Source/JavaScriptCore/runtime/JSString.h:575
>> +            RELEASE_ASSERT(vm()->heap.expectDoesGC());
> 
> Shouldn't these checks and assert be inside the isRope() then code?  If this string isn't a rope, we don't care about these checks.

No, actually, we want this here because even if we don't have a test that exercises this code path with a rope, we want to detect that a GC is possible here because a rope resolution is possible.

[Mark Lam]

(In reply to Mark Lam from comment #9)
> >> Source/JavaScriptCore/heap/Heap.h:308
> >> +#else
> > 
> > Would it be possible to put RELEASE_ASSERT_NOT_REACHED in these three functions in the else case? It looks like an error to ever call them when DFG_DOES_GC_VALIDATION is 0.
> 
> Good point.  They should all be guarded with a validateDFGDoesGC check, and
> we don't actually want them to be called.  I'll make this change.

This turns out to be painful because the compiler is not happy that the "code will never be executed" (due to the RELEASE_ASSERT_NOT_REACHED()).  I don't think it is worth making the code more convoluted in order to accommodate
this assertion.  I'll just leave it as it was before.

[Keith Miller]

(In reply to Mark Lam from comment #10)
> (In reply to Mark Lam from comment #9)
> > >> Source/JavaScriptCore/heap/Heap.h:308
> > >> +#else
> > > 
> > > Would it be possible to put RELEASE_ASSERT_NOT_REACHED in these three functions in the else case? It looks like an error to ever call them when DFG_DOES_GC_VALIDATION is 0.
> > 
> > Good point.  They should all be guarded with a validateDFGDoesGC check, and
> > we don't actually want them to be called.  I'll make this change.
> 
> This turns out to be painful because the compiler is not happy that the
> "code will never be executed" (due to the RELEASE_ASSERT_NOT_REACHED()).  I
> don't think it is worth making the code more convoluted in order to
> accommodate
> this assertion.  I'll just leave it as it was before.

You can use UNREACHABLE_FOR_PLATFORM that will trick the compiler into thinking the crashing code may not be reachable.

[Saam Barati]

Comment on attachment 362432 [details]
proposed patch.

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

r=me too

> Source/JavaScriptCore/ChangeLog:11
> +        to the value returned by doesGC() for that node.  In the runtime (i.e. in allocateCell()

As you mentioned in person, it's worth adding this to at least DeferGC. Perhaps other places too. That could be a good follow-up to do.

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:250
> +        jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC());

Why not just do this in the OSRExit common code?

> Source/JavaScriptCore/runtime/JSString.h:571
> +inline const String& JSString::tryGetValue(bool allocationAllowed) const

This seems like a different patch. Why is this change part of this patch?

[Mark Lam]

Comment on attachment 362432 [details]
proposed patch.

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

>> Source/JavaScriptCore/ChangeLog:11
>> +        to the value returned by doesGC() for that node.  In the runtime (i.e. in allocateCell()
> 
> As you mentioned in person, it's worth adding this to at least DeferGC. Perhaps other places too. That could be a good follow-up to do.

Will do that in a follow up.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:250
>> +        jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC());
> 
> Why not just do this in the OSRExit common code?

I'm trying to do this at a place where all registers are free to use.  I don't want to make any assumptions about the tempRegister, dataTempRegister, or memoryTempRegister.

>> Source/JavaScriptCore/runtime/JSString.h:571
>> +inline const String& JSString::tryGetValue(bool allocationAllowed) const
> 
> This seems like a different patch. Why is this change part of this patch?

This is needed because the assertion below would fail in some of our JSC tests if I didn't have this.  Specifically, the JIT::emit_compareAndJump() above will tryGetValue() on a string that is expected to never be a rope, but our assertion will not know that if we don't thread that info down here.

[Mark Lam]

Comment on attachment 362432 [details]
proposed patch.

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

>>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:250
>>> +        jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC());
>> 
>> Why not just do this in the OSRExit common code?
> 
> I'm trying to do this at a place where all registers are free to use.  I don't want to make any assumptions about the tempRegister, dataTempRegister, or memoryTempRegister.

My thinking is wrong.  I forgot that in OSR exit we don't need to restore any register state that would be trashed by this.  I'll look into moving this into OSRExit common code.

[Mark Lam]

Comment on attachment 362432 [details]
proposed patch.

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

>>>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:250
>>>> +        jit.store8(CCallHelpers::TrustedImm32(true), vm->heap.addressOfExpectDoesGC());
>>> 
>>> Why not just do this in the OSRExit common code?
>> 
>> I'm trying to do this at a place where all registers are free to use.  I don't want to make any assumptions about the tempRegister, dataTempRegister, or memoryTempRegister.
> 
> My thinking is wrong.  I forgot that in OSR exit we don't need to restore any register state that would be trashed by this.  I'll look into moving this into OSRExit common code.

Correction: I can't wait till the common code to set m_expectDoesGC because it needs to be set before we do object materialization for both the DFG and FTL, which happens before the common parts.  The DFG does materialization of the arguments array.

[Mark Lam]

Created attachment 362559 [details]
patch for landing.

[Mark Lam]

Landed in r241849: <http://trac.webkit.org/r241849>.