Bug 197993 - Allow OSR exit to the LLInt
Summary: Allow OSR exit to the LLInt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
Depends on:
Blocks: 202503
  Show dependency tree / graph
 
Reported: 2019-05-17 11:56 PDT by Saam Barati
Modified: 2019-10-09 18:02 PDT (History)
19 users (show)

See Also:

Attachments
WIP (26.66 KB, patch)
2019-05-17 18:31 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (60.04 KB, patch)
2019-05-19 14:50 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (60.36 KB, patch)
2019-05-19 14:55 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (60.98 KB, patch)
2019-05-19 16:48 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (61.38 KB, patch)
2019-05-19 21:34 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (61.38 KB, patch)
2019-05-19 21:37 PDT, Saam Barati 		msaboff: review+
Details | Formatted Diff | Diff
Bad derived asm (WinCairo) (2.59 MB, text/plain)
2019-05-20 16:11 PDT, Ross Kirsling 		no flags Details
WIP (55.56 KB, patch)
2019-10-01 18:20 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
WinCairo ASM from trunk build (3.77 MB, text/plain)
2019-10-02 15:27 PDT, Ross Kirsling 		no flags Details
WinCairo ASM from patch 379977 (errors out in mid-assembly) (3.80 MB, text/plain)
2019-10-02 15:29 PDT, Ross Kirsling 		no flags Details
patch (58.45 KB, patch)
2019-10-02 17:07 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch (58.95 KB, patch)
2019-10-03 16:34 PDT, Saam Barati 		tzagallo: review+
Details | Formatted Diff | Diff
patch for landing (60.86 KB, patch)
2019-10-04 13:22 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch for landing (61.00 KB, patch)
2019-10-04 13:28 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
patch for landing (60.51 KB, patch)
2019-10-07 15:46 PDT, Saam Barati 		no flags Details | Formatted Diff | Diff
Show Obsolete (14) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2019-05-17 11:56:03 PDT 
Seems like a fun Friday patch. Let's see how far I get.
Comment 1 Saam Barati 2019-05-17 18:31:12 PDT 
Created attachment 370175 [details]
WIP

works for many things. Probably still has bugs. Just fixed a bug where array profiling during exit was overriding a get_by_id IC
Comment 2 Saam Barati 2019-05-17 18:37:49 PDT 
(In reply to Saam Barati from comment #1)
> Created attachment 370175 [details]
> WIP
> 
> works for many things. Probably still has bugs. Just fixed a bug where array
> profiling during exit was overriding a get_by_id IC

actually it seems like it might just work.

I need to still write the part of the patch that:
1. conditionally exits to llint/baseline depending on the compilation state of the code block. This patch is currently forcing all exits to LLInt.
2. Write a stub for get_by_id/put_by_id to return to from getter/setter calls
Comment 3 Saam Barati 2019-05-19 14:50:17 PDT 
Created attachment 370227 [details]
patch
Comment 4 Saam Barati 2019-05-19 14:55:07 PDT 
Created attachment 370228 [details]
patch

rebased
Comment 5 EWS Watchlist 2019-05-19 14:59:03 PDT 
Attachment 370228 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/llint/LLIntData.h:158:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:161:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:162:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:164:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:165:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:167:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/llint/LLIntData.h:168:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 7 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Saam Barati 2019-05-19 16:48:30 PDT 
Created attachment 370229 [details]
patch

try to fix 32-bit
Comment 7 Saam Barati 2019-05-19 21:34:15 PDT 
Created attachment 370237 [details]
patch

Try to fix mips/armv7
Comment 8 Saam Barati 2019-05-19 21:37:27 PDT 
Created attachment 370238 [details]
patch

fix armv7+mips take 2
Comment 9 Tadeu Zagallo 2019-05-19 23:25:42 PDT 
Comment on attachment 370238 [details]
patch

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

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:258
> +                    skipProfile = jit.branch8(CCallHelpers::NotEqual, CCallHelpers::AbsoluteAddress(&metadata.m_mode), CCallHelpers::TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));

Just to check if I understand it. Is this necessary because we may exit, execute some LLInt code that changes the mode of this get_by_id and re-enter FTL from LLInt with a different mode?

> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:1630
> -putByValOp(put_by_val, OpPutByVal)
> +putByValOp(put_by_val, OpPutByVal, macro (size, dispatch)
> +    .osrReturnPoint:
> +        getterSetterOSRExitReturnPoint(op_put_by_val, size)
> +        dispatch()
> +end)
>  
> -putByValOp(put_by_val_direct, OpPutByValDirect)
> +putByValOp(put_by_val_direct, OpPutByValDirect, macro (a, b) end)

Not this patch, but this could be in LowLevelInterpreter.asm

> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918
> +    macro defineWide()
> +        global _%opcodeName%_return_location_wide
> +        _%opcodeName%_return_location_wide:
> +    end
> +
> +    macro defineNarrow()
> +        global _%opcodeName%_return_location_narrow
> +        _%opcodeName%_return_location_narrow:
> +    end
> +
> +    size(defineNarrow, defineWide, macro (f) f() end)

nit: you could move this into e.g. returnLocalForOpcode(size, opcodeName)
Comment 10 Michael Saboff 2019-05-20 08:22:48 PDT 
Comment on attachment 370238 [details]
patch

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

r=me

> Source/JavaScriptCore/ChangeLog:29
> +        is a LLInt frame. Let's consider an inline tack A->B->C, where A is a LLInt frame.

typo *stack*
Comment 11 Saam Barati 2019-05-20 11:08:55 PDT 
Comment on attachment 370238 [details]
patch

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

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:258
>> +                    skipProfile = jit.branch8(CCallHelpers::NotEqual, CCallHelpers::AbsoluteAddress(&metadata.m_mode), CCallHelpers::TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));
> 
> Just to check if I understand it. Is this necessary because we may exit, execute some LLInt code that changes the mode of this get_by_id and re-enter FTL from LLInt with a different mode?

