Bug 193938 - We should add code to validate expected GC activity modelled by doesGC() against what the runtime encounters.
Summary: We should add code to validate expected GC activity modelled by doesGC() agai...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-28 16:35 PST by Mark Lam
Modified: 2019-02-20 16:11 PST (History)
10 users (show)

See Also:


Attachments
proposed patch. (14.77 KB, patch)
2019-02-19 14:39 PST, Mark Lam
msaboff: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.87 MB, application/zip)
2019-02-19 17:21 PST, EWS Watchlist
no flags Details
patch for landing. (17.21 KB, patch)
2019-02-20 15:59 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-01-28 16:35:28 PST
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.
Comment 1 Mark Lam 2019-01-28 16:36:10 PST
z
Comment 2 Radar WebKit Bug Importer 2019-01-28 16:36:26 PST
<rdar://problem/47616277>
Comment 3 Mark Lam 2019-02-19 14:10:35 PST
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.
Comment 4 Mark Lam 2019-02-19 14:39:01 PST
Created attachment 362432 [details]
proposed patch.
Comment 5 EWS Watchlist 2019-02-19 17:21:35 PST
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
Comment 6 EWS Watchlist 2019-02-19 17:21:47 PST
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
Comment 7 Robin Morisset 2019-02-20 10:18:34 PST
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.
Comment 8 Michael Saboff 2019-02-20 10:32:02 PST
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.
Comment 9 Mark Lam 2019-02-20 11:08:22 PST
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.
Comment 10 Mark Lam 2019-02-20 11:21:55 PST
(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.
Comment 11 Keith Miller 2019-02-20 11:24:36 PST
(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.
Comment 12 Saam Barati 2019-02-20 13:53:45 PST
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?
Comment 13 Mark Lam 2019-02-20 14:22:36 PST
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.
Comment 14 Mark Lam 2019-02-20 14:31:04 PST
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.
Comment 15 Mark Lam 2019-02-20 15:32:05 PST
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.
Comment 16 Mark Lam 2019-02-20 15:59:28 PST
Created attachment 362559 [details]
patch for landing.
Comment 17 Mark Lam 2019-02-20 16:11:32 PST
Landed in r241849: <http://trac.webkit.org/r241849>.