Post bug 197993 (r250806), JSC 32bit started failing with many segmentation faults. Debugging the segfault we see that we are hitting addresses that shouldn't be reachable. 0x2582ee <llint_op_call+314> blx r0 >│0x2582f0 <llint_op_call+316> eorseq lr, r4, r8, lsr sp 0x2582f4 <llint_op_call+320> andeq r2, r0, r8, rrx 0x2582f8 <llint_op_call+324> ldr r2, [r7, #8] LLIntAssembly.h looks like: OFFLINE_ASM_LOCAL_LABEL(_offlineasm_callOp__commonCallOp__llintOpWithMetadata__llintOpWithRe turn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__fn__slowPathForCall__callCallSlowPath__51 1_action__dontUpdateSP) "\tmovw r10, #55459\n" // /home/pmatos/dev/igalia/WebKit /Source/JavaScriptCore/llint/LowLevelInterpreter.asm:256 "\tblx r0\n" // /home/pmatos/dev/igalia/WebKit/Source/JavaScriptCore/llint/LowLevelInterpreter.asm:952 "\t" LOCAL_LABEL_STRING(offlineasm_arm_got_1020) ":\n" "\t.word _GLOBAL_OFFSET_TABLE_-(" LOCAL_LABEL_STRING(offlineasm_arm_got_offset_1020) "+4)\n" "\t.word " LOCAL_REFERENCE(g_opcodeMap) "(GOT)\n" OFFLINE_ASM_GLUE_LABEL(op_construct_slow_return_location_wide32) "\tldr r2, [r7, #8]\n"
Created attachment 380735 [details] Patch
Comment on attachment 380735 [details] Patch Attachment 380735 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13121117 New failing tests: editing/style/iframe-onload-crash-mac.html editing/style/apply-style-iframe-crash.html
Created attachment 380781 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 380865 [details] Patch
Created attachment 380878 [details] Patch
Comment on attachment 380878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380878&action=review > Source/JavaScriptCore/ChangeLog:8 > + Do not allow instruction execution to reach OSR return label on ARMv7 or MIPS. Why is this needed? The changelog should also say “why”, not just “what”
Comment on attachment 380878 [details] Patch Please put the information in here that you emailed me for explanation
Created attachment 380975 [details] Patch
(In reply to Saam Barati from comment #6) > Comment on attachment 380878 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380878&action=review > > > Source/JavaScriptCore/ChangeLog:8 > > + Do not allow instruction execution to reach OSR return label on ARMv7 or MIPS. > > Why is this needed? The changelog should also say “why”, not just “what” Thanks for your time reviewing this. Improved changelog.
Created attachment 380998 [details] Patch
(In reply to Paulo Matos from comment #10) > Created attachment 380998 [details] > Patch Turns out the fix is not necessary for MIPS. Applying to ARMv7 only.
Comment on attachment 380998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380998&action=review > Source/JavaScriptCore/llint/LowLevelInterpreter.asm:967 > defineOSRExitReturnLabel(opcodeName, size) so returning here is still OK?
Comment on attachment 380998 [details] Patch Clearing flags on attachment: 380998 Committed r251196: <https://trac.webkit.org/changeset/251196>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56343045>
Comment on attachment 380998 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380998&action=review >> Source/JavaScriptCore/llint/LowLevelInterpreter.asm:967 >> defineOSRExitReturnLabel(opcodeName, size) > > so returning here is still OK? Yes. The problem with ARMv7 is that before “return_location” labels, we emit a sort of “constant pool”. Since it was in the middle of callTargetFunction code path, it turned out to be an invalid instruction whenever we had execution of this part of the code (this means that we crashed even when JIT was disabled). This constant pool is still emmited, but when we jump to return_location points now, there is no invalid instruction during the code path.
Thank you very much for the review!