603821 - debug non-libxul builds fail in WebGLContext.cpp, with "Undefined reference to `mozilla::gl::GLContext::sCurrentGLContext'"

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Daniel Holbert [:dholbert]]

This build error just started (in my debug non-libxul build) when I did a pull -u:
{
WebGLContext.cpp:(.text._ZN7mozilla2gl9GLContext11MakeCurrentEi[mozilla::gl::GLContext::MakeCurrent(int)]+0x16): undefined reference to `mozilla::gl::GLContext::sCurrentGLContext'
/usr/bin/ld.bfd.real: ../../content/canvas/src/libgkconcvs_s.a(WebGLContext.o): relocation R_X86_64_PC32 against undefined hidden symbol `mozilla::gl::GLContext::sCurrentGLContext' can not be used when making a shared object
/usr/bin/ld.bfd.real: final link failed: Bad value
collect2: ld returned 1 exit status
distcc[8634] ERROR: compile (null) on localhost failed
make[5]: *** [libgklayout.so] Error 1
make[4]: *** [libs] Error 2
make[3]: *** [libs_tier_platform] Error 2
make[2]: *** [tier_platform] Error 2
make[1]: *** [default] Error 2
make: *** [build] Error 2
}
Looks like this variable (sCurrentGLContext) was just added, in bug 597881's changeset:
  http://hg.mozilla.org/mozilla-central/rev/d3f448b20f20

Comment 1

[Daniel Holbert [:dholbert]]

bjacob / vlad, this probably needs a backout or a bustage fix, right?

Perhaps an #ifdef MOZ_LIBXUL around all uses of sCurrentGLContext would work as a bustage fix. (haven't looked at the patch, so I don't know how much that actually makes sense)

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Sorry, I dont have my ssh key right now. Will take care of this tomorrow morning toronto time if nobody does it before (feel free to backout).

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

needs a THEBES_API in the declaration, I believe.

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Benoit, it would be great if you remember to add THEBES_API (or the equivalent) for symbols which need to be accessed across modules.

Comment 5

[[e-mail]]

(In reply to comment #4)
> Benoit, it would be great if you remember to add THEBES_API (or the equivalent)
> for symbols which need to be accessed across modules.
If only it was that obvious, e.g. try making MakeCurrent virtual.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to comment #5)
> (In reply to comment #4)
> > Benoit, it would be great if you remember to add THEBES_API (or the equivalent)
> > for symbols which need to be accessed across modules.
> If only it was that obvious, e.g. try making MakeCurrent virtual.

Sorry, I didn't know about that.

MakeCurrent used to be virtual before my faulty commit.

Neil, are you saying that a virtual function can't access a THEBES_API static member variable?

Anyway, thanks a lot for fixing this for me, Ehsan!

This being a bustage fix, do you really need review?

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 482786 [details] [diff] [review]
Patch (v1)

This is a simple symbol visibility bustage fix.  Ship it.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/mozilla-central/rev/c18c9fa00a80

(Checked it doesn't break normal linux debug build)

Comment 9

[(no longer active)]

(In reply to comment #5)
> (In reply to comment #4)
> > Benoit, it would be great if you remember to add THEBES_API (or the equivalent)
> > for symbols which need to be accessed across modules.
> If only it was that obvious, e.g. try making MakeCurrent virtual.

Virtual functions don't need to be visible across modules if called virtually.  They only need to be visible when the linker is resolving their symbol statically, right?

Benoit, here's what's happening here (and in the recent similar bugs as well).  In libxul builds, we link pretty much everything in a single library called libxul, therefore the linker can resolve almost all symbol names statically.  In non-libxul builds, however, we have separate shared object modules.  For example, we have libgkgfx.dylib (which includes most of the stuff coming from gfx/), and libgklayout.dylib (which includes layout/ and content/, among other stuff).  Now, when you add a symbol which is local to libgkgfx.dylib, it is not exported by default.  Therefore, when you use it from code which ends up in libgklayout.dylib for example, the linker can't find that symbol, hence the error.  THEBES_API and the equivalent mark the symbol as visible across shared object modules (or exported on Windows), which makes the linker able to find them and correctly link the program.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Many thanks Ehsan for the explanation! Very useful.