IonMonkey: (ARM) ARMv6 has some issues with baseline

RESOLVED FIXED in Firefox 23

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mjrosenb, Unassigned)

Tracking

(Blocks 1 bug, {crash, regression, topcrash})

23 Branch
mozilla23
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 unaffected, firefox23 fixed)

Details

(Whiteboard: [native-crash][startupcrash][ARMv6][fuzzblocker], crash signature)

Attachments

(1 attachment)

Reporter

Description

6 years ago
The baseline compiler currently explicitly uses MOVW/MOVT, which is problematic because armv6 doesn't have these instructions.  Due to a minor clerical error on my part, most, if not all armv7 builds think they are armv6 for purposes of asserting if they can generate movw/movt.
STR
echo 'print("hello world");' | ./js --ion-eager

Updated

6 years ago
Severity: normal → critical
Keywords: regression
OS: Linux → Android
Whiteboard: [native-crash][startupcrash][ARMv6]
Version: unspecified → 23 Branch
Where does baseline explicitly reference movw/movt instructions?  IIRC the MacroAssembler is used for everything, which should abstract over that..
This bug is the #1 topcrash for Nightly 23 on Android.
Keywords: topcrash

Updated

6 years ago
tracking-fennec: --- → ?
Reporter

Comment 4

6 years ago
This doesn't fix BC completely on ARM, expect another bug shortly.
Attachment #733821 - Flags: review?(Jacob.Bramley)
Whiteboard: [native-crash][startupcrash][ARMv6] → [native-crash][startupcrash][ARMv6][fuzzblocker]
Comment on attachment 733821 [details] [diff] [review]
/home/mjrosenb/patches/fixBC_ARMv6-r0.patch

Review of attachment 733821 [details] [diff] [review]:
-----------------------------------------------------------------

Minor changes only, as discussed.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +3268,2 @@
>      addPendingJump(bo, target->raw(), Relocation::IONCODE);
> +    ma_movPatchable(Imm32(uint32_t(target->raw())), ScratchRegister, Always, L_LDR);

As discussed, this should use MOVW/T on ARMv7.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +563,5 @@
>      // this instruction.
>      CodeOffsetLabel toggledCall(IonCode *target, bool enabled);
>  
>      static size_t ToggledCallSize() {
>          // Size of a movw, movt, nop/blx instruction.

The comment needs to be updated to match.

@@ +564,5 @@
>      CodeOffsetLabel toggledCall(IonCode *target, bool enabled);
>  
>      static size_t ToggledCallSize() {
>          // Size of a movw, movt, nop/blx instruction.
> +        return 8;

As discussed, this size will change for ARMv7, where MOVW/MOVT is used.
Attachment #733821 - Flags: review?(Jacob.Bramley) → review+
https://hg.mozilla.org/mozilla-central/rev/6f83b4eedf82
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.