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

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gaston, Assigned: njn)

Tracking

Trunk
mozilla15
Other
OpenBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 623479 [details] [diff] [review]
Readd pad32 on 32 bits archs
Assignee: general → landry
Attachment #623479 - Flags: review?(n.nethercote)
(Assignee)

Comment 2

5 years ago
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-
(Reporter)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
> 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.
(Reporter)

Comment 5

5 years ago
(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..
(Reporter)

Comment 6

5 years ago
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..
(Assignee)

Comment 7

5 years ago
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
(Reporter)

Comment 8

5 years ago
[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.
(Assignee)

Comment 9

5 years ago
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.
(Reporter)

Comment 10

5 years ago
(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 ?
(Assignee)

Comment 11

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

Yes!  My mistake.
(Assignee)

Updated

5 years ago
Blocks: 739512
Summary: Fix build on powerpc after Bug 739512 → Fix 32-bit --disable-methodjit builds after bug 739512
(Assignee)

Comment 12

5 years ago
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.
Assignee: landry → n.nethercote
Attachment #623479 - Attachment is obsolete: true
Attachment #624953 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #625816 - Flags: review?(luke)

Updated

5 years ago
Attachment #625816 - Flags: review?(luke) → review+
(Reporter)

Comment 13

5 years ago
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]
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/021554387271
(Assignee)

Updated

5 years ago
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/021554387271
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 17

5 years ago
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 → ---
(Reporter)

Comment 18

5 years ago
Filed bug 786588 instead, per glandium's request over irc.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

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