Bug 202844 - Invalid instruction generated for ARM_THUMB2 in llint
Summary: Invalid instruction generated for ARM_THUMB2 in 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: Paulo Matos
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-11 02:05 PDT by Paulo Matos
Modified: 2019-10-16 15:14 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.43 KB, patch)
2019-10-11 02:12 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (14.46 MB, application/zip)
2019-10-11 13:52 PDT, EWS Watchlist
no flags Details
Patch (1.62 KB, patch)
2019-10-14 03:16 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (1.64 KB, patch)
2019-10-14 05:23 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2019-10-15 02:34 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff
Patch (1.99 KB, patch)
2019-10-15 10:20 PDT, Paulo Matos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paulo Matos 2019-10-11 02:05:09 PDT
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"
Comment 1 Paulo Matos 2019-10-11 02:12:53 PDT
Created attachment 380735 [details]
Patch
Comment 2 EWS Watchlist 2019-10-11 13:52:10 PDT
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
Comment 3 EWS Watchlist 2019-10-11 13:52:12 PDT
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
Comment 4 Paulo Matos 2019-10-14 03:16:00 PDT
Created attachment 380865 [details]
Patch
Comment 5 Paulo Matos 2019-10-14 05:23:13 PDT
Created attachment 380878 [details]
Patch
Comment 6 Saam Barati 2019-10-14 08:27:59 PDT
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 7 Saam Barati 2019-10-14 14:54:37 PDT
Comment on attachment 380878 [details]
Patch

Please put the information in here that you emailed me for explanation
Comment 8 Paulo Matos 2019-10-15 02:34:34 PDT
Created attachment 380975 [details]
Patch
Comment 9 Paulo Matos 2019-10-15 02:35:28 PDT
(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.
Comment 10 Paulo Matos 2019-10-15 10:20:27 PDT
Created attachment 380998 [details]
Patch
Comment 11 Paulo Matos 2019-10-15 10:21:13 PDT
(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 12 Saam Barati 2019-10-16 12:05:52 PDT
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 13 WebKit Commit Bot 2019-10-16 12:50:30 PDT
Comment on attachment 380998 [details]
Patch

Clearing flags on attachment: 380998

Committed r251196: <https://trac.webkit.org/changeset/251196>
Comment 14 WebKit Commit Bot 2019-10-16 12:50:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-10-16 12:51:30 PDT
<rdar://problem/56343045>
Comment 16 Caio Lima 2019-10-16 15:14:03 PDT
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.
Comment 17 Caio Lima 2019-10-16 15:14:37 PDT
Thank you very much for the review!