Bug 233822 – [JSC] Port EXTRA_CTI_THUNKS to all platforms

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

[Geza Lore]

[JSC] Port EXTRA_CTI_THUNKS to all platforms

[Geza Lore]

Created attachment 445869 [details]
WIP - EWS sanity check

[Geza Lore]

Created attachment 445870 [details]
WIP - remove unused arguments

[Geza Lore]

Created attachment 445873 [details]
WIP - build fix

[Radar WebKit Bug Importer]

<rdar://problem/86326948>

[Geza Lore]

Created attachment 447123 [details]
WIP

[Geza Lore]

Created attachment 448291 [details]
WIP

[Geza Lore]

Created attachment 448301 [details]
Windows fix 1

[Geza Lore]

Created attachment 448303 [details]
Windows fix 2

[Geza Lore]

Created attachment 448312 [details]
Windows fix 3

[Geza Lore]

Created attachment 448385 [details]
cleanup 1

[Geza Lore]

Created attachment 448400 [details]
cleanup 2

[Geza Lore]

Created attachment 448407 [details]
Xcode build fix [hopefully]

[Geza Lore]

Created attachment 448482 [details]
For review

[Geza Lore]

Created attachment 448500 [details]
minor style fix

[Yusuke Suzuki]

Comment on attachment 448500 [details]
minor style fix

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

Nice. But found several bugs.
In ARM64E, we need to tag return address register in the prologue. And we should not tag it when doing a tail call. It seems that this patch broke these things.
Please check this is met by comparing old code and new code.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:44
> +    static inline constexpr RegisterID dataTempRegister = ARMRegisters::ip;
> +    static inline constexpr RegisterID addressTempRegister = ARMRegisters::r6;
>  
> -    static constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister = ARMRegisters::d7;
> +    static inline constexpr ARMRegisters::FPDoubleRegisterID fpTempRegister = ARMRegisters::d7;

When constexpr is specified, static member variable is implicitly inline. So this is not necessary.
http://eel.is/c++draft/dcl.constexpr#1.sentence-3

> Source/JavaScriptCore/jit/CCallHelpers.cpp:98
> +static_assert(!((maxFrameExtentForSlowPathCall + 2*sizeof(CPURegister)) % 16), "Stack must be aligned after CTI thunk entry");

We should add space between 2 and * and sizeof.

> Source/JavaScriptCore/jit/CCallHelpers.cpp:100
> +void CCallHelpers::emitCTIThunkPrologue()

This is not correct. It is removing tagging from the original code.
Please ensure that these code is exactly the same to the original sequence of generated code.

> Source/JavaScriptCore/jit/JITOpcodes.cpp:-1286
> -    jit.tagReturnAddress();

This is missing.

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:-2625
> -    jit.tagReturnAddress();

This is removed.

[Geza Lore]

Created attachment 448839 [details]
fix PACs in thunks

[Geza Lore]

Created attachment 449389 [details]
fix PACs in thunks - v2

[Geza Lore]

Created attachment 449400 [details]
fix PACs in thunks - v2

[Yusuke Suzuki]

I tried applying it and found that all tests are crashing, probably, PAC is broken with this change.
Can we split this patch into two?

1. Enabling EXTRA_CTI_THUNKS for Windows etc. part
2. Enhancing the existing thunks part

(1) should not change the existing thunk so that it will not break PAC.

[Geza Lore]

Created attachment 450418 [details]
rebase

[Saam Barati]

I think I know what your arm64e issues are. Take resolve_scope:

On the fast path, we call:
jit.tagReturnAddress()

Then, if we go to the slow path, inside slow_op_resolve_scopeGenerator, we call:
jit.emitCTIThunkPrologue()

which does:
void CCallHelpers::emitCTIThunkPrologue()
{
    tagReturnAddress();


This leads us to tagging an already tagged value. So you'll want to audit all fast path to slow paths. Maybe we can just call untag? Or maybe we don't need to tag at all if all fast paths already tag? Otherwise, we could pass in a boolean to this function if it needs to tag. It probably depends on the context of the opcode. resolve_scope is just jumping to its slow paths. Maybe other opcodes call it?

Anyways, I think this should be enough info to go in and audit the code with this in mind.

[Geza Lore]

Created attachment 450677 [details]
fix double tagging of return address in resolve_scope and get_from_scope

[Geza Lore]

The issue indeed seems to have been double tagging in resolve_scope and get_from_scope. Thanks again for the assistance tracking this down. 

FWIW I managed to get a mostly functional arm64e build on an M1 device (still fails some 6% of tests, most having to do with i18l), and I confirmed that with the latest version of this patch there are no new test failures, so I'm now fairly confident this is correct.

[Saam Barati]

Comment on attachment 450677 [details]
fix double tagging of return address in resolve_scope and get_from_scope

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

r=me

> Source/JavaScriptCore/ChangeLog:31
> +        Enabling the extra CTI thunks on ARMv7/Thumb-2 saves about 25%

Nice!

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 450677 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Geza Lore]

Created attachment 453486 [details]
Patch

[EWS]

Committed r290647 (247920@main): <https://commits.webkit.org/247920@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453486 [details].