So the issue is this:
1. We make the LLInt IC ArrayLength. This makes it so accessing getArrayProfile returns non-null.
2. We take this OSR exit
3. We rewrite the IC in the LLInt to go from ArrayLength to a normal structure IC
4. We call the function in question again (now it's back in the DFG/FTL)
5. We OSR exit, writing to this array profile
6. Now, parts of the data structure used for the LLInt IC is garbage because of this array profile store.

That's why I added this runtime check. Doing the array profile only makes sense when we're in the ArrayLength mode of the LLInt IC.
Comment 12 Saam Barati 2019-05-20 11:15:22 PDT 
Comment on attachment 370238 [details]
patch

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

>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918
>> +    size(defineNarrow, defineWide, macro (f) f() end)
> 
> nit: you could move this into e.g. returnLocalForOpcode(size, opcodeName)

I don't follow. What's returnLocalForOpcode?
Comment 13 Saam Barati 2019-05-20 11:16:51 PDT 
Comment on attachment 370238 [details]
patch

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

> Tools/ChangeLog:17
> +2019-05-19  Saam barati  <sbarati@apple.com>
> +
> +        Allow OSR exit to the LLInt
> +        https://bugs.webkit.org/show_bug.cgi?id=197993
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Scripts/run-jsc-stress-tests:

Will fix.
Comment 14 Tadeu Zagallo 2019-05-20 11:42:43 PDT 
Comment on attachment 370238 [details]
patch

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

>>> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918
>>> +    size(defineNarrow, defineWide, macro (f) f() end)
>> 
>> nit: you could move this into e.g. returnLocalForOpcode(size, opcodeName)
> 
> I don't follow. What's returnLocalForOpcode?

I meant that you could move this code to its own macro, since you do the same thing in getterSetterOSRExitReturnPoint
Comment 15 Saam Barati 2019-05-20 12:03:06 PDT 
Trying to resolve windows build issue now
Comment 16 Ross Kirsling 2019-05-20 16:11:08 PDT 
Created attachment 370276 [details]
Bad derived asm (WinCairo)

Here's WinCairo's LowLevelInterpreterWin.asm (the derived source generated instead of LLIntAssembly.h on Windows).

FWIW, I'm seeing that if I comment out the two `size(defineNarrow, defineWide, macro (f) f() end)` lines then the compilation succeeds and there are straightforward linking errors like `unresolved external symbol op_call_return_location_wide`.
Comment 17 Caio Lima 2019-07-18 04:46:07 PDT 
About issues on ARMv7, I've done a little investigation to understand what's going on. The problem is how we implement `globaladdr` in `offlineasm/arm.rb`. It is using `$asm.deferNextLabelAction` to generate some code as a kind of "constant pool" to properly load global labels. With this Patch, we now emit global labels in the middle of a LLInt operation (e.g Source/JavaScriptCore/llint/LowLevelInterpreter.asm:918) and `@deferredNextLabelActions` are bing called "prematurely". One propose fix for ARMv7 I've made and tested is https://github.com/caiolima/webkit/commit/5ae730134127d560010c88dc96b5c3f703de4b23, but  I don't think this is the right fix. Maybe we want to keep this `@deferredNextLabelActions` as it is right now and have a new `@deferredNextOpCodeActions`, or something like that, to be used by `globaladdr`.
Comment 18 Saam Barati 2019-10-01 16:21:29 PDT 
gonna pick this back up
Comment 19 Saam Barati 2019-10-01 18:20:36 PDT 
Created attachment 379977 [details]
WIP
Comment 20 Saam Barati 2019-10-02 12:26:53 PDT 
I don't understand why Win is failing. I looked through the emitted Asm from a few months ago. Also read some offlineasm code. It seems like it should work to me. So clearly there is some detail I'm missing.
Comment 21 Ross Kirsling 2019-10-02 15:27:06 PDT 
Created attachment 380065 [details]
WinCairo ASM from trunk build
Comment 22 Ross Kirsling 2019-10-02 15:29:40 PDT 
Created attachment 380066 [details]
WinCairo ASM from patch 379977 (errors out in mid-assembly)

>  Assembling: .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(248) : error A2006:undefined symbol : _llint_op_has_indexed_property
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(251) : error A2006:undefined symbol : _llint_op_profile_control_flow
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(254) : error A2006:undefined symbol : _llint_op_profile_type
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(257) : error A2006:undefined symbol : _llint_op_catch
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(260) : error A2006:undefined symbol : _llint_op_get_from_arguments
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(263) : error A2006:undefined symbol : _llint_op_put_to_scope
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(266) : error A2006:undefined symbol : _llint_op_create_this
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(269) : error A2006:undefined symbol : _llint_op_get_internal_field
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(272) : error A2006:undefined symbol : _llint_op_create_promise
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(275) : error A2006:undefined symbol : _llint_op_get_from_scope
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(278) : error A2006:undefined symbol : _llint_op_create_generator
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(281) : error A2006:undefined symbol : _llint_op_resolve_scope
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(287) : error A2006:undefined symbol : _llint_op_construct_varargs
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(293) : error A2006:undefined symbol : _llint_op_construct
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(296) : error A2006:undefined symbol : _llint_op_new_object
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(299) : error A2006:undefined symbol : _llint_op_new_array
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(302) : error A2006:undefined symbol : _llint_op_new_array_with_size
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(305) : error A2006:undefined symbol : _llint_op_new_array_buffer
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(308) : error A2006:undefined symbol : _llint_op_tail_call_forward_arguments
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(311) : error A2006:undefined symbol : _llint_op_tail_call_varargs
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(314) : error A2006:undefined symbol : _llint_op_call_varargs
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(317) : error A2006:undefined symbol : _llint_op_call_eval
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(320) : error A2006:undefined symbol : _llint_op_tail_call
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(323) : error A2006:undefined symbol : _llint_op_call
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(326) : error A2006:undefined symbol : _llint_op_jneq_ptr
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(329) : error A2006:undefined symbol : _llint_op_put_by_val_direct
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(332) : error A2006:undefined symbol : _llint_op_put_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(335) : error A2006:undefined symbol : _llint_op_get_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(338) : error A2006:undefined symbol : _llint_op_get_direct_pname
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(341) : error A2006:undefined symbol : _llint_op_put_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(344) : error A2006:undefined symbol : _llint_op_try_get_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(350) : error A2006:undefined symbol : _llint_op_get_by_val_with_this
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(353) : error A2006:undefined symbol : _llint_op_get_by_id_with_this
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(389) : error A2006:undefined symbol : _llint_op_in_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(419) : error A2006:undefined symbol : _llint_op_is_object_or_null
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(422) : error A2006:undefined symbol : _llint_op_is_function
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(425) : error A2006:undefined symbol : _llint_op_inc
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(428) : error A2006:undefined symbol : _llint_op_dec
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(443) : error A2006:undefined symbol : _llint_op_identity_with_profile
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(449) : error A2006:undefined symbol : _llint_op_instanceof
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(452) : error A2006:undefined symbol : _llint_op_instanceof_custom
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(455) : error A2006:undefined symbol : _llint_op_typeof
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(464) : error A2006:undefined symbol : _llint_op_in_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(470) : error A2006:undefined symbol : _llint_op_pow
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(473) : error A2006:undefined symbol : _llint_op_mod
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(476) : error A2006:undefined symbol : _llint_op_beloweq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(479) : error A2006:undefined symbol : _llint_op_below
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(482) : error A2006:undefined symbol : _llint_op_greatereq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(485) : error A2006:undefined symbol : _llint_op_put_by_id_with_this
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(488) : error A2006:undefined symbol : _llint_op_greater
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(491) : error A2006:undefined symbol : _llint_op_lesseq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(494) : error A2006:undefined symbol : _llint_op_less
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(497) : error A2006:undefined symbol : _llint_op_put_by_val_with_this
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(503) : error A2006:undefined symbol : _llint_op_del_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(506) : error A2006:undefined symbol : _llint_op_put_getter_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(509) : error A2006:undefined symbol : _llint_op_put_setter_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(512) : error A2006:undefined symbol : _llint_op_put_getter_setter_by_id
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(515) : error A2006:undefined symbol : _llint_op_put_getter_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(518) : error A2006:undefined symbol : _llint_op_put_setter_by_val
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(521) : error A2006:undefined symbol : _llint_op_define_data_property
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(524) : error A2006:undefined symbol : _llint_op_define_accessor_property
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(527) : error A2006:undefined symbol : _llint_op_jmp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(530) : error A2006:undefined symbol : _llint_op_jtrue
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(533) : error A2006:undefined symbol : _llint_op_jfalse
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(536) : error A2006:undefined symbol : _llint_op_jeq_null
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(539) : error A2006:undefined symbol : _llint_op_jneq_null
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(542) : error A2006:undefined symbol : _llint_op_jundefined_or_null
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(545) : error A2006:undefined symbol : _llint_op_jnundefined_or_null
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(551) : error A2006:undefined symbol : _llint_op_jeq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(557) : error A2006:undefined symbol : _llint_op_jneq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(563) : error A2006:undefined symbol : _llint_op_jless
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(566) : error A2006:undefined symbol : _llint_op_jlesseq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(569) : error A2006:undefined symbol : _llint_op_jgreater
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(572) : error A2006:undefined symbol : _llint_op_jgreatereq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(575) : error A2006:undefined symbol : _llint_op_jnless
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(578) : error A2006:undefined symbol : _llint_op_jnlesseq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(581) : error A2006:undefined symbol : _llint_op_jngreater
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(584) : error A2006:undefined symbol : _llint_op_jngreatereq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(587) : error A2006:undefined symbol : _llint_op_jbelow
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(590) : error A2006:undefined symbol : _llint_op_jbeloweq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(593) : error A2006:undefined symbol : _llint_op_loop_hint
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(596) : error A2006:undefined symbol : _llint_op_switch_imm
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(599) : error A2006:undefined symbol : _llint_op_switch_char
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(602) : error A2006:undefined symbol : _llint_op_switch_string
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(605) : error A2006:undefined symbol : _llint_op_new_func
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(608) : error A2006:undefined symbol : _llint_op_new_func_exp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(611) : error A2006:undefined symbol : _llint_op_new_generator_func
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(614) : error A2006:undefined symbol : _llint_op_new_generator_func_exp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(617) : error A2006:undefined symbol : _llint_op_new_async_func
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(620) : error A2006:undefined symbol : _llint_op_new_async_func_exp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(623) : error A2006:undefined symbol : _llint_op_new_async_generator_func
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(626) : error A2006:undefined symbol : _llint_op_new_async_generator_func_exp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(629) : error A2006:undefined symbol : _llint_op_set_function_name
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(632) : error A2006:undefined symbol : _llint_op_neq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(635) : error A2006:undefined symbol : _llint_op_eq
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(641) : error A2006:undefined symbol : _llint_op_new_regexp
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(644) : error A2006:undefined symbol : _llint_op_spread
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(647) : error A2006:undefined symbol : _llint_op_new_array_with_spread
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(656) : error A2006:undefined symbol : _llint_op_ret
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(659) : error A2006:undefined symbol : _llint_op_strcat
> .../WebKitBuild/Release/JavaScriptCore/DerivedSources/LowLevelInterpreterWin.asm(662) : fatal error A1012:error count exceeds 100; stopping assembly
Comment 23 Saam Barati 2019-10-02 17:07:03 PDT 
Thanks for your help Ross. I think we've figured out what the Win issue was.
Comment 24 Saam Barati 2019-10-02 17:07:30 PDT 
Created attachment 380073 [details]
patch
Comment 25 Caio Lima 2019-10-03 06:05:39 PDT 
@Saam, even with EWS of ARMv7 green, this patch is crashing into this architecture. I think we can't trust in the EWS until we solve https://bugs.webkit.org/show_bug.cgi?id=202419. Do you mind adding those changes in your patch? https://github.com/caiolima/webkit/commit/5ae730134127d560010c88dc96b5c3f703de4b23
Comment 26 Ross Kirsling 2019-10-03 10:33:12 PDT 
It also seems like the latest patch is missing the ASM fix for WinCairo, right?
Comment 27 Saam Barati 2019-10-03 10:57:03 PDT 
(In reply to Ross Kirsling from comment #26)
> It also seems like the latest patch is missing the ASM fix for WinCairo,
> right?

I tried to keep the labels but just introduce an instruction between the two labels. Seems like that doesn't work. I'll do what we discussed on IRC yesterday
Comment 28 Saam Barati 2019-10-03 10:58:25 PDT 
(In reply to Caio Lima from comment #25)
> @Saam, even with EWS of ARMv7 green, this patch is crashing into this
> architecture. I think we can't trust in the EWS until we solve
> https://bugs.webkit.org/show_bug.cgi?id=202419. Do you mind adding those
> changes in your patch?
> https://github.com/caiolima/webkit/commit/
> 5ae730134127d560010c88dc96b5c3f703de4b23

Will do. Thanks
Comment 29 Ross Kirsling 2019-10-03 11:15:31 PDT 
(In reply to Saam Barati from comment #27)
> (In reply to Ross Kirsling from comment #26)
> > It also seems like the latest patch is missing the ASM fix for WinCairo,
> > right?
> 
> I tried to keep the labels but just introduce an instruction between the two
> labels. Seems like that doesn't work. I'll do what we discussed on IRC
> yesterday

Ahh, I do see the `call llint_crash` in the output...too bad that wasn't enough. :-/
Comment 30 Saam Barati 2019-10-03 16:34:28 PDT 
Created attachment 380168 [details]
patch
Comment 31 Tadeu Zagallo 2019-10-03 17:08:53 PDT 
Comment on attachment 380168 [details]
patch

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

Looks good to me. I mostly just have nits about many things still being called baseline, even when now referring to llint, and questions about code that I'm not entirely familiar with.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:375
>          ASSERT(baselineCodeBlock->jitType() == JITType::BaselineJIT);

How is this still correct?

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:810
> +            CodeBlock* baselineCodeBlockForCaller = baselineCodeBlockForOriginAndBaselineCodeBlock(*trueCaller, outermostBaselineCodeBlock);

`baselineCodeBlockForOriginAndBaselineCodeBlock` doesn't seem like an accurate name anymore

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:173
> +void* callerReturnPC(CodeBlock* baselineCodeBlockForCaller, unsigned callBytecodeIndex, InlineCallFrame::Kind trueCallerCallKind, bool& callerIsLLInt)

should `baselineCodeBlockForCaller` be called e.g. `codeBlockForCaller`?

> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:331
> +        if (callerIsLLInt) {

I'm not familiar with this code. Why is the exact same code duplicated in DFGOSRExit.cpp?

> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:40
> +#include "GetByIdMetadata.h"

Is this actually needed?

> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1738
> +    #.osrReturnPoint:

Not really relevant, but this is a bit weird... for a second I thought you had invented some crazy syntax for magically declaring the osr exit points
Comment 32 Saam Barati 2019-10-04 10:43:33 PDT 
Comment on attachment 380168 [details]
patch

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

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:375
>>          ASSERT(baselineCodeBlock->jitType() == JITType::BaselineJIT);
> 
> How is this still correct?

it can't be. There. are a couple asserts like this now that I'm grepping around. Will remove.

>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:810
>> +            CodeBlock* baselineCodeBlockForCaller = baselineCodeBlockForOriginAndBaselineCodeBlock(*trueCaller, outermostBaselineCodeBlock);
> 
> `baselineCodeBlockForOriginAndBaselineCodeBlock` doesn't seem like an accurate name anymore

I agree it's weird, but it's consistent with what we mean by "baseline" in this context. See:

    static bool isBaselineCode(JITType jitType)
    {
        return jitType == JITType::InterpreterThunk || jitType == JITType::BaselineJIT;
    }

That said, this function also has a bad assert. Will fix.

>> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:173
>> +void* callerReturnPC(CodeBlock* baselineCodeBlockForCaller, unsigned callBytecodeIndex, InlineCallFrame::Kind trueCallerCallKind, bool& callerIsLLInt)
> 
> should `baselineCodeBlockForCaller` be called e.g. `codeBlockForCaller`?

we really do require it be the "baseline" code block, so I think the name is accurate.

>> Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:331
>> +        if (callerIsLLInt) {
> 
> I'm not familiar with this code. Why is the exact same code duplicated in DFGOSRExit.cpp?

the one in DFGOSRExit.cpp is our probe based OSR exit interpreter. This here is the JIT based OSR exit compiler.

>> Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp:40
>> +#include "GetByIdMetadata.h"
> 
> Is this actually needed?

probably not. I had this before Keith pulled part of this patch into what he landed the other day. I'll remove it.

>> Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:1738
>> +    #.osrReturnPoint:
> 
> Not really relevant, but this is a bit weird... for a second I thought you had invented some crazy syntax for magically declaring the osr exit points

yea...

I would like this to just be a label, but it breaks the WinCairo port.

I'll just delete the comments.
Comment 33 Saam Barati 2019-10-04 10:45:12 PDT 
I'll run through debug tests before landing
Comment 34 Saam Barati 2019-10-04 10:47:43 PDT 
Comment on attachment 380168 [details]
patch

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

>>> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:375
>>>          ASSERT(baselineCodeBlock->jitType() == JITType::BaselineJIT);
>> 
>> How is this still correct?
> 
> it can't be. There. are a couple asserts like this now that I'm grepping around. Will remove.

I'm actually gonna change these types of asserts to be JITCode::isBaselineCode(...)
Comment 35 Saam Barati 2019-10-04 13:22:45 PDT 
Created attachment 380244 [details]
patch for landing
Comment 36 Saam Barati 2019-10-04 13:28:17 PDT 
Created attachment 380246 [details]
patch for landing
Comment 37 WebKit Commit Bot 2019-10-04 15:21:05 PDT 
Comment on attachment 380246 [details]
patch for landing

Clearing flags on attachment: 380246

Committed r250750: <https://trac.webkit.org/changeset/250750>
Comment 38 WebKit Commit Bot 2019-10-04 15:21:07 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 39 Radar WebKit Bug Importer 2019-10-04 15:22:23 PDT 
<rdar://problem/55997122>
Comment 40 Ross Kirsling 2019-10-04 19:04:14 PDT 
Damn, somehow this didn't work and I'm starting to wonder if the success I saw locally was a fluke somehow.

Looking at the before-and-after again and really considering the error messages that are appearing, the most relevant thing seems certainly like the drastic change in the position of `llint_entry ENDP`.

Namely, all of the undefined symbols that it's complaining about used to be between llint_entry's PROC PUBLIC and ENDP (because virtually the entire file was), but with the 45 procedures added in this patch, _llint_op_has_indexed_property now ends up inside op_put_by_val_return_location_wide_32 (as do a huge amount of others), _llint_op_construct_varargs ends up inside op_tail_call_forward_arguments_slow_return_location_wide_32, ...
Comment 41 Saam Barati 2019-10-06 15:12:20 PDT 
(In reply to Ross Kirsling from comment #40)
> Damn, somehow this didn't work and I'm starting to wonder if the success I
> saw locally was a fluke somehow.
> 
> Looking at the before-and-after again and really considering the error
> messages that are appearing, the most relevant thing seems certainly like
> the drastic change in the position of `llint_entry ENDP`.
> 
> Namely, all of the undefined symbols that it's complaining about used to be
> between llint_entry's PROC PUBLIC and ENDP (because virtually the entire
> file was), but with the 45 procedures added in this patch,
> _llint_op_has_indexed_property now ends up inside
> op_put_by_val_return_location_wide_32 (as do a huge amount of others),
> _llint_op_construct_varargs ends up inside
> op_tail_call_forward_arguments_slow_return_location_wide_32, ...

Maybe on windows we should just make all opcodes global labels? Can you try that to see if it will build?
Comment 42 Pablo Saavedra 2019-10-07 06:02:43 PDT 
I getting this error building a WebKit WPE for ARMv7 since this patch was integrated: https://bugs.webkit.org/show_bug.cgi?id=202629

I wonder if you could take a look on this or provide some guidelines to get a fix for it. Thanks in advance
Comment 43 Matt Lewis 2019-10-07 09:43:35 PDT 
This broke internal tests.
Comment 44 Matt Lewis 2019-10-07 09:47:36 PDT 
Reverted r250750 for reason:

Reverting change as this broke interal test over the weekend.

Committed r250775: <https://trac.webkit.org/changeset/250775>
Comment 45 Alexey Proskuryakov 2019-10-07 09:56:11 PDT 
See rdar://problem/56038859 for the breakage.

Also, there was a WPE build failure, bug 202629.
Comment 46 Caio Lima 2019-10-07 10:05:48 PDT 
@Saam the ARMv7 build failure was caused by changes I sent to you in https://github.com/caiolima/webkit/commit/5ae730134127d560010c88dc96b5c3f703de4b23. This Patch doesn't apply as fix anymore for current patch version, in this case I think you should remove it. We are investigating a work around. FYI, if we apply the Patch without those changes, we are able to build ARMv7, but we get Segmentation fault in a log of tests.
Comment 47 Ross Kirsling 2019-10-07 11:44:25 PDT 
(In reply to Saam Barati from comment #41)
> Maybe on windows we should just make all opcodes global labels? Can you try
> that to see if it will build?

This did not seem to work (more specifically, it helps with most of the cases, but if I add `global _llint_op_call_eval` it creates a proc called `llint_op_call_eval` instead)...

If I make the new ones non-global then the assembling works but I get linking errors instead, like:
> UnifiedSource-bfc896e1-11.cpp.obj : error LNK2019: unresolved external symbol op_call_return_location_narrow referenced in function "void * __cdecl JSC::DFG::callerReturnPC(class JSC::CodeBlock *,unsigned int,enum JSC::InlineCallFrame::Kind,bool &)" (?callerReturnPC@DFG@JSC@@YAPEAXPEAVCodeBlock@2@IW4Kind@InlineCallFrame@2@AEA_N@Z)

...which may not be surprising if they were global for a reason.
Comment 48 Saam Barati 2019-10-07 15:46:42 PDT 
Created attachment 380368 [details]
patch for landing
Comment 49 Saam Barati 2019-10-07 15:51:32 PDT 
(In reply to Saam Barati from comment #48)
> Created attachment 380368 [details]
> patch for landing

I think this should Just Work on Windows too
Comment 50 Caio Lima 2019-10-07 16:31:51 PDT 
Comment on attachment 380368 [details]
patch for landing

I tested this test locally and it breaks ARMv7 port.
Comment 51 WebKit Commit Bot 2019-10-07 16:34:08 PDT 
Comment on attachment 380368 [details]
patch for landing

Clearing flags on attachment: 380368

Committed r250806: <https://trac.webkit.org/changeset/250806>
Comment 52 WebKit Commit Bot 2019-10-07 16:34:11 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 53 Saam Barati 2019-10-09 18:02:44 PDT 
try to fix windows CLOOP in:
https://trac.webkit.org/changeset/250949/webkit