Bug 197993 – Allow OSR exit to the LLInt

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

[Saam Barati]

Seems like a fun Friday patch. Let's see how far I get.

[Saam Barati]

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

[Saam Barati]

(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

[Saam Barati]

Created attachment 370227 [details]
patch

[Saam Barati]

Created attachment 370228 [details]
patch

rebased

[EWS Watchlist]

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.

[Saam Barati]

Created attachment 370229 [details]
patch

try to fix 32-bit

[Saam Barati]

Created attachment 370237 [details]
patch

Try to fix mips/armv7

[Saam Barati]

Created attachment 370238 [details]
patch

fix armv7+mips take 2

[Tadeu Zagallo]

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)

[Michael Saboff]

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*

[Saam Barati]

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.

[Saam Barati]

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?

[Saam Barati]

Comment on attachment 370238 [details]
patch

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

> Tools/ChangeLog:17
> [phone]  Saam barati  <[e-mail]>
> +
> +        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.

[Tadeu Zagallo]

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

[Saam Barati]

Trying to resolve windows build issue now

[Ross Kirsling]

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`.

[Caio Lima]

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`.

[Saam Barati]

gonna pick this back up

[Saam Barati]

Created attachment 379977 [details]
WIP

[Saam Barati]

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.

[Ross Kirsling]

Created attachment 380065 [details]
WinCairo ASM from trunk build

[Ross Kirsling]

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

[Saam Barati]

Thanks for your help Ross. I think we've figured out what the Win issue was.

[Saam Barati]

Created attachment 380073 [details]
patch

[Caio Lima]

@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

[Ross Kirsling]

It also seems like the latest patch is missing the ASM fix for WinCairo, right?

[Saam Barati]

(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

[Saam Barati]

(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

[Ross Kirsling]

(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. :-/

[Saam Barati]

Created attachment 380168 [details]
patch

[Tadeu Zagallo]

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

[Saam Barati]

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.

[Saam Barati]

I'll run through debug tests before landing

[Saam Barati]

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(...)

[Saam Barati]

Created attachment 380244 [details]
patch for landing

[Saam Barati]

Created attachment 380246 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 380246 [details]
patch for landing

Clearing flags on attachment: 380246

Committed r250750: <https://trac.webkit.org/changeset/250750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55997122>

[Ross Kirsling]

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, ...

[Saam Barati]

(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?

[Pablo Saavedra]

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

[Matt Lewis]

This broke internal tests.

[Matt Lewis]

Reverted r250750 for reason:

Reverting change as this broke interal test over the weekend.

Committed r250775: <https://trac.webkit.org/changeset/250775>

[Alexey Proskuryakov]

See rdar://problem/56038859 for the breakage.

Also, there was a WPE build failure, bug 202629.

[Caio Lima]

@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.

[Ross Kirsling]

(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.

[Saam Barati]

Created attachment 380368 [details]
patch for landing

[Saam Barati]

(In reply to Saam Barati from comment #48)
> Created attachment 380368 [details]
> patch for landing

I think this should Just Work on Windows too

[Caio Lima]

Comment on attachment 380368 [details]
patch for landing

I tested this test locally and it breaks ARMv7 port.

[WebKit Commit Bot]

Comment on attachment 380368 [details]
patch for landing

Clearing flags on attachment: 380368

Committed r250806: <https://trac.webkit.org/changeset/250806>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

try to fix windows CLOOP in:
https://trac.webkit.org/changeset/250949/webkit