951743 - toolkit/library/libs fails to link with exact rooting enabled on MacOSX 10.7 builds

Status: RESOLVED FIXED

(Toolkit Graveyard :: Build Config, defect)

Description

[Jon Coppeard (:jonco)]

The following error is reported when building the browser with generational GC enabled.  It appears to only happen on MacOSX opt builds.

ld: bad codegen, pointer diff in __ZN2js23DirectEvalStringFromIonEP9JSContextN2JS6HandleIP8JSObjectEENS3_IP8JSScriptEENS3_INS2_5ValueEEENS3_IP8JSStringEEPhNS2_13MutableHandleISA_EE to global weak symbol __ZTVN2JS14CompileOptionsE for architecture i386
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[6]: *** [XUL] Error 1
make[5]: *** [toolkit/library/libs] Error 2
make[4]: *** [libs] Error 2
make[3]: *** [default] Error 2
make[2]: *** [realbuild] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2

Comment 1

[Steve Fink [:sfink] [:s:]]

Ok, this is ugly.

Files compiled within js/src:
  class CompileOptions;
Files compiled outside of js/src:
  class __attribute__((visibility("default"))) CompileOptions;

The first one produces:
  (__DATA,__datacoal_nt) weak private external __ZTVN2JS14CompileOptionsE
the second:
  (__DATA,__datacoal_nt) weak external __ZTVN2JS14CompileOptionsE

That's the vtable for CompileOptions.

This mismatch is the cause of the "bad codegen, pointer diff" error.

We don't see the problem without exact rooting because CompileOptions is apparently boring enough to not emit its vtable when used outside of js/src. When you turn on exact rooting, Rooted<> gains a destructor that does stuff, and for whatever reason that's enough to tip over the users (eg dom/workers/WorkerPrivate.cpp) into emitting the vtable. (If you leave everything the same but just delete the destructor, the vtable will not be emitted.) If it isn't emitted, then it has no chance to clash with other definitions of that symbol.

So now the question is what should the visibility be, and how should we make it that way?

CompileOptions is declared with JS_PUBLIC_API(). Outside the engine, that results in default visibility. Inside the engine, we're building as a static lib, and that skips any visibility overriding:

/*
 * The linkage of JS API functions differs depending on whether the file is
 * used within the JS library or not. Any source file within the JS
 * interpreter should define EXPORT_JS_API whereas any client of the library
 * should not. STATIC_JS_API is used to build JS as a static library.
 */
#if defined(STATIC_JS_API)
#  define JS_PUBLIC_API(t)   t
#  define JS_PUBLIC_DATA(t)  t
#elif defined(EXPORT_JS_API) || defined(STATIC_EXPORTABLE_JS_API)
#  define JS_PUBLIC_API(t)   MOZ_EXPORT t
#  define JS_PUBLIC_DATA(t)  MOZ_EXPORT t
#else
#  define JS_PUBLIC_API(t)   MOZ_IMPORT_API t
#  define JS_PUBLIC_DATA(t)  MOZ_IMPORT_DATA t
#endif

This seems strange to me -- if you're declaring something to be part of the public API, then why would you hide it even if you're linking it into a static library? But I totally don't understand this stuff.

Comment 2

[Steve Fink [:sfink] [:s:]]

Oh, wait. I'm wondering if types should use JS_EXTERN_API, and only functions should use JS_PUBLIC_API?

Comment 3

[Steve Fink [:sfink] [:s:]]

So switching

  class JS_EXPORT_API(CompileOptions)

to be

  class MOZ_EXPORT CompileOptions

made both libjs_static.a and (eg) WorkerPrivate.o refer to the vtable as "weak external", and that allowed XUL to link successfully.

I'm still unsure as to what the correct fix is, though. Should both be "weak private external", or should both be "weak external"? I don't know if you can use a private symbol from a static lib like libjs_static.a.

Comment 4

[Steve Fink [:sfink] [:s:]]

I think this might be regression-ish from bug 920731. We're intentionally not exporting the JSAPI to addons. It seems like some parts of it may be no longer exported to Gecko as a result.

