429105 - [Solaris] Failed to build mozilla-central on solaris in js module.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Leon Sha]

After js module changed to c++, there are several build issues on solaris.


"../../../js/src/jsapi.cpp", line 4413: Error: Different types for "?:" (extern "C" int(*)(JSContext*,JSObject*,unsigned,long*,long*) and int(*)(JSContext*,JSObject*,unsigned,long*,long*)).
1 Error(s) and 1 Warning(s) detected.

"../../../js/src/jsgc.cpp", line 699: Error: The name table is unusable in a default parameter.
"../../../js/src/jsgc.cpp", line 699: Error: An integer constant expression is required within the array subscript operator.
"../../../js/src/jsgc.cpp", line 854: Error: Formal argument 1 of type char* in call to munmap(char*, unsigned) is being passed void*.


"../../../js/src/jsscript.cpp", line 610: Error: The name tn is unusable in a default parameter.
"../../../js/src/jsscript.cpp", line 610: Error: An integer constant expression is required within the array subscript operator.
"../../../js/src/jsscript.cpp", line 611: Error: The name tn is unusable in a default parameter.
"../../../js/src/jsscript.cpp", line 611: Error: An integer constant expression is required within the array subscript operator.


Also there are a lot of warings.

Comment 1

[Leon Sha]

Attachment: patch

I removed several JS_STATIC_ASSERT lines because there is a bug for "sizeof" in sun studio. If these assert check is really needed, I'll add #ifdef here.

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 315737 [details] [diff] [review]
patch

