On big-endian platform, methodjit stricteq array element comparison always false (except -127)

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: t_mrc-ct, Assigned: t_mrc-ct)

Tracking

17 Branch
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; PPC Mac OS X 10.5; rv:17.0) Gecko/20130805 Firefox/17.0 TenFourFox/G5 (Nightly/Aurora)
Build ID: 20130805171245

Steps to reproduce:

On xpcshell with option -m, run following script:

let a=new Array(20), b=new Array(20);
for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
for(let i=0; i < a.length; i++) {a[i] === a[i];}
for(let i=0; i < b.length; i++) {b[i] === b[i];}
for(let i=0; i < b.length; i++) {a[i] === b[i];}
for(let i=0; i < b.length; i++) {b[i] === a[i];}


Actual results:

$ ./xpcshell -m
js> let a=new Array(20), b=new Array(20);
js> for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
-127
js> for(let i=0; i < a.length; i++) {a[i] === a[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === b[i];}
true
js> for(let i=0; i < b.length; i++) {a[i] === b[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === a[i];}
true


Expected results:

$ ./xpcshell -m
js> let a=new Array(20), b=new Array(20);
js> for(let i=0; i < a.length; i++) {a[i] = i; b[i] = -127;}
-127
js> for(let i=0; i < a.length; i++) {a[i] === a[i];}
true
js> for(let i=0; i < b.length; i++) {b[i] === b[i];}
true
js> for(let i=0; i < b.length; i++) {a[i] === b[i];}
false
js> for(let i=0; i < b.length; i++) {b[i] === a[i];}
false
(Assignee)

Comment 1

4 years ago
Created attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

On little-endian(32bit), jsval_layout defined as {payload; tag;}.
On big-endian(32bit) ,   jsval_layout defined as {tag; payload;}.

On little-endian platform, accessing address without masm.payloadOf() does not cause bug because there is payload data.
But on big-endian, there is tag data (tag of int32 is -127). So stricteq compares array element to -127.
Attachment #792813 - Flags: review?(jdemooij)
JaegerMonkey (methodjit directory) does not exists anymore, not even in release builds, except in b2g18 and in esr17.

What branch are you working on?
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> What branch are you working on?

(In reply to t_mrc-ct from comment #0)
> User Agent: Mozilla/5.0 (Macintosh; PPC Mac OS X 10.5; rv:17.0)
> Gecko/20130805 Firefox/17.0 TenFourFox/G5 (Nightly/Aurora)
Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

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

Looks good!
Attachment #792813 - Flags: review?(jdemooij) → review+

Updated

4 years ago
Assignee: general → t_mrc-ct
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fixes a JIT bug on PowerPC processors.

> User impact if declined: 
Broken websites on PowerPC hardware.

> Fix Landed on Version:
N/A, JaegerMonkey is disabled in Firefox 23 and has been removed in Firefox 24.

> Risk to taking this patch (and alternatives if risky): 
Low risk. Should have no effect on x86/x64/ARM platforms.

> String or UUID changes made by this patch: 
None.
Attachment #792813 - Flags: approval-mozilla-esr17?
CC'ing Cameron from TenFourFox.
Yes, that looks correct. Thanks, t_mrc-ct!
Comment on attachment 792813 [details] [diff] [review]
patch for methodjit stricteq bug

This does not meet ESR landing criteria (https://wiki.mozilla.org/Release_Management/ESR_Landing_Process) and if this is a case of something being disabled in FF24, we'll be putting out our first ESR24 on this coming cycle so that will be a viable workaround for any orgs impacted by this on the ESR17 branch - migrating to ESR24 is enforced after two release cycles so this will be moot in 2 more cycles regardless.
Attachment #792813 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17-
OK, marking this WONTFIX then per comment 8.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
We'll adopt this at the local level then. t_mrc-ct, this will be in the 17.0.9 sets; see http://code.google.com/p/tenfourfox/issues/detail?id=239
You need to log in before you can comment on or make changes to this bug.