Closed Bug 754641 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other
OpenBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: gaston, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Readd pad32 on 32 bits archs (obsolete) — Splinter Review
Assignee: general → landry
Attachment #623479 - Flags: review?(n.nethercote)
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.
Attachment #623479 - Flags: review?(n.nethercote) → review-
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
> 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.
(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..
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..
Attached patch patch to print offsets (obsolete) — Splinter Review
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
[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.
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.
(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 ?
> Shouldn't it be the other way round, ie add non-methodjit only padding ?

Yes!  My mistake.
Blocks: 739512
Summary: Fix build on powerpc after Bug 739512 → Fix 32-bit --disable-methodjit builds after bug 739512
Attached patch patchSplinter Review
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.
Assignee: landry → n.nethercote
Attachment #623479 - Attachment is obsolete: true
Attachment #624953 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #625816 - Flags: review?(luke)
Attachment #625816 - Flags: review?(luke) → review+
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!
Duplicate of this bug: 757558
Whiteboard: [js:t]
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/021554387271
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Filed bug 786588 instead, per glandium's request over irc.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Blocks: 816250
You need to log in before you can comment on or make changes to this bug.