I'm not confident enough of what the right thing is here. If there's a way to make Gecko declare it to be "private" and still link against it, that would be good. Otherwise, it seems like we need a separate #define for this situation. I just don't know what to call it. JS_PUBLIC_CLASS?

/me waits for glandium.

Comment 5

[Steve Fink [:sfink] [:s:]]

Or we could call it JS_FRIEND_API?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Steve Fink [:sfink] from comment #4)
> I think this might be regression-ish from bug 920731.

That can easily be verified by removing MOZ_DISABLE_EXPORT_JS=1 from browser/confvars.sh. 

(In reply to Steve Fink [:sfink] from comment #5)
> Or we could call it JS_FRIEND_API?

There is already a JS_FRIEND_API. And I guess that's actually what you want to use for those symbols. If you absolutely don't want those symbols exported, then we'd need to build gecko with -DSTATIC_JS_API, which sounds awful.

Kind of relatedly, note that these non exported APIs that gecko uses are a timebomb for c++ unit tests using them, because they need to link against the static js engine while they *also* link against libxul, which contains another copy of the static js engine. That's an already existing problem, but it's something to keep in mind when adding new "friend" APIs.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Make JS_FRIEND_API always export symbols with default visibility

Ok, then perhaps this *is* landable as-is! This patch showed some failures in https://tbpl.mozilla.org/?tree=Try&rev=c880fd3a5d7a but I'm guessing they're all bogus because I based it on a broken inbound pull.

I rebased for https://tbpl.mozilla.org/?tree=Try&rev=aa801fb852ef but I haven't checked its parent rev either.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8360014 [details] [diff] [review]
Make JS_FRIEND_API always export symbols with default visibility

Review of attachment 8360014 [details] [diff] [review]:
-----------------------------------------------------------------

Please file a followup to remove linking js into C++ unit tests that are using friend apis, since they would now be exported.

::: js/src/jstypes.h
@@ +82,5 @@
> +#  define JS_FRIEND_DATA(t)   MOZ_EXPORT t
> +#else
> +#  define JS_FRIEND_API(t)   MOZ_IMPORT_API t
> +#  define JS_FRIEND_DATA(t)  MOZ_IMPORT_DATA t
> +#endif

Alternatively, you could:
#if defined(STATIC_JS_API)
#  define JS_FRIEND_API(t) MOZ_EXPORT t
(...)
#else
#  define JS_FRIEND_API(t) JS_PUBLIC_API(t)
(...)
#endif

Comment 9

[Mike Hommey [:glandium]]

Although, note this goes against bug 920731.

Comment 10

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e73050ee8c0e

Comment 11

[Steve Fink [:sfink] [:s:]]

(In reply to Mike Hommey [:glandium] from comment #8)
> Alternatively, you could:
> #if defined(STATIC_JS_API)
> #  define JS_FRIEND_API(t) MOZ_EXPORT t
> (...)
> #else
> #  define JS_FRIEND_API(t) JS_PUBLIC_API(t)
> (...)
> #endif

That was my first thought too, but it seems like whenever I'm looking at this, I always end up wanting to resolve it down to exactly what it does. That would add an extra level of indirection to trace through. It may be more logically sensible, though.

(In reply to Mike Hommey [:glandium] from comment #9)
> Although, note this goes against bug 920731.

Yes, it does. I figure that for the specific things I care about here, it really doesn't matter, since you can't do much of anything with a CompileOptions when things like JS_CompileScript aren't accessible.

However, this does expose all of jsfriendapi.h in addition. That... er... well, that sucks. I don't think it'll be too much of an issue when the main JSAPI is inaccessible, so it's pretty obvious to addon authors that they aren't supposed to get at it. But it does open up the possibility for all kinds of bizarre breakage, since the jsfriendapi.h stuff *really* shouldn't be used.

I'll file a bug. I want to land this now to unblock exact rooting, but it's probably better to split out a new #define for this purpose. ("JS_PUBLIC_API", maybe?)

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e73050ee8c0e