Bug 203285 – [JSC] Figure out missing prepareCallOperation

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

[Yusuke Suzuki]

prepareCallOperation is not necessary for ARM64 / X86_64 JIT environments. This means that we could miss some of them.
I'll introduce a mechanism to figure out missing ones.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #0)
> prepareCallOperation is not necessary for ARM64 / X86_64 JIT environments.
> This means that we could miss some of them.
> I'll introduce a mechanism to figure out missing ones.

Note that this prepareCallOperation is only related to JIT.

[Yusuke Suzuki]

Created attachment 381661 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 381661 [details]
Patch

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

> Source/WTF/wtf/Platform.h:1007
>  #if COMPILER(GCC_COMPATIBLE) && (CPU(ARM64) || CPU(X86_64)) && (OS(LINUX) || OS(DARWIN))

I now believe that we do not need to have `(OS(LINUX) || OS(DARWIN))`. And it makes everything simple.

[Yusuke Suzuki]

Comment on attachment 381661 [details]
Patch

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

>> Source/WTF/wtf/Platform.h:1007
>>  #if COMPILER(GCC_COMPATIBLE) && (CPU(ARM64) || CPU(X86_64)) && (OS(LINUX) || OS(DARWIN))
> 
> I now believe that we do not need to have `(OS(LINUX) || OS(DARWIN))`. And it makes everything simple.

If we are using clang/gcc on ARM64/X86_64, __builtin_frame_address should work well.

[Yusuke Suzuki]

Created attachment 381662 [details]
Patch

[Yusuke Suzuki]

Created attachment 381665 [details]
Patch

[Zan Dobersek]

(In reply to Yusuke Suzuki from comment #6)
> Created attachment 381665 [details]
> Patch

The patch does fix 32-bit ARMv7, as suggested in bug #202392.

[Yusuke Suzuki]

Created attachment 381741 [details]
Patch

[Yusuke Suzuki]

Created attachment 381748 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 381748 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:16
> +        We also found that FTL's custom getter calling is putting wrong value to vm.topCallFrame. This patch fixes it too.

In addition, I started renaming all operation functions like, `operationXXX`.

[Yusuke Suzuki]

Comment on attachment 381748 [details]
Patch

I need to update it slightly.

[Yusuke Suzuki]

Created attachment 381749 [details]
Patch

[Mark Lam]

Comment on attachment 381749 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/FrameTracers.h:120
> +        // When debugging mode, USE(BUILTIN_FRAME_ADDRESS) environment also puts frame pointer to vm.topCallFrame.
> +        // And we can ensure it is working by comparing with the result of __builtin_frame_adress.

I suggest rephrasing this as:

If !ASSERT_DISABLED and USE(BUILTIN_FRAME_ADDRESS), prepareCallOperation() will put the frame pointer into vm.topCallFrame. We can ensure here that a call to prepareCallOperation() (or its equivalent) is not missing by comparing vm.topCallFrame to the result of __builtin_frame_address which is passed in as callFrame.

[Yusuke Suzuki]

Comment on attachment 381749 [details]
Patch

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

>> Source/JavaScriptCore/interpreter/FrameTracers.h:120
>> +        // And we can ensure it is working by comparing with the result of __builtin_frame_adress.
> 
> I suggest rephrasing this as:
> 
> If !ASSERT_DISABLED and USE(BUILTIN_FRAME_ADDRESS), prepareCallOperation() will put the frame pointer into vm.topCallFrame. We can ensure here that a call to prepareCallOperation() (or its equivalent) is not missing by comparing vm.topCallFrame to the result of __builtin_frame_address which is passed in as callFrame.

Sounds nice.
I ensured that mac-debug-wk1 is not showing new regression. And I ensured that this passes JSC tests locally. I'll land it :)

[Yusuke Suzuki]

Committed r251518: <https://trac.webkit.org/changeset/251518>

[Radar WebKit Bug Importer]

<rdar://problem/56563125>