Summary: | We should add code to validate expected GC activity modelled by doesGC() against what the runtime encounters. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, ews-watchlist, fpizlo, keith_miller, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mark Lam
2019-01-28 16:35:28 PST
z 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. Created attachment 362432 [details]
proposed patch.
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 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 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 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 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. (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. (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 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 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 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 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. Created attachment 362559 [details]
patch for landing.
Landed in r241849: <http://trac.webkit.org/r241849>. |