Closed Bug 659902 Opened 10 years ago Closed 9 years ago

JSAtomState::commonAtomsOffset and JSAtomState::lazyAtomsOffset induce static initializers

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: [js:t])

Attachments

(1 file)

Using consts defined outside the class definition makes the use of these variables require calculations, despite them being constants.
This is the most notable in imacros.c.out, where the tables need to be filled at runtime instead of build time.

Sadly, defining the const values in the class definition is not possible because the class is not completely defined by then.

This also makes a problem when building in c++0x mode with gcc on x86-64:
./imacros.c.out:50:1: error: narrowing conversion of ‘((784ul - ((long unsigned int)JSAtomState::commonAtomsOffset)) / 8ul)’ from ‘long unsigned int’ to ‘jsbytecode’ inside { } [-fpermissive]
./imacros.c.out:50:1: error: narrowing conversion of ‘((72ul - ((long unsigned int)JSAtomState::commonAtomsOffset)) / 8ul)’ from ‘long unsigned int’ to ‘jsbytecode’ inside { } [-fpermissive]
./imacros.c.out:50:1: error: narrowing conversion of ‘((768ul - ((long unsigned int)JSAtomState::commonAtomsOffset)) / 8ul)’ from ‘long unsigned int’ to ‘jsbytecode’ inside { } [-fpermissive]
./imacros.c.out:50:1: error: narrowing conversion of ‘((784ul - ((long unsigned int)JSAtomState::commonAtomsOffset)) / 8ul)’ from ‘long unsigned int’ to ‘jsbytecode’ inside { } [-fpermissive]
(...)
because of the size difference between jsbytecode and size_t on x86-64.

It would probably make sense to use macros instead of consts, like it was before bug 654301.
Simply moving the definitions in the header works, actually.
Assignee: general → mh+mozilla
Attachment #535293 - Flags: review?(cdleary)
Comment on attachment 535293 [details] [diff] [review]
Move JSAtomState::commonAtomsOffset and JSAtomState::lazyAtomsOffset in jsatom.h

Doh, no it doesn't work. Fails to link.
Attachment #535293 - Flags: review?(cdleary)
Wow, my faith in C++ constexprs has totally been shaken. To enumerate the options for hacking around C++ compiler stupidity: does moving them to non-member static const exprs work? If not, then we can try the enum hack, and finally we can always fall back on macros, which was how it was originally done.
(In reply to comment #4)
> Wow, my faith in C++ constexprs has totally been shaken. To enumerate the
> options for hacking around C++ compiler stupidity: does moving them to
> non-member static const exprs work? If not, then we can try the enum hack,
> and finally we can always fall back on macros, which was how it was
> originally done.

Unfortunately, i don't think moving them to non-member static const exprs will work, for the same reason my patch doesn't work. The only way static const exprs don't end up creating static initializers when used is if they are defined within the class, but that's not possible in this very case because their value depends on the class being defined, which it is not at the point where they are defined. The enum hack would have the same problem, except if the enum is defined outside the class, at which point it's not very much different from using macros...
Depends on: 790349
I fixed this in bug 789635.
Status: NEW → RESOLVED
Closed: 9 years ago
Depends on: 789635
No longer depends on: 790349
Resolution: --- → FIXED
Whiteboard: [js:t]
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.