Build break js_static_assert on OS/2

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Peter Weilbacher, Assigned: Igor Bukanov)

Tracking

({fixed1.9.1})

1.9.1 Branch
x86
OS/2
fixed1.9.1
Points:
---
Bug Flags:
wanted1.9.1 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 3 obsolete attachments)

v3
26.91 KB, patch
brendan
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
With current mozilla-1.9.1 I get a build break on OS/2 in js/src. Errors are

LIR.cpp
In file included from M:/central/comm/mozilla/js/src/jsbool.h:46,
                 from M:/central/comm/mozilla/js/src/jstracer.cpp:56:
M:/central/comm/mozilla/js/src/jsprvtd.h:268: error: previous declaration
   of `void js_static_assert(int*)' with C++ linkage
M:/central/comm/mozilla/js/src/jsapi.h:1019: error: conflicts with new
   declaration with C linkage

or

Assembler.cpp
In file included from M:/central/comm/mozilla/js/src/jstracer.cpp:54:
M:/central/comm/mozilla/js/src/nanojit/nanojit.h:141:44: warning: 
   anonymous variadic macros were introduced in C99
M:/central/comm/mozilla/js/src/nanojit/nanojit.h:188:30: warning:
   anonymous variadic macros were introduced in C99
In file included from M:/central/comm/mozilla/js/src/nanojit/nanojit.h:235,
                 from M:/central/comm/mozilla/js/src/jstracer.cpp:54:
M:/central/comm/mozilla/js/src/nanojit/Native.h:120:36: warning:
   anonymous variadic macros were introduced in C99
In file included from M:/central/comm/mozilla/js/src/jsbool.h:46,
                 from M:/central/comm/mozilla/js/src/jstracer.cpp:56:
M:/central/comm/mozilla/js/src/jsprvtd.h:268: error: previous
   declaration of `void js_static_assert(int*)' with C++ linkage
M:/central/comm/mozilla/js/src/jsapi.h:1019: error: conflicts with new
   declaration with C linkage

Haven't tried yet to find out where this comes from. This is when compiling with GCC 3.3.5.

Comment 1

