Bug 212825 – Make FAST_JIT_PERMISSIONS check in LinkBuffer::copyCompactAndLinkCode a runtime check

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

[Michael Saboff]

There are some ARM64 devices that have runtime support for APRR, but the current way we link in copyCompactAndLinkCode() causes us to use a side buffer to link the code before copying the results to the JIT memory.  By making the FAST_JIT_PERMISSION paths runtime selected, we can eliminate the allocation of the buffer and the subsequent copying out of the buffer.

This is a follow to https://bugs.webkit.org/show_bug.cgi?id=212765.

[Michael Saboff]

<rdar://problem/64030110>

[Michael Saboff]

Created attachment 401158 [details]
Patch

[Michael Saboff]

Created attachment 401161 [details]
Patch with speculative build fixes

[Michael Saboff]

Created attachment 401176 [details]
Patch

More changes to make the code statically compiled where appropriate.  This should fix the ARMv7 build.

[Saam Barati]

Comment on attachment 401176 [details]
Patch

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

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:282
> +    IF_FAST_JIT_PERMISSIONS(os_thread_self_restrict_rwx_to_rw());

why not just use a normal if statement here? Doesn't feel like canonical webkit style.

I think 
if (useFastJITPermissions())
is much easier to parse and read than macros as control flow.

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:371
> +        IF_FAST_JIT_PERMISSIONS_ELSE(MacroAssembler::link<memcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target), \
> +            MacroAssembler::link<performJITMemcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target));

same comment

[Michael Saboff]

Created attachment 401188 [details]
Patch with suggested changes

(In reply to Saam Barati from comment #5)
> Comment on attachment 401176 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401176&action=review
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:282
> > +    IF_FAST_JIT_PERMISSIONS(os_thread_self_restrict_rwx_to_rw());
> 
> why not just use a normal if statement here? Doesn't feel like canonical
> webkit style.
> 
> I think 
> if (useFastJITPermissions())
> is much easier to parse and read than macros as control flow.
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:371
> > +        IF_FAST_JIT_PERMISSIONS_ELSE(MacroAssembler::link<memcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target), \
> > +            MacroAssembler::link<performJITMemcpy>(jumpsToLink[i], outData + jumpsToLink[i].from(), location, target));
> 
> same comment

Here's a patch with the suggested changes.

[Sam Weinig]

Comment on attachment 401188 [details]
Patch with suggested changes

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

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:137
> +ALWAYS_INLINE bool useFastJITPermissions()
> +{
> +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP)
> +    return true;
> +#elif ENABLE(FAST_JIT_PERMISSIONS)
> +    return g_jscConfig.useFastPermisionsJITCopy;
> +#else
> +    return false;
> +#endif
> +}

If you moved this to ExecutableAllocator.h, you could use useFastJITPermissions() to slightly simplify performJITMemcpy as well (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)).

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:268
> +        os_thread_self_restrict_rwx_to_rw();

Is this still in an #ifdef that is Darwin specific? Or are we just getting away with this because we only build branch compaction on Darwin systems?

[Saam Barati]

Comment on attachment 401188 [details]
Patch with suggested changes

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        Made the FAST_JIT_PERMISSIONS code to be runtime conditional.

"code to be" => maybe something a bit more specific: "code when linking to be"

> Source/JavaScriptCore/ChangeLog:16
> +        Broke out the "verify hash" conditionally compiled code with a file local
> +        ENABLE_VERIFY_JIT_HASH macro for readability.

this is nicer

> Source/JavaScriptCore/assembler/LinkBuffer.cpp:250
> +#if ENABLE(VERIFY_JIT_HASH)

can we assert that useFastJITPermissions() is true

alternatively, just return true from that function #if ENABLE(VERIFY_JIT_HASH)

[Saam Barati]

Comment on attachment 401188 [details]
Patch with suggested changes

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Made the FAST_JIT_PERMISSIONS code to be runtime conditional.
> 
> "code to be" => maybe something a bit more specific: "code when linking to be"

or maybe even more specific, say something about the side buffer vs malloc buffer.

[Michael Saboff]

(In reply to Sam Weinig from comment #7)
> Comment on attachment 401188 [details]
> Patch with suggested changes
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401188&action=review
> 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:137
> > +ALWAYS_INLINE bool useFastJITPermissions()
> > +{
> > +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP)
> > +    return true;
> > +#elif ENABLE(FAST_JIT_PERMISSIONS)
> > +    return g_jscConfig.useFastPermisionsJITCopy;
> > +#else
> > +    return false;
> > +#endif
> > +}
> 
> If you moved this to ExecutableAllocator.h, you could use
> useFastJITPermissions() to slightly simplify performJITMemcpy as well
> (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)).

Let me look into that.
 
> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:268
> > +        os_thread_self_restrict_rwx_to_rw();
> 
> Is this still in an #ifdef that is Darwin specific? Or are we just getting
> away with this because we only build branch compaction on Darwin systems?

Yes it is.  I posted this patch to see if ALWAYS_LINE could get the other compilers / SDKs to work, but it wasn't successful.

[Saam Barati]

r=me still with old Macro control flow version, after talking with Michael, compiling in the control flow is leading to build annoyance

[Michael Saboff]

Created attachment 401220 [details]
Patch for Landing

> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:137
> > +ALWAYS_INLINE bool useFastJITPermissions()
> > +{
> > +#if ENABLE(FAST_JIT_PERMISSIONS) && !ENABLE(SEPARATED_WX_HEAP)
> > +    return true;
> > +#elif ENABLE(FAST_JIT_PERMISSIONS)
> > +    return g_jscConfig.useFastPermisionsJITCopy;
> > +#else
> > +    return false;
> > +#endif
> > +}
> 
> If you moved this to ExecutableAllocator.h, you could use
> useFastJITPermissions() to slightly simplify performJITMemcpy as well
> (remove one #if/endif ENABLE(SEPARATED_WX_HEAP)).

Done.

> > Source/JavaScriptCore/assembler/LinkBuffer.cpp:250
> > +#if ENABLE(VERIFY_JIT_HASH)
> 
> can we assert that useFastJITPermissions() is true
> 
> alternatively, just return true from that function #if
> ENABLE(VERIFY_JIT_HASH)

Although these sections of code were part of the same "#if CPU(ARM64E) && ENABLE(FAST_JIT_PERMISSIONS)" sections, this is actually independent of FAST_JIT_PERMISSIONS.  You'll see elsewhere (AssemblerBuffer.h ~183) that the assembly hash code is simply #if CPU(ARM64E).  I have adjusted the setting of ENABLE_VERIFY_JIT_HASH accordingly.

[Michael Saboff]

Committed r262670: <https://trac.webkit.org/changeset/262670>