Bug 212680 – Enhance DoesGC verification to print more useful info when verification fails.

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

[Mark Lam]

Previously, we were just getting an assertion failure without too much extra info.

[Mark Lam]

Created attachment 400879 [details]
proposed patch.

[Mark Lam]

Created attachment 400881 [details]
proposed patch.

[Mark Lam]

Created attachment 400883 [details]
proposed patch.

[Mark Lam]

Created attachment 400887 [details]
proposed patch.

[Mark Lam]

Comment on attachment 400887 [details]
proposed patch.

Looks like the patch is ready for a review.

[Yusuke Suzuki]

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.

[Mark Lam]

Comment on attachment 400887 [details]
proposed patch.

Taking out of review while I address Yusuke's concern.

[Mark Lam]

Created attachment 400889 [details]
proposed patch.

Fixed store64 to use a single store.

[Yusuke Suzuki]

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)`?

[Mark Lam]

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.

[Mark Lam]

Thanks for the review.  Landed in r262475: <http://trac.webkit.org/r262475>.

[Radar WebKit Bug Importer]

<rdar://problem/63908362>

[Mark Lam]

Rolled out in r262478: <http://trac.webkit.org/r262478> because of broken Windows build.

[Mark Lam]

Created attachment 400902 [details]
test for Windows bot.

[Mark Lam]

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

[Mark Lam]

Landed Windows debug build fix in r262517: <http://trac.webkit.org/r262517>.