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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Simply moving the definitions in the header works, actually.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Move JSAtomState::commonAtomsOffset and JSAtomState::lazyAtomsOffset in jsatom.h

Comment 3

[Mike Hommey [:glandium]]

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.

Comment 4

[Chris Leary [:cdleary] (not checking bugmail)]

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.

Comment 5

[Mike Hommey [:glandium]]

(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...

Comment 6

[Jeff Walden [:Waldo]]

I fixed this in bug 789635.