| Summary: | Enhance DoesGC verification to print more useful info when verification fails. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | annulen, ews-watchlist, gyuyoung.kim, keith_miller, msaboff, pmatos, ryuan.choi, saam, sergio, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 212687 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Mark Lam
2020-06-02 19:05:20 PDT
Created attachment 400879 [details]
proposed patch.
Created attachment 400881 [details]
proposed patch.
Created attachment 400883 [details]
proposed patch.
Created attachment 400887 [details]
proposed patch.
Comment on attachment 400887 [details]
proposed patch.
Looks like the patch is ready for a review.
Comment on attachment 400887 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=400887&action=review > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:977 > + } We should do the same thing to `void store64(TrustedImm64 imm, ImplicitAddress address)`. This is splitting store64 into two stores. This is not good since it is not atomic. If the client assumes that this store is atomic in 64bit width, we have a bad time. Comment on attachment 400887 [details]
proposed patch.
Taking out of review while I address Yusuke's concern.
Created attachment 400889 [details]
proposed patch.
Fixed store64 to use a single store.
Comment on attachment 400889 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=400889&action=review r=me with comment > Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:988 > > + void store64(TrustedImm64 imm, void* address) > + { > + auto src = scratchRegister(); > + move(imm, src); > + swap(src, X86Registers::eax); > + m_assembler.movq_EAXm(address); > + swap(src, X86Registers::eax); > + } > + Why not just doing the same thing to `void store64(TrustedImm64 imm, ImplicitAddress address)`? Comment on attachment 400889 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=400889&action=review >> Source/JavaScriptCore/assembler/MacroAssemblerX86_64.h:988 >> + > > Why not just doing the same thing to `void store64(TrustedImm64 imm, ImplicitAddress address)`? I talked with Yusuke offline about this: the ImplicitAddress takes a register as the base of the address. My store64 does not have an extra register to use as that base. However, we noted that in the scenario where the imm fits in 32-bits, we can use the scratchRegister to store the address instead of the imm value. So, I added a fast path for this case. Thanks for the review. Landed in r262475: <http://trac.webkit.org/r262475>. Rolled out in r262478: <http://trac.webkit.org/r262478> because of broken Windows build. Created attachment 400902 [details]
test for Windows bot.
Now that the true cause of the Windows bot build failure has been resolved in https://bugs.webkit.org/show_bug.cgi?id=212687, we can re-land this. Re-landed in r262513: <http://trac.webkit.org/r262513>. Landed Windows debug build fix in r262517: <http://trac.webkit.org/r262517>. |