jsapi.h now depends on unpublished header jsutil.h

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
minor
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Wesley W. Garland, Assigned: jorendorff)

Tracking

({verified1.9.0.9})

1.9.0 Branch
verified1.9.0.9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
(Reporter)

Comment 3

10 years ago
Created attachment 298738 [details] [diff] [review]
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)
Assignee: general → wes
Status: NEW → ASSIGNED
Attachment #298738 - Flags: review?
(Reporter)

Updated

10 years ago
Attachment #298738 - Flags: review? → review?(igor)

Comment 4

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

Comment 5

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

New build system fixes this -- bug 97954
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → WONTFIX

Comment 6

9 years ago
fix for 1.9.0 SpiderMonkey 1.8.0
Blocks: 428420
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Version: unspecified → 1.9.0 Branch
(Assignee)

Comment 7

9 years ago
Created attachment 362586 [details] [diff] [review]
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.
Assignee: wes → jorendorff
Attachment #298738 - Attachment is obsolete: true
Attachment #362586 - Flags: review?(brendan)
Attachment #298738 - Flags: review?(igor)
Attachment #362586 - Flags: review?(brendan) → review+
(Assignee)

Comment 8

9 years ago
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.
Attachment #362586 - Flags: approval1.9.0.8?
Comment on attachment 362586 [details] [diff] [review]
v1

Approved for 1.9.0.8, a=dveditz for release-drivers
Attachment #362586 - Flags: approval1.9.0.8? → approval1.9.0.8+

Comment 10

9 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Keywords: fixed1.9.0.8

Comment 11

9 years ago
v 1.9.0 dist/include/js/jsutil.h now included.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.0.8 → verified1.9.0.8
You need to log in before you can comment on or make changes to this bug.