Last Comment Bug 754641 - Fix 32-bit --disable-methodjit builds after bug 739512
: Fix 32-bit --disable-methodjit builds after bug 739512
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: Other OpenBSD
: -- normal (vote)
: mozilla15
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
: 757558 (view as bug list)
Depends on:
Blocks: 739512 816250
  Show dependency treegraph
 
Reported: 2012-05-13 00:18 PDT by Landry Breuil (:gaston)
Modified: 2012-11-28 13:56 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Readd pad32 on 32 bits archs (976 bytes, patch)
2012-05-13 00:20 PDT, Landry Breuil (:gaston)
n.nethercote: review-
Details | Diff | Review
patch to print offsets (2.65 KB, patch)
2012-05-17 16:39 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
patch (770 bytes, patch)
2012-05-21 16:29 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
luke: review+
Details | Diff | Review

Description Landry Breuil (:gaston) 2012-05-13 00:18:32 PDT
Bug 739512, patch 3 removed 
+#if JS_BITS_PER_WORD == 32
+private:
+   uint32_t        pad32;
+#endif
+
(which got added in patch 9...)
This breaks the build on powerpc here with js/src/jsscript.h:937: error: size of array 'arg' is negative

Readding that chunk fixes it.
Comment 1 Landry Breuil (:gaston) 2012-05-13 00:20:02 PDT
Created attachment 623479 [details] [diff] [review]
Readd pad32 on 32 bits archs
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-13 16:13:20 PDT
Comment on attachment 623479 [details] [diff] [review]
Readd pad32 on 32 bits archs

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

This breaks x86-32 builds, both debug and opt, and probably does likewise for ARM builds.

What compiler are you using?  Presumably it's laying out the struct differently, which is surprising.  I don't have access to a ppc32 machine -- do you have access to an x86-32 machine?  We need to properly understand the difference in struct layout in order to fix this properly.
Comment 3 Landry Breuil (:gaston) 2012-05-13 22:02:02 PDT
Then why was the chunk added then removed in bug 739512 ? And how do you want to display the struct layout ?

Target: powerpc-unknown-openbsd5.1
Configured with: OpenBSD/powerpc system compiler
Thread model: posix
gcc version 4.2.1 20070719
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-13 22:47:41 PDT
> Then why was the chunk added then removed in bug 739512 ?

sizeof(JSScript) must be a multiple of 8.  In bug 739512 I did several patches that removed fields from JSScript, gradually shrinking it.  At different times I had to add and remove the padding.

> And how do you
> want to display the struct layout ?

