Fix 32-bit --disable-methodjit builds after a recent jsscript.h commit

RESOLVED FIXED in Firefox 16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: gaston)

Tracking

unspecified
mozilla18
PowerPC
OpenBSD
Points:
---

Firefox Tracking Flags

(firefox15 unaffected, firefox16 fixed, firefox17 fixed, firefox18 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Filing a new bug per glandium's request, similar to #754641.

A recent commit to jsscript changed the struct size, thus breaking --disable-methodjit (ie, powerpc in my case) builds :

gcc 4.6 :
js/src/jsscript.h:845:1: error: size of array 'moz_static_assert95' is negative 

gcc 4.2 :
js/src/jsscript.h:845: error: size of array 'arg' is negative

Will try the c snippet and look in hg log to see which struct member was added/removed.
(Assignee)

Comment 1

5 years ago
With a somewhat different updated version of the script in bug #754641 (going to attach it) here's what i get for struct members offsets on ppc/32 bits.

sizeof(JSScript): 100
                 bindings: 0
                     code: 12
                     data: 16
                 filename: 20
                    atoms: 24
               principals: 28
         originPrincipals: 32
                    types: 36
                function_: 44
                   length: 52
                   lineno: 56
               mainOffset: 60
                   natoms: 64
                 useCount: 76
                  version: 84
                   nfixed: 86
                nTypeSets: 88
                   nslots: 90
              staticLevel: 92
             hasArrayBits: 94
(Assignee)

Comment 2

5 years ago
Created attachment 656631 [details] [diff] [review]
dump jsscript offsets

Here's the diff to print offsets. Maybe some of the members could be added.. will have a closer look tmrw to find where padding is needed.
(Assignee)

Comment 3

5 years ago
So recently in struct jsscript :
- hasFreezeConstraints bool was added in bug 778724
- callDestroyHook bool was removed in Bug 756267
- funHasAnyAliasedFormal bool was added in Bug 767013

And that's only for august.. none of them seem to be methodjit-only.
Interestingly the pad32 added (for non-methodjit only) to fix #754641 in https://hg.mozilla.org/mozilla-central/rev/021554387271 was changed to be 32-bit only (mj and !mj) padding, removed in Bug 753145 and added in Bug 767750. As of now there's only debug-only padding.. so i wonder if the !mj padding shouldnt come back.

Can anyone familiar in this area help me pinpoint where the issue is ?
(Assignee)

Comment 4

5 years ago
It seems readding :

+#if !defined(JS_METHODJIT) && JS_BITS_PER_WORD == 32
+    uint32_t        pad32;
+#endif

doesnt trigger the assert anymore on ppc. Checking if aurora is affected too.
(Assignee)

Comment 5

5 years ago
with that chunk readded here's what i get for offsets :


$LD_LIBRARY_PATH=../../dist/bin/ shell/js                                                  
sizeof(JSScript): 104
                 bindings: 0
                     code: 12
                     data: 16
                 filename: 20
                    atoms: 24
               principals: 28
         originPrincipals: 32
                    types: 36
                function_: 44
                   length: 52
                   lineno: 56
               mainOffset: 60
                   natoms: 64
                 useCount: 76
                  version: 88
                   nfixed: 90
                nTypeSets: 92
                   nslots: 94
              staticLevel: 96
             hasArrayBits: 98
(Assignee)

Comment 6

5 years ago
Created attachment 657153 [details] [diff] [review]
Readd 32 bits padding for 32 bits archs non - methodjit

This produces a working shell and browser on OpenBSD/ppc.
Assignee: general → landry
Attachment #657153 - Flags: review?(n.nethercote)
Comment on attachment 657153 [details] [diff] [review]
Readd 32 bits padding for 32 bits archs non - methodjit

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

Looks reasonable.  Have you tried all eight combinations of:  methodjit vs. nomethodjit, 32-bit vs 64-bit, opt vs. debug?
Attachment #657153 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 8

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Comment on attachment 657153 [details] [diff] [review]
> Readd 32 bits padding for 32 bits archs non - methodjit
> 
> Review of attachment 657153 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable.  Have you tried all eight combinations of:  methodjit vs.
> nomethodjit, 32-bit vs 64-bit, opt vs. debug?

https://tbpl.mozilla.org/?tree=Try&rev=021b0b24336f for a try run.

I'm also trying a debug non-methodjit build of the js shell on my ppc.
(Assignee)

Comment 9

5 years ago
All green on try after a bunch of retriggers. A debug-enabled js shell also works on my mac mini.
https://hg.mozilla.org/integration/mozilla-inbound/rev/43e3314f1da8
(Assignee)

Comment 10

5 years ago
Comment on attachment 657153 [details] [diff] [review]
Readd 32 bits padding for 32 bits archs non - methodjit

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

one of the recent commits to jsscript.h before last uplift

User impact if declined: 

aurora doesnt build on powerpc (non-methodjit 32 bits) :

/home/landry/src/mozilla-aurora/js/src/jsscript.h:899:1: error: size of array 'moz_static_assert91' is negative
$uname -a
OpenBSD mikey.home.rhaalovely.net 5.2 GENERIC#246 macppc

Testing completed (on m-c, etc.): 

https://tbpl.mozilla.org/?tree=Try&rev=021b0b24336f
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=43e3314f1da8

Risk to taking this patch (and alternatives if risky):

Break subtly other non-methodjit platforms ? doubtful, and noone works on them anyway.
 
String or UUID changes made by this patch: 

none
Attachment #657153 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/43e3314f1da8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 12

5 years ago
Comment on attachment 657153 [details] [diff] [review]
Readd 32 bits padding for 32 bits archs non - methodjit

And it also affects beta :

/usr/ports/pobj/firefox-16.0beta1/mozilla-beta/js/src/jsscript.h:899: error: size of array 'arg' is negative


[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

one of the recent commits to jsscript.h before last uplift

User impact if declined: 

aurora doesnt build on powerpc (non-methodjit 32 bits) :

/usr/ports/pobj/firefox-16.0beta1/mozilla-beta/js/src/jsscript.h:899: error: size of array 'arg' is negative

$uname -a
OpenBSD mikey.home.rhaalovely.net 5.2 GENERIC#246 macppc

Testing completed (on m-c, etc.): 

https://tbpl.mozilla.org/?tree=Try&rev=021b0b24336f
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=43e3314f1da8

Risk to taking this patch (and alternatives if risky):

Break subtly other non-methodjit platforms ? doubtful, and noone works on them anyway.
 
String or UUID changes made by this patch: 

none
Attachment #657153 - Flags: approval-mozilla-beta?
Comment on attachment 657153 [details] [diff] [review]
Readd 32 bits padding for 32 bits archs non - methodjit

Early enough in beta, and this shouldn't impact anything else, approving.
Attachment #657153 - Flags: approval-mozilla-beta?
Attachment #657153 - Flags: approval-mozilla-beta+
Attachment #657153 - Flags: approval-mozilla-aurora?
Attachment #657153 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/aca07b596c6f

cloning a beta tree to push to it unless someone beats me to it..
status-firefox15: --- → unaffected
status-firefox16: --- → affected
status-firefox17: --- → fixed
status-firefox18: --- → fixed
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/8ceb8dd4f1cb
status-firefox16: affected → fixed
(Assignee)

Updated

5 years ago
Blocks: 816250
You need to log in before you can comment on or make changes to this bug.