Bug 203285 - [JSC] Figure out missing prepareCallOperation
Summary: [JSC] Figure out missing prepareCallOperation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 202392
Blocks: 203290
  Show dependency treegraph
 
Reported: 2019-10-22 22:17 PDT by Yusuke Suzuki
Modified: 2019-10-23 18:01 PDT (History)
13 users (show)

See Also:


Attachments
Patch (23.85 KB, patch)
2019-10-22 23:35 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (25.58 KB, patch)
2019-10-22 23:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (174.83 KB, patch)
2019-10-23 00:56 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (219.76 KB, patch)
2019-10-23 15:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (224.67 KB, patch)
2019-10-23 15:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (224.67 KB, patch)
2019-10-23 16:02 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-10-22 22:17:43 PDT
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.
Comment 1 Yusuke Suzuki 2019-10-22 22:18:03 PDT
(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.
Comment 2 Yusuke Suzuki 2019-10-22 23:35:17 PDT
Created attachment 381661 [details]
Patch
Comment 3 Yusuke Suzuki 2019-10-22 23:36:41 PDT
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.
Comment 4 Yusuke Suzuki 2019-10-22 23:37:54 PDT
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.
Comment 5 Yusuke Suzuki 2019-10-22 23:40:37 PDT
Created attachment 381662 [details]
Patch
Comment 6 Yusuke Suzuki 2019-10-23 00:56:49 PDT
Created attachment 381665 [details]
Patch
Comment 7 Zan Dobersek 2019-10-23 12:41:25 PDT
(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.
Comment 8 Yusuke Suzuki 2019-10-23 15:36:16 PDT
Created attachment 381741 [details]
Patch
Comment 9 Yusuke Suzuki 2019-10-23 15:54:16 PDT
Created attachment 381748 [details]
Patch
Comment 10 Yusuke Suzuki 2019-10-23 15:55:52 PDT
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`.
Comment 11 Yusuke Suzuki 2019-10-23 16:00:10 PDT
Comment on attachment 381748 [details]
Patch

I need to update it slightly.
Comment 12 Yusuke Suzuki 2019-10-23 16:02:28 PDT
Created attachment 381749 [details]
Patch
Comment 13 Mark Lam 2019-10-23 17:07:07 PDT
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.
Comment 14 Yusuke Suzuki 2019-10-23 17:24:06 PDT
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 :)
Comment 15 Yusuke Suzuki 2019-10-23 18:00:13 PDT
Committed r251518: <https://trac.webkit.org/changeset/251518>
Comment 16 Radar WebKit Bug Importer 2019-10-23 18:01:16 PDT
<rdar://problem/56563125>