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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Leon Sha, Assigned: Leon Sha)

Tracking

Trunk
x86
SunOS
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

10 years ago
Created attachment 315737 [details] [diff] [review]
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.
Attachment #315737 - Flags: review?(brendan)
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.)
Attachment #315737 - Flags: review?(brendan) → review-
(Assignee)

Comment 3

10 years ago
Created attachment 315922 [details] [diff] [review]
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."
-------------------------------------------------------------------------------
Attachment #315737 - Attachment is obsolete: true
Attachment #315922 - Flags: review?(shaver)

Comment 4

10 years ago
(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])
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
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 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.
Attachment #315922 - Flags: review?(shaver) → review-
(Assignee)

Comment 8

10 years ago
Created attachment 316180 [details] [diff] [review]
patch v3
Assignee: general → leon.sha
Attachment #315922 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #316180 - Flags: review?(shaver)
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?
Attachment #316180 - Flags: review?(shaver) → review-
(Assignee)

Comment 10

10 years ago
Created attachment 316981 [details] [diff] [review]
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.
Attachment #316180 - Attachment is obsolete: true
Attachment #316981 - Flags: review?(shaver)
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.
Attachment #316981 - Flags: review?(shaver) → review+
(Assignee)

Comment 12

10 years ago
Created attachment 324021 [details] [diff] [review]
patch v5

Another function need JS_EXTERN_C in debug build.
Attachment #324021 - Flags: review?(shaver)
(Assignee)

Comment 13

10 years ago
Created attachment 324411 [details] [diff] [review]
patch v5
Attachment #324021 - Attachment is obsolete: true
Attachment #324021 - Flags: review?(shaver)
(Assignee)

Comment 14

10 years ago
Comment on attachment 324411 [details] [diff] [review]
patch v5

Sorry to attach the wrong patch.
Attachment #324411 - Flags: review?(shaver)
(Assignee)

Comment 15

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

Comment 16

10 years ago
Created attachment 331058 [details] [diff] [review]
Patch v6

Add the comments with shaver's suggestion.
Add another JS_EXTERN_C in debug build.
Attachment #324411 - Attachment is obsolete: true
Attachment #331058 - Flags: superreview?(brendan)
Attachment #331058 - Flags: review?(brendan)
Attachment #324411 - Flags: review?(shaver)
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

Updated

10 years ago
Blocks: 449757
(Assignee)

Comment 18

10 years ago
(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
> 
(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 on attachment 331058 [details] [diff] [review]
Patch v6

We don't use SR in js/src.

/be
Attachment #331058 - Flags: superreview?(brendan)
(Assignee)

Comment 21

10 years ago
Created attachment 336308 [details] [diff] [review]
patch v7
Attachment #331058 - Attachment is obsolete: true
Attachment #336308 - Flags: review?(brendan)
Attachment #331058 - Flags: review?(brendan)

Updated

10 years ago
Attachment #336308 - Flags: review?(brendan) → review+
Comment on attachment 336308 [details] [diff] [review]
patch v7

Thanks for your patience.

/be
(Assignee)

Comment 23

10 years ago
Checked in. Thanks Brendan.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.