9 years ago
(In reply to comment #0)
 
> Haven't tried yet to find out where this comes from. This is when compiling
> with GCC 3.3.5.
bug478543
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f97755b4ba32
(Assignee)

Updated

9 years ago
Assignee: general → igor
(Reporter)

Comment 2

9 years ago
Bracketing the JS_STATIC_ASSERT line in jsprvtd.h with JS_BEGIN_EXTERN_C / JS_END_EXTERN_C (or the whole file) helps. GCC 3.3.5 can then compile the C++ files that include it. But as GCC 4.3.3 can do that without this hack, I am not sure what is correct.
(Assignee)

Comment 3

9 years ago
Created attachment 364903 [details] [diff] [review]
v1

The fix moves the static asserts from headers into source files and moves the definition of data-function casts macros into jstypes.h to make them available outside SM.
Attachment #364903 - Flags: review?(brendan)
Comment on attachment 364903 [details] [diff] [review]
v1

>+/***********************************************************************
>+** MACROS:      JS_FUNC_TO_DATA_PTR
>+**              JS_DATA_TO_FUNC_PTR
>+** DESCRIPTION:
>+**      Macros to convert between function and data pointers assuming that
>+**      they they have the same size. Use them like this:

s/they they/they/

r=me with that. IIRC mrbkap has a need for this patch, possibly a dup bug.

/be
Attachment #364903 - Flags: review?(mrbkap)
Attachment #364903 - Flags: review?(brendan)
Attachment #364903 - Flags: review+
Flags: wanted1.9.1?

Updated

9 years ago
Flags: wanted1.9.1? → wanted1.9.1+
Comment on attachment 364903 [details] [diff] [review]
v1

bug 480236 is the bug brendan was talking about. I'll dupe to this one. Thanks igor!
Attachment #364903 - Flags: review?(mrbkap) → review+

Updated

9 years ago
Duplicate of this bug: 480236
(Assignee)

Comment 7

9 years ago
Created attachment 365406 [details] [diff] [review]
v1b

The new version of the patch fixes the comment text.
Attachment #364903 - Attachment is obsolete: true
Attachment #365406 - Flags: review+
(Assignee)

Comment 8

9 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/5befb6301e9b
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 9

9 years ago
Blake Kaplan wrote in comment #5:

> bug 480236 is the bug brendan was talking about. 

Note that besides the generic macros for casts between function and data pointers the bug 478543 also added specialized inline functions for JSPropertyOp and JSObject* casts. The functions are in jsscope.h, http://hg.mozilla.org/tracemonkey/file/5befb6301e9b/js/src/jsscope.h#l277 . Should they also be public?
Blocks: 478543
(Assignee)

Comment 10

9 years ago
I backed out the landed patch as it broke 32-bit Linux build with the same error as was reported on OS2:

In file included from /home/igor/m/tm/js/src/jsutil.cpp:76:
/home/igor/m/tm/js/src/jsutil.cpp:57: error: previous declaration of 'void js_static_assert(int*)' with 'C++' linkage
/home/igor/m/tm/js/src/jsbit.h:210: error: conflicts with new declaration with 'C' linkage
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 11

9 years ago
Created attachment 365411 [details] [diff] [review]
v2

To fix this bug I moved JS_STATIC_ASSERT from the header to the source file. But due to the inclusion of jsbit.h there it was necessary to remove the static asserts from the header as well. But in the previous version of the patch I missed one assert. The new version fixes that.
Attachment #365406 - Attachment is obsolete: true
Attachment #365411 - Flags: review+
(Assignee)

Comment 12

9 years ago
Created attachment 365416 [details] [diff] [review]
v3

To fix the problem once and for all the patch moves all static asserts out of headers into the source files.
Attachment #365411 - Attachment is obsolete: true
Attachment #365416 - Flags: review?(brendan)

Comment 13

9 years ago
Comment on attachment 365416 [details] [diff] [review]
v3

I really don't think this is a good idea. Part of the goal of the static asserts is to protect API clients from bad/mismatched compile flags... if you move all the checks into the sources, that value is lost.
Comment on attachment 365416 [details] [diff] [review]
v3

I can't find it now, but another bug has a comment from me talking about the repeated problems we've had with static asserts in header files. That was more about modularity than portability, but we have spent too much time on both.

Benjamin, I do not recall a case of embedding failing to use the same int type model as SpiderMonkey. It's not a big problem in the field, AFAIK.

/be
Attachment #365416 - Flags: review?(brendan) → review+
(Assignee)

Comment 15

9 years ago
(In reply to comment #13)
> Part of the goal of the static
> asserts is to protect API clients from bad/mismatched compile flags...

This is a fine goal but it is not relevant for assert in questions from SM headers. Most of the asserts are in private headers and those that are in public ones just enforce platform-independent definitions of various macros. Thus to verify them it is sufficient just to compile SM, compiling the whole application does not add any safety. 

Yet the asserts in headers do cause problems requiring spending efforts to fix them as this bug demonstrates. Unfortunately so far nobody has come up with a bulletproof implementation of the JS_STATIC_ASSERT macro that works on all platforms. The history of the macro evolution is rather revealing in that regard. So the latest patch in this bug is just a conservative measure to prevent spending more time on the issue in future.
(Assignee)

Comment 16

9 years ago
landed to TM - http://hg.mozilla.org/tracemonkey/rev/78c1faf2028b
Whiteboard: fixed-in-tracemonkey

Comment 17

9 years ago
http://hg.mozilla.org/mozilla-central/rev/78c1faf2028b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(In reply to comment #15)
> Yet the asserts in headers do cause problems requiring spending efforts to fix
> them as this bug demonstrates. Unfortunately so far nobody has come up with a
> bulletproof implementation of the JS_STATIC_ASSERT macro that works on all
> platforms. The history of the macro evolution is rather revealing in that
> regard. So the latest patch in this bug is just a conservative measure to
> prevent spending more time on the issue in future.

For what it's worth, I don't think we should be moving static asserts away from the relevant code just to make non-tier1 compilers happy.  We disabled static asserts for the Sun compiler when they hit problems with it, and I think it would have been perfectly acceptable to do that here.  Pretty sure I'm on the losing end of that argument, but I'm not going to let people get away with thinking that we have to move static asserts out of headers when in some cases that's the most natural place for them to reside.  :-P
Portability is not the only reason to avoid static asserts in .h files -- trying to add them can drag in other .h files that should not be nested. It's not just about mean ol' tier-2 compilers.

/be
(In reply to comment #9)
> Should they also be public?

I don't think it's necessary to expose those functions.

Updated

9 years ago
Flags: in-testsuite-

Comment 21

9 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13ef79dbe05a
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.