492720 - libgjs failed to compile with jsopcode.h of Firefox 3.5 beta 4

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Ginn Chen]

The error message is

cc -G -h console.so -o .libs/console.so  .libs/console_la-console.o -R/export/home/brianca/packages/BUILD/gjs-0.3/gjs/.libs -L/usr/lib/firefox ./.libs/libgjs.so -L/usr/lib/mps -lgmodule-2.0 -lgobject-2.0 -lglib-2.0 -lmozjs -lplds4 -lplc4 -lnspr4 -lpthread -ldl -lposix4 -lc  -z defs
Undefined                       first referenced
symbol                             in file
js_GetVariableStackUses             ./.libs/libgjs.so
js_GetEnterBlockStackDefs           ./.libs/libgjs.so


In jsopcode.h, we have
356 static JS_INLINE uintN
357 js_GetStackUses(const JSCodeSpec *cs, JSOp op, jsbytecode *pc)
358 {
359     JS_ASSERT(cs == &js_CodeSpec[op]);
360     if (cs->nuses >= 0)
361         return cs->nuses;
362     return js_GetVariableStackUses(op, pc);
363 }
364 
365 static JS_INLINE uintN
366 js_GetStackDefs(JSContext *cx, const JSCodeSpec *cs, JSOp op, JSScript *script,
367                 jsbytecode *pc)
368 {
369     JS_ASSERT(cs == &js_CodeSpec[op]);
370     if (cs->ndefs >= 0)
371         return cs->ndefs;
372 
373     /* Only JSOP_ENTERBLOCK has a variable number of stack defs. */
374     JS_ASSERT(op == JSOP_ENTERBLOCK);
375     return js_GetEnterBlockStackDefs(cx, script, pc);
376 }

But js_GetVariableStackUses and js_GetEnterBlockStackDefs are not export API.
Therefore we have the link problem.

Comment 1

[Andreas Gal :gal]

Attachment: patch

I don't think inlining these particular functions is worth the trouble. Can you please verify that this patch fixes your linker problem?

Comment 2

[Brendan Eich [:brendan]]

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

Deferring to Igor, the inline may be worth it for some kind of perf.

But I suspect you are wallpapering over a "JS_INLINE is misdefined on Solaris" bug. :-/

/be

Comment 3

[Igor Bukanov]

(In reply to comment #1)
> I don't think inlining these particular functions is worth the trouble.

The js_GetStackDefs has debug-only parameter that would be completely eliminated in a non-debug build. When the function is not inlined it would add an extra bloat.

(In reply to comment #2)
> (From update of attachment 377118 [details] [diff] [review])
> But I suspect you are wallpapering over a "JS_INLINE is misdefined on Solaris"
> bug. :-/

There are two possibility. Either some code inside libgjs calls js_GetStackUses or js_GetStackDefs. In this case the embedding in question does need FRIEND on js_GetVariableStackUses and js_GetEnterBlockStackDefs. Or the embedding does not call the functions from libgjs. That means a compiler bug where it put unused static inline function in the object file and the linker tried to resolve it. In this case we need to consider how to work-around this.

To Ginn Chen: could you tell which possibility happens in your case? If it is the second one and the issue is a compiler, what happen if you replace static JS_INLINE to just inline or "extern inline" on these two functions?

Comment 4

[Igor Bukanov]

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

I clear the review request pending the input about the real nature of the bug.

Comment 5

[Ginn Chen]

Brian Cameron told me there's no call of js_GetStackUses, js_GetStackDefs in libgjs.

I'm not sure if it is a bug of the compiler.
I wrote an almost empty C program, it just includes jsopcode.h.
If I compile it with -xO0 to -xO5, it is fine. If I compile it without -xO[0-5], or with -g, it fails to link.

Using "extern inline" or "inline" doesn't fix it.
inline has different meanings with C89/C99 or different compilers.
See: http://blogs.sun.com/dew/entry/c99_inline_function
I think we should not rely on it.

Comment 6

[Brian Cameron]

I think it is important to fix ths bug.  Since the problem happens when simply including the header file, that is a pretty serious problem, I think.  I have a suspicion that just moving the two offending functions from the header file to the c-file would likely hide them so the compiler and linker don't get confused.  
Ginn, could you try this, or perhaps work with the Sun Studio compiler team and ask them the best way to fix this?  If you don't have a contact person, I would
recommend pinging [e-mail], who works on the compiler and has been helpful in addressing other desktop compiler issues.  Thanks.

Comment 7

[Brendan Eich [:brendan]]

Attachment: #ifdef __cplusplus

Please test this and confirm it fixes the problem.

/be

Comment 8

[Brendan Eich [:brendan]]

Ginn, could you carry this to completion? If it fixes the compilation problem, r=me on the patch (you can mark it yourself). Thanks,

/be

Comment 9

[Ginn Chen]

Comment on attachment 381328 [details] [diff] [review]
#ifdef __cplusplus

yes, it works.
r=brendan

Comment 10

[Ginn Chen]

Committed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ef153536c649

Comment 11

[Ginn Chen]

Brendan,
It seems we have some similar problems with jsinterp.h now.
js_PurgeScopeChainHelper, js_GetVariableStackUses, js_GetEnterBlockStackDefs, JS_GetTrapOpcode, js_ComputeThis are local linkage in libmozjs.so.
So a C program includes <jsinterp.h> can't be linked with ld option "-z defs" which is default.

Do you think we should use "#ifdef __cplusplus" for all these function declarations?

Comment 12

[Ginn Chen]

oops, JS_GetTrapOpcode is obviously global.
And js_GetVariableStackUses, js_GetEnterBlockStackDefs were fixed by the earlier patch.

So we still have problems with
js_PurgeScopeChainHelper
js_ComputeThis

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 381328 [details] [diff] [review]
#ifdef __cplusplus

a1912=beltzner

Comment 14

[Ginn Chen]

Attachment: similar patch to fix js_PurgeScopeChainHelper and js_ComputeThis

Comment 15

[Ginn Chen]

Part 2 committed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/9efce50d28ca

Comment 16

[Ginn Chen]

Attachment: combined patch for 1.9.1 branch

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 390191 [details] [diff] [review]
combined patch for 1.9.1 branch

a1912=beltzner

Comment 18

[Ginn Chen]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ebefa2ea540c

Comment 19

[u279076]

What is the best/simplest way for QA to verify this bug for Firefox 3.5.2?

Comment 20

[Ginn Chen]

I think you can only verify it from source code or source code in SDK tarball.