Those static assertions are important.  Please condition JS_STATIC_ASSERT's definition on detection of the broken compiler, if you can't just get the compiler fixed promptly.  (The conditioning in the definition should include a reference to the Sun bug number, if at all possible, and should be removed when there's an update Sun Studio with the fix.)

Comment 3

[Leon Sha]

Attachment: patch v2

Use a simple work around provided by our compiler team.
Below is the comments from out compiler team.
--------------------------------------------------------------------------
"1. The size of an array parameter has not significance. The declarations
   void foo(int a[]);
   void foo(int a[10]);
are equivalent. Omit the size of the array to avoid the compiler bug.

2. Declaring functions inside functions is generally a Bad Idea in C++ code, and is not a good idea in C code. It's bad in C++ code because it affects overload resolution. It's not good in either language because it confuses scopes, and because global functions should be declared only in headers to avoid conflicts.

If the function is declared at global scope where it belongs, you need to reference a global-scope variable in the sizeof expression, which the compiler accepts. That is, the code
       struct test_s *test;
       extern void test_f(int arg[sizeof(test->a) == sizeof(int) ? 1: -1]);
is OK outside of a function."
-------------------------------------------------------------------------------

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> Created an attachment (id=315922) [details]
> patch v2
> 
> Use a simple work around provided by our compiler team.
> Below is the comments from out compiler team.
> --------------------------------------------------------------------------
> "1. The size of an array parameter has not significance. The declarations
>    void foo(int a[]);
>    void foo(int a[10]);
> are equivalent. Omit the size of the array to avoid the compiler bug.
> 
> 2. Declaring functions inside functions is generally a Bad Idea in C++ code,
> and is not a good idea in C code. It's bad in C++ code because it affects
> overload resolution. It's not good in either language because it confuses
> scopes, and because global functions should be declared only in headers to
> avoid conflicts.

The declaration of the function is done as a hack to implement the static assert. AFAICS the current code is a valid C/C++ so this is not a bad idea in this particular case. But from a practical point of view something has to be done about that. 

The way the proposed patch implements this is not good since it moves the static asserts away from the code that relies on them. This requires to comment the assert defeating the self-documenting property of the assert.

I think the simplest way would be to split JS_STATIC_ASSERT in 2, JS_STATIC_ASSERT() and, say, JS_STATIC_CODE_ASSERT(), where the former can only be used outside the function while the latter can be used only inside. It can be implemented via:

#define JS_STATIC_CODE_ASSERT(condition) ((void) sizeof(int[(condition) ? 1 : -1])

Comment 5

[Brendan Eich [:brendan]]

Better to turn off JS_STATIC_ASSERT for compilers that don't support it as is. We get assertion coverage on major platforms. I'm not dismissing Solaris here but I would not want to complicate JS_STATIC_ASSERT users' lives for only that platform.

/be

Comment 6

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Agreed; I think that's the patch I asked for in comment 2.  We can condition the exclusion on the specific versions of Sun Studio that are broken, when there's one available with a fix.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 315922 [details] [diff] [review]
patch v2

r-; just disable STATIC_ASSERT for the broken compiler.  The cast and EXTERN_C changes look good, though.

Comment 8

[Leon Sha]

Attachment: patch v3

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 316180 [details] [diff] [review]
patch v3

No, you should put that check in to provide an alternate (empty) definition of JS_STATIC_ASSERT, or you'll just get broken when we add new ones.

Why does the JS_EXTERN_C want to be in the jscntxt.cpp file, rather than in the .h where that function is declared?

Comment 10

[Leon Sha]

Attachment: patch v4

About the JS_EXTERN_C, in jscntxt.h this is already added for function js_InitThreadPrivateIndex. Since this function is special, we also add extern c in cpp file. js_InitThreadPrivateIndex has a argument whose type is function pointer. The function pointer for c and c++ is different.
There is a warning for this.
"../../../js/src/jscntxt.cpp", line 82: Warning: function int(void(*)(void*)) overloads extern "C" int(extern "C" void(*)(void*)) because of different language linkages
This warning makes the function js_InitThreadPrivateIndex to be a c++ function. So for this function we also need to add extern c in cpp file.

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 316981 [details] [diff] [review]
patch v4

>+/*
>+ * Sun Studio C++ compiler has a bug 
>+ * "sizeof expression not accepted as size of array parameter"
>+ * Turn off this assert for Sun Studio until this bug got fixed.

"until this bug is fixed".

Please also add a link to the bug entry in Sun's system, or at least the number of the defect, so that it's easier for people to tell if it's fixed.

r=shaver with those changes.

Comment 12

[Leon Sha]

Attachment: patch v5

Another function need JS_EXTERN_C in debug build.

Comment 13

[Leon Sha]

Attachment: patch v5

Comment 14

[Leon Sha]

Comment on attachment 324411 [details] [diff] [review]
patch v5

Sorry to attach the wrong patch.

Comment 15

[Leon Sha]

(In reply to comment #11)
> (From update of attachment 316981 [details] [diff] [review])
> >+/*
> >+ * Sun Studio C++ compiler has a bug 
> >+ * "sizeof expression not accepted as size of array parameter"
> >+ * Turn off this assert for Sun Studio until this bug got fixed.
> 
> "until this bug is fixed".
> 
> Please also add a link to the bug entry in Sun's system, or at least the number
> of the defect, so that it's easier for people to tell if it's fixed.
> 
> r=shaver with those changes.
> 
I will make the changes when I push the patch.

Comment 16

[Leon Sha]

Attachment: Patch v6

Add the comments with shaver's suggestion.
Add another JS_EXTERN_C in debug build.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 331058 [details] [diff] [review]
Patch v6

>Bug 429105 [Solaris] Failed to build mozilla-central on solaris in js module.
>
>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -4473,7 +4473,8 @@
>                                     (flags & JSFUN_FAST_NATIVE)
>                                     ? (JSNative)
>                                       js_generic_fast_native_method_dispatcher
>-                                    : js_generic_native_method_dispatcher,
>+                                    : (JSNative)
>+                                      js_generic_native_method_dispatcher,

Is the added cast needed because of JS_STATIC_DLL_CALLBACK being used around the return type in the definition of js_generic_native_method_dispatcher? Otherwise that function seems to match JSNative already.

Is it possible to write the cast once, around the (?:) expression?

                                    (JSNative)
                                    ((flags & JSFUN_FAST_NATIVE)
                                     ? js_generic_fast_native_method_dispatcher
                                     : js_generic_native_method_dispatcher),

(still keeping line length < 80).

>+JS_BEGIN_EXTERN_C
> void
> js_DumpNamedRoots(JSRuntime *rt,
>                   void (*dump)(const char *name, void *rp, void *data),

Sorry, I didn't get the explanation of why extern "C" {...} is needed around function *definitions* if it's already around their prototypes.

My apologies for the tardy review. Too much travel in the last few weeks, but that is no excuse.

/be

Comment 18

[Leon Sha]

(In reply to comment #17)
> (From update of attachment 331058 [details] [diff] [review])
> >Bug 429105 [Solaris] Failed to build mozilla-central on solaris in js module.
> >
> >diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
> >--- a/js/src/jsapi.cpp
> >+++ b/js/src/jsapi.cpp
> >@@ -4473,7 +4473,8 @@
> >                                     (flags & JSFUN_FAST_NATIVE)
> >                                     ? (JSNative)
> >                                       js_generic_fast_native_method_dispatcher
> >-                                    : js_generic_native_method_dispatcher,
> >+                                    : (JSNative)
> >+                                      js_generic_native_method_dispatcher,
> 
> Is the added cast needed because of JS_STATIC_DLL_CALLBACK being used around
> the return type in the definition of js_generic_native_method_dispatcher?
> Otherwise that function seems to match JSNative already.
> 

> Is it possible to write the cast once, around the (?:) expression?
> 
>                                     (JSNative)
>                                     ((flags & JSFUN_FAST_NATIVE)
>                                      ? js_generic_fast_native_method_dispatcher
>                                      : js_generic_native_method_dispatcher),
> 
> (still keeping line length < 80).
> 

The error is not because of the return type. It is caused by different types
for "?:". The error message is
"../../../js/src/jsapi.cpp", line 4476: Error: Different types for "?:" (extern
"C" int(*)(JSContext*,JSObject*,unsigned,long*,long*) and
int(*)(JSContext*,JSObject*,unsigned,long*,long*)).
The reason for this is JSNative is a C function pointer while
js_generic_native_method_dispatcher is a c++ function.
> >+JS_BEGIN_EXTERN_C
> > void
> > js_DumpNamedRoots(JSRuntime *rt,
> >                   void (*dump)(const char *name, void *rp, void *data),
> 
> Sorry, I didn't get the explanation of why extern "C" {...} is needed around
> function *definitions* if it's already around their prototypes.
> 
This function is different with other functions because the second argument is
a c++ function pointer, which cause this function can not compile to c
function.
There is a warning when extern "C" was not added.
"../../../js/src/jsgc.cpp", line 1571: Warning: function
void(JSRuntime*,void(*)(const char*,void*,void*),void*) overloads extern "C"
void(JSRuntime*,extern "C" void(*)(const char*,void*,void*),void*) because of
different language linkages.

And when you use nm to check the symbols you get two symbols.
[leon.sha@intercept src]$ nm libmozjs.so |grep js_DumpNamedRoots
[10450] |    612992|        79|FUNC |GLOB |0    |12    
|__1cRjs_DumpNamedRoots6FpnJJSRuntime_pFpkcpv4_v4_v_
[9522]  |         0|         0|FUNC |GLOB |0    |UNDEF  |js_DumpNamedRoots

> My apologies for the tardy review. Too much travel in the last few weeks, but
> that is no excuse.
> 
> /be
> 

Comment 19

[Brendan Eich [:brendan]]

(In reply to comment #18)
> The error is not because of the return type. It is caused by different types
> for "?:". The error message is
> "../../../js/src/jsapi.cpp", line 4476: Error: Different types for "?:" (extern
> "C" int(*)(JSContext*,JSObject*,unsigned,long*,long*) and
> int(*)(JSContext*,JSObject*,unsigned,long*,long*)).
> The reason for this is JSNative is a C function pointer while
> js_generic_native_method_dispatcher is a c++ function.

Suggest making js_generic_native_method_dispatcher be extern "C", then. We need the API to use C linkage for these everywhere.

Same for js_generic_fast_native_method_dispatcher I think.

> > >+JS_BEGIN_EXTERN_C
> > > void
> > > js_DumpNamedRoots(JSRuntime *rt,
> > >                   void (*dump)(const char *name, void *rp, void *data),
> > 
> > Sorry, I didn't get the explanation of why extern "C" {...} is needed around
> > function *definitions* if it's already around their prototypes.
> > 
> This function is different with other functions because the second argument is
> a c++ function pointer, which cause this function can not compile to c
> function.
> There is a warning when extern "C" was not added.
> "../../../js/src/jsgc.cpp", line 1571: Warning: function
> void(JSRuntime*,void(*)(const char*,void*,void*),void*) overloads extern "C"
> void(JSRuntime*,extern "C" void(*)(const char*,void*,void*),void*) because of
> different language linkages.

Ok, this makes sense and supports the idea of making js_generic_native_method_dispatcher be extern "C".

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 331058 [details] [diff] [review]
Patch v6

We don't use SR in js/src.

/be

Comment 21

[Leon Sha]

Attachment: patch v7

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 336308 [details] [diff] [review]
patch v7

Thanks for your patience.

/be

Comment 23

[Leon Sha]

Checked in. Thanks Brendan.