Bug 202844 – Invalid instruction generated for ARM_THUMB2 in llint

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Paulo Matos]

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"

[Paulo Matos]

Created attachment 380735 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Paulo Matos]

Created attachment 380865 [details]
Patch

[Paulo Matos]

Created attachment 380878 [details]
Patch

[Saam Barati]

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”

[Saam Barati]

Comment on attachment 380878 [details]
Patch

Please put the information in here that you emailed me for explanation

[Paulo Matos]

Created attachment 380975 [details]
Patch

[Paulo Matos]

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

[Paulo Matos]

Created attachment 380998 [details]
Patch

[Paulo Matos]

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

[Saam Barati]

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?

[WebKit Commit Bot]

Comment on attachment 380998 [details]
Patch

Clearing flags on attachment: 380998

Committed r251196: <https://trac.webkit.org/changeset/251196>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56343045>

[Caio Lima]

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.

[Caio Lima]

Thank you very much for the review!