Any way that makes it clear which field(s) are taking up a different size.  E.g. printing the offset of every field would do it.
Comment 5 Landry Breuil (:gaston) 2012-05-14 01:38:38 PDT
(In reply to Nicholas Nethercote [:njn] from comment #4)
> > And how do you
> > want to display the struct layout ?
> 
> Any way that makes it clear which field(s) are taking up a different size. 
> E.g. printing the offset of every field would do it.

Can you provide a code snippet for that ? I'll glady test it on various archs..
Comment 6 Landry Breuil (:gaston) 2012-05-17 05:28:04 PDT
I've tried the obvious #include "jsscript.h" int main() { JSScript j;} but of course it needs way more than that to build a simple testcase..
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-17 16:39:04 PDT
Created attachment 624953 [details] [diff] [review]
patch to print offsets

Sorry for the delay, this bug slipped off my radar.  

The attached patch prints the offsets.  (I guess you'll need to comment out the static assertion as well.)  I get this output on an optimized 32-bit build:

sizeof(JSScript): 104
                 bindings: 0
                     code: 12
                     data: 16
                 filename: 20
                    atoms: 24
               principals: 28
         originPrincipals: 32
             globalObject: 36
                    types: 40
          jitHandleNormal: 44
 jitHandleNormalBarriered: 48
            jitHandleCtor: 52
   jitHandleCtorBarriered: 56
                function_: 60
                   length: 64
                   lineno: 68
               mainOffset: 72
                   natoms: 76
                 useCount: 80
                  version: 84
                   nfixed: 86
                nTypeSets: 88
                   nslots: 90
              staticLevel: 92
                argsSlot_: 94
             hasArrayBits: 96
Comment 8 Landry Breuil (:gaston) 2012-05-18 02:39:27 PDT
[11:26] mikey:/usr/obj/m-c/js/src/ $LD_LIBRARY_PATH=../../dist/bin/ ./shell/js                                                
js> 1+1
sizeof(JSScript): 84
                 bindings: 0
                     code: 12
                     data: 16
                 filename: 20
                    atoms: 24
               principals: 28
         originPrincipals: 32
             globalObject: 36
                    types: 40
                function_: 44
                   length: 48
                   lineno: 52
               mainOffset: 56
                   natoms: 60
                 useCount: 64
                  version: 68
                   nfixed: 70
                nTypeSets: 72
                   nslots: 74
              staticLevel: 76
                argsSlot_: 78
             hasArrayBits: 80
2

16 of the 20 missing bytes seems to account for the 4 jit* members in #if JS_METHODJIT part.., and 4 bytes for the 2 bools within #if JS_METHODJIT ? That's with disable-debug enable-optimize.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-18 02:58:04 PDT
Ah:  there are 25 1-bit fields at the end, but 2 of them are only with JS_METHODJIT.  So I suspect I broke non-methodjit builds.  Adding JS_METHODJIT-only padding should fix it.  It's Friday night here, but I can take a deeper look on Monday.
Comment 10 Landry Breuil (:gaston) 2012-05-18 03:01:43 PDT
(In reply to Nicholas Nethercote [:njn] from comment #9)
> Ah:  there are 25 1-bit fields at the end, but 2 of them are only with
> JS_METHODJIT.  So I suspect I broke non-methodjit builds.  Adding
> JS_METHODJIT-only padding should fix it.  

Shouldn't it be the other way round, ie add non-methodjit only padding ?
Comment 11 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-18 03:06:19 PDT
> Shouldn't it be the other way round, ie add non-methodjit only padding ?

Yes!  My mistake.
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-21 16:29:07 PDT
Created attachment 625816 [details] [diff] [review]
patch

I tested this on all eight combinations of the following: opt vs. debug, 32-bit vs 64-bit, mjit vs no-mjit.

Landry, can you try this on PPC just to be extra sure?  Thanks.
Comment 13 Landry Breuil (:gaston) 2012-05-22 01:41:28 PDT
mikey:/usr/obj/m-c/js/src/ $LD_LIBRARY_PATH=../../dist/bin/ ./shell/js
js> 1+1
sizeof(JSScript): 88
                 bindings: 0
                     code: 12
                     data: 16
                 filename: 20
                    atoms: 24
               principals: 28
         originPrincipals: 32
             globalObject: 36
                    types: 40
                function_: 44
                   length: 48
                   lineno: 52
               mainOffset: 56
                   natoms: 60
                 useCount: 64
                  version: 72
                   nfixed: 74
                nTypeSets: 76
                   nslots: 78
              staticLevel: 80
                argsSlot_: 82
             hasArrayBits: 84
2

Seems to be okay on PPC, thanks!
Comment 14 Steve Fink [:sfink] [:s:] 2012-05-22 12:00:15 PDT
*** Bug 757558 has been marked as a duplicate of this bug. ***
Comment 15 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-05-22 19:01:58 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/021554387271
Comment 16 Ed Morley [:emorley] 2012-05-23 04:51:13 PDT
https://hg.mozilla.org/mozilla-central/rev/021554387271
Comment 17 Landry Breuil (:gaston) 2012-08-29 01:29:06 PDT
Sorry to bring bad news again... but one of the recent commits to jsscript broke ppc/--disable-methodjit again :

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
Comment 18 Landry Breuil (:gaston) 2012-08-29 01:33:34 PDT
Filed bug 786588 instead, per glandium's request over irc.

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