754641 - Fix 32-bit --disable-methodjit builds after bug 739512

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Landry Breuil (:gaston)]

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)]

Attachment: Readd pad32 on 32 bits archs

Comment 2

[Nicholas Nethercote [inactive]]

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)]

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 [inactive]]

> 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)]

(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)]

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 [inactive]]

Attachment: 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)]

[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 [inactive]]

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)]

(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 [inactive]]

> Shouldn't it be the other way round, ie add non-methodjit only padding ?

Yes!  My mistake.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: 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)]

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 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/021554387271

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/021554387271

Comment 17

[Landry Breuil (:gaston)]

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)]

Filed bug 786588 instead, per glandium's request over irc.