Bug 202569

Summary: [JSC] Change signature of HostFunction to (JSGlobalObject*, CallFrame*)
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, benjamin, calvaris, cdumez, dbates, dino, eric.carlson, esprehn+autocc, ews-watchlist, glenn, graouts, gyuyoung.kim, hi, jer.noble, jiewen_tan, joepeck, jsbell, kangil.han, keith_miller, kondapallykalyan, macpherson, mark.lam, menard, msaboff, philipj, pmatos, saam, sergio, tsavell, tzagallo, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 202392    
Attachments:
Description Flags
Patch
none
WIP
none
WIP
none
Patch
none
Patch
none
Patch saam: review+

Description Yusuke Suzuki 2019-10-04 00:46:13 PDT
...
Comment 1 Radar WebKit Bug Importer 2019-10-04 00:47:00 PDT
<rdar://problem/55975858>
Comment 2 Yusuke Suzuki 2019-10-04 01:30:49 PDT
Created attachment 380195 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2019-10-04 02:04:10 PDT
Created attachment 380200 [details]
WIP
Comment 4 Yusuke Suzuki 2019-10-04 02:56:36 PDT
Created attachment 380207 [details]
WIP
Comment 5 Yusuke Suzuki 2019-10-04 02:59:35 PDT
OK, let's see the status of EWS.
Comment 6 Yusuke Suzuki 2019-10-04 13:01:54 PDT
Created attachment 380237 [details]
Patch
Comment 7 Yusuke Suzuki 2019-10-04 13:37:18 PDT
Created attachment 380247 [details]
Patch
Comment 8 Yusuke Suzuki 2019-10-04 15:39:07 PDT
Created attachment 380260 [details]
Patch
Comment 9 Yusuke Suzuki 2019-10-04 15:41:39 PDT
Comment on attachment 380260 [details]
Patch

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

The other parts are mechanical one.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:840
>      ProtoCallFrame protoCallFrame;

ProtoCallFrame's globalObject change is important.

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:273
>      // Calling convention:      f(edi, esi, edx, ecx, ...);

ThunkGenerators.cpp change is important.

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:265
> +    makeCall(entry, protoCallFrame, t3, t4)

LLInt change is important.
Comment 10 Saam Barati 2019-10-07 15:30:43 PDT
Comment on attachment 380260 [details]
Patch

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

r=me

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:273
>>      // Calling convention:      f(edi, esi, edx, ecx, ...);
> 
> ThunkGenerators.cpp change is important.

can we file a bug on using GPRInfo here instead of naming registers?

> Source/JavaScriptCore/jit/ThunkGenerators.cpp:337
> +    static_assert(GPRInfo::regT2 != GPRInfo::argumentGPR0);
> +    static_assert(GPRInfo::regT2 != GPRInfo::argumentGPR1);

I think arm64 and x86_64 on Windows/*nix can easily be written using GPRInfo and RegisterSet. As I said above, filing a bug for this work would be nice (you don't necessarily need to do the work, but we should just have a bug)
Comment 11 Yusuke Suzuki 2019-10-07 15:47:46 PDT
Comment on attachment 380260 [details]
Patch

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

Thanks!

>>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:273
>>>      // Calling convention:      f(edi, esi, edx, ecx, ...);
>> 
>> ThunkGenerators.cpp change is important.
> 
> can we file a bug on using GPRInfo here instead of naming registers?

Yes, filed https://bugs.webkit.org/show_bug.cgi?id=202657.

>> Source/JavaScriptCore/jit/ThunkGenerators.cpp:337
>> +    static_assert(GPRInfo::regT2 != GPRInfo::argumentGPR1);
> 
> I think arm64 and x86_64 on Windows/*nix can easily be written using GPRInfo and RegisterSet. As I said above, filing a bug for this work would be nice (you don't necessarily need to do the work, but we should just have a bug)

Right. Fixed. https://bugs.webkit.org/show_bug.cgi?id=202657
Comment 12 Yusuke Suzuki 2019-10-07 16:13:52 PDT
Committed r250803: <https://trac.webkit.org/changeset/250803>
Comment 13 Yusuke Suzuki 2019-10-07 16:53:34 PDT
Committed r250807: <https://trac.webkit.org/changeset/250807>
Comment 14 Paulo Matos 2019-10-09 06:32:53 PDT
Since this bug's patch made trunk, that we have 32bits failing to build in Debug mode or segfaulting in tests when built in Release mode - opened bug 202748
Comment 15 Truitt Savell 2019-10-09 10:58:41 PDT
It looks like the changes in r250803 and the subsequent change in 250807 has caused the windows builds to fail. 

build:
https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/7170
https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/7173
Comment 16 Yusuke Suzuki 2019-10-10 12:40:54 PDT
Committed r250982: <https://trac.webkit.org/changeset/250982>