401300 - jsapi.h now depends on unpublished header jsutil.h

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wesley W. Garland]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.8) Gecko/20071008 Firefox/2.0.0.8
Build Identifier: jsapi.h head 3.162

jsapi.h now depends on jsutil.h. jsutil.h is not published to dist/include during make export. 

This breaks all embeddings which do not -I include the SpiderMonkey source directory in their CPPFLAGS.

I believe the last version which did not depend on jsutil.h was 3.142. 3.143 was checked in to fix bug 375270.  (2007 04 17)

The reason jsutil.h was added seems to be to provide the JS_STATIC_ASSERT macro.  Use of this macro is not limited (at least in the current jsapi.h) to conditional code.

Reproducible: Always

Steps to Reproduce:
1. Embed spidermonkey in a C program
2. Try to compile your program
3. Observe that it does not compile, but rather, complains that it cannot find jsutil.h


Expected Results:  
Build with error.

Suggested possible fixes:
 1 - Add jsutil.h to the export list in Makefile.ref / Makefile.in
 2 - Add JS_STATIC_ASSERT macro to jsapi.h
 3 - Remove references to JS_STATIC_ASSERT from jsapi.h

I have worked around this problem using both methods 1 and 2 without issue.

Until then, new embedders pulling from CVS will be unable to build SpiderMonkey.

Comment 1

[Brendan Eich [:brendan]]

I've whinged before about not putting JS_STATIC_ASSERTS in header files -- it's a modularity killer.

Igor, can you land the patch?

/be

Comment 2

[Brendan Eich [:brendan]]

I mean take this bug and patch it, of course. Preemptive r+a=me.

On the right possible fix, 1 is out for now (we liberally export but I do not think jsutil.h should be nested in jsapi.h); 2 is possible but breaks symmetry with file housing JS_ASSERT; 3 is what we've done elsewhere: put JS_STATIC_ASSERT calls in a canonical (for that assertion) .c file.

/be

Comment 3

[Wesley W. Garland]

Attachment: Proposed patch which fixes jsapi.h and dependencies

Here is a proposed patch against a recent trunk checkout which fixes the problem for me.

The changes I made were to remove jsutil.h from jsapi.h, then move some static asserts out of jsapi.h and jsatom.h to allow Spidermonkey and trivial embeddings to build without publishing jsutil.h.  (i.e., option 3 in my initial bug report).

The following JS_STATIC_ASSERTs were moved:
 - JS_TRACE_KIND from jsapi.h to jsgc.c
 - sizeof(JSAtom *) from jsatom.h into jsatom.c
 - sizeof(JSHashNumber) removed from jsatom.h, already in jsscope.c
 - ATOM_OFFSET_START and ATOM_OFFSET_LIMIT from jsatom.h into jsatom.c
 - offsetof(JSAtomState, typeAtoms) and booleanAtoms from jsatom.h into jsatom.c

Note: Related to this bug, the following files in dist/include still depend on jsutil.h: jsbit.h, jscntxt.h, jsgc.h, jsopcode.h, jsotypes.h.

Note 2: Related to this bug, the following files in dist/include still have JS_STATIC_ASSERT statements: jsbit.h, jscntxt.h, jsgc.h, jsopcode.h

Open question -- should those files also be static-assertion-cleaned at a later date? Or should they be removed from dist/include?  (They do not appear, at a casual glance, to be dependencies of jsapi.h)

Comment 4

[Igor Bukanov]

Comment on attachment 298738 [details] [diff] [review]
Proposed patch which fixes jsapi.h and dependencies

>Index: jsatom.c
...
>+JS_STATIC_ASSERT(1 * sizeof(JSAtom *) ==
>+                 offsetof(JSAtomState, typeAtoms) - ATOM_OFFSET_START);
>+JS_STATIC_ASSERT((1 + JSTYPE_LIMIT) * sizeof(JSAtom *) ==
>+                 offsetof(JSAtomState, booleanAtoms) - ATOM_OFFSET_START);
>+
>+JS_STATIC_ASSERT(sizeof(JSAtom *) == JS_BYTES_PER_WORD);
> JS_STATIC_ASSERT(JS_ARRAY_LENGTH(js_common_atom_names) * sizeof(JSAtom *) ==
>                  LAZY_ATOM_OFFSET_START - ATOM_OFFSET_START);
> 

These four asses require a comment about their relation with js_common_atom_names. 

>Index: jsatom.h
>@@ -282,21 +275,16 @@ extern const char *const js_common_atom_
> /*
>  * Macros to access C strings for JSType and boolean literals together with
>  * checks that type names and booleans starts from index 1 and 1+JSTYPE_LIMIT
>  * correspondingly.
>  */
> #define JS_TYPE_STR(type)    (js_common_atom_names[1 + (type)])
> #define JS_BOOLEAN_STR(type) (js_common_atom_names[1 + JSTYPE_LIMIT + (type)])

The comments should refer to the static checks in jsatom.c.

Comment 5

[Wesley W. Garland]

"[12:54]	<jimb>	Wes: jsutil.h is installed these days."

New build system fixes this -- bug 97954

Comment 6

[Bob Clary [:bc] (inactive)]

fix for 1.9.0 SpiderMonkey 1.8.0

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

This is the one-line patch to put a copy of jsutil.h in dist/include when the user does `make -f Makefile.ref export`.

r?brendan because Brendan didn't want to fix it this way originally.  We can fix it in a different way for 1.8.1.  If desired, I can specifically release-note that JS_STATIC_ASSERT is not an API and will go away in 1.8.1.  I just want to do a 1.8 release embedders can use.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 362586 [details] [diff] [review]
v1

Requesting approval1.9.0.8.  This is NPOTB and the bug blocks the official SpiderMonkey 1.8 source release.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 362586 [details] [diff] [review]
v1

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 10

[Bob Clary [:bc] (inactive)]

attachment 362586 [details] [diff] [review] checked into 1.9.0

/cvsroot/mozilla/js/src/Makefile.ref,v  <--  Makefile.ref
new revision: 3.54; previous revision: 3.53

Comment 11

[Bob Clary [:bc] (inactive)]

v 1.9.0 dist/include/js/jsutil.h now included.