Bug 203285

Summary: [JSC] Figure out missing prepareCallOperation
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, dbates, ews-watchlist, fujii.hironori, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 202392    
Bug Blocks: 203290    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch mark.lam: review+

Yusuke Suzuki
Reported 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.
Attachments
Patch (23.85 KB, patch)
2019-10-22 23:35 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (25.58 KB, patch)
2019-10-22 23:40 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (174.83 KB, patch)
2019-10-23 00:56 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (219.76 KB, patch)
2019-10-23 15:36 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (224.67 KB, patch)
2019-10-23 15:54 PDT, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (224.67 KB, patch)
2019-10-23 16:02 PDT, Yusuke Suzuki 		mark.lam: review+
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 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.
Yusuke Suzuki
Comment 2 2019-10-22 23:35:17 PDT
Created attachment 381661 [details] Patch
Yusuke Suzuki
Comment 3 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.
Yusuke Suzuki
Comment 4 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.
Yusuke Suzuki
Comment 5 2019-10-22 23:40:37 PDT
Created attachment 381662 [details] Patch
Yusuke Suzuki
Comment 6 2019-10-23 00:56:49 PDT
Created attachment 381665 [details] Patch
Zan Dobersek
Comment 7 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.
Yusuke Suzuki
Comment 8 2019-10-23 15:36:16 PDT
Created attachment 381741 [details] Patch
Yusuke Suzuki
Comment 9 2019-10-23 15:54:16 PDT
Created attachment 381748 [details] Patch
Yusuke Suzuki
Comment 10 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`.
Yusuke Suzuki
Comment 11 2019-10-23 16:00:10 PDT
Comment on attachment 381748 [details] Patch I need to update it slightly.
Yusuke Suzuki
Comment 12 2019-10-23 16:02:28 PDT
Created attachment 381749 [details] Patch
Mark Lam
Comment 13 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.
Yusuke Suzuki
Comment 14 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 :)
Yusuke Suzuki
Comment 15 2019-10-23 18:00:13 PDT
Committed r251518: <https://trac.webkit.org/changeset/251518>
Radar WebKit Bug Importer
Comment 16 2019-10-23 18:01:16 PDT
<rdar://problem/56563125>
Note You need to log in before you can comment on or make changes to this bug.