Last Comment Bug 786588 - Fix 32-bit --disable-methodjit builds after a recent jsscript.h commit
: Fix 32-bit --disable-methodjit builds after a recent jsscript.h commit
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: PowerPC OpenBSD
: -- normal (vote)
: mozilla18
Assigned To: Landry Breuil (:gaston)
:
Mentors:
Depends on:
Blocks: 816250
  Show dependency treegraph
 
Reported: 2012-08-29 01:32 PDT by Landry Breuil (:gaston)
Modified: 2012-11-28 13:43 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed
fixed


Attachments
dump jsscript offsets (3.67 KB, patch)
2012-08-29 15:11 PDT, Landry Breuil (:gaston)
no flags Details | Diff | Splinter Review
Readd 32 bits padding for 32 bits archs non - methodjit (929 bytes, patch)
2012-08-30 22:21 PDT, Landry Breuil (:gaston)
n.nethercote: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Landry Breuil (:gaston) 2012-08-29 01:32:54 PDT
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.
Comment 1 Landry Breuil (:gaston) 2012-08-29 15:09:33 PDT
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
Comment 2 Landry Breuil (:gaston) 2012-08-29 15:11:01 PDT
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.
Comment 3 Landry Breuil (:gaston) 2012-08-30 02:36:39 PDT
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 ?
Comment 4 Landry Breuil (:gaston) 2012-08-30 03:59:37 PDT
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.
Comment 5 Landry Breuil (:gaston) 2012-08-30 04:00:18 PDT
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
Comment 6 Landry Breuil (:gaston) 2012-08-30 22:21:57 PDT
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.
Comment 7 Nicholas Nethercote [:njn] 2012-08-30 22:39:43 PDT
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?
Comment 8 Landry Breuil (:gaston) 2012-08-31 00:49:38 PDT
(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.
Comment 9 Landry Breuil (:gaston) 2012-09-01 00:10:29 PDT
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
Comment 10 Landry Breuil (:gaston) 2012-09-01 00:24:27 PDT
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
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-09-01 05:49:21 PDT
https://hg.mozilla.org/mozilla-central/rev/43e3314f1da8
Comment 12 Landry Breuil (:gaston) 2012-09-03 03:18:44 PDT
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
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-04 16:49:09 PDT
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.
Comment 14 Landry Breuil (:gaston) 2012-09-05 00:06:35 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/aca07b596c6f

cloning a beta tree to push to it unless someone beats me to it..
Comment 15 Landry Breuil (:gaston) 2012-09-05 01:12:37 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/8ceb8dd4f1cb

Note You need to log in before you can comment on or make changes to this bug.