714967 - Export mozilla::services::Get* to external consumers as well

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Wood (:Callek)]

Attachment: v1

The work I am doing in Bug 560772 to make comm-central use mozilla::services will work fine, in internal linkage as per current libxul based code.

I wish to make this API exposed to external code, such that we can link against libxul and use it.

There is a currently supported configuration that Neil (NeilAway) wants to continue to support, that has mailnews built as a separate library linking against libxul rather than *with* libxul. The workaround of doing a wrapper library or additional Macro's for this, I feel is overkill and a bad idea.

I'm not advocating the use of this API for 3rd party consumers, (the do_GetService etc is a better API for those uses) however for our case of primarily internal-to-libxul code, but must-be-available for external linkage, warrants my addition here.

I pushed a patch to try for this at https://tbpl.mozilla.org/?tree=Try&rev=692b8f72d587 and will follow up with results here soon.

Ben, if you don't think you'll get to the review soon can you name a delegate, and if this needs sr, let me know whom to request from, if not you.

Comment 1

[Benjamin Smedberg]

The reason we did not do this before, and why I don't want to do it unconditionally now, is that it means that every function which uses mozilla::services within libxul will end up with relocations, and relocations hurt our startup time.

So I don't *think* I want to accept this patch. cc'ing glandium for his thoughts.

Comment 2

[Nathan Froyd [:froydnj]]

Exporting functions doesn't mean adding relocations, it means that those functions are now called through PLT entries.  That hurts because there is now some extra overhead with each call, and (on x86 Linux, at least), you need to sacrifice %ebx in functions where it wasn't already being used for the GOT pointer.

Comment 3

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

So do the standard trick here and have different symbol names for internal and external code.

Comment 4

[Justin Wood (:Callek)]

Attachment: v2

Alternate approach, suggested by khuey on IRC... I have not yet tested this though... so if there is a simple gotchya feel free to point it out.

Comment 5

[Justin Wood (:Callek)]

Attachment: v2.1

v2 approach, with a typo fixed.

Comment 6

[Justin Wood (:Callek)]

Attachment: v2.2

One more catch... if you want v1, I need to fix it to have an NS_IMPORT_ in external users as well though.

Comment 7

[Mike Hommey [:glandium]]

A small note: I don't know about MSVC and OSX's linker, but on Linux, we could use -Wl,-Bsymbolic-functions to make internal calls not go through the PLT, making the #define trick unnecessary (and possibly improving things for all the other exported stuff). We could even use -Bsymbolic, which does more than functions.

Comment 8

[Benjamin Smedberg]

How would external code *use* this API? It looks like the code would actually end up calling
mozilla::services::_external_GetPrefService, which sounds un-ideal.

Could we instead use a namespace like mozilla::services::external::GetPrefService or even mozilla::external::services::GetPrefService. The latter might be better because you'd end up with something like

using mozilla::external;
And then you'd just be calling services::GetPrefService.

Also, you should be using XPCOM_API(already_AddRefed<TYPE>) instead of NS_IMPORT and NS_EXPORT, which will suppress warnings from the linker about exporting an imported function.

Comment 9

[Justin Wood (:Callek)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Also, you should be using XPCOM_API(already_AddRefed<TYPE>) instead of
> NS_IMPORT and NS_EXPORT, which will suppress warnings from the linker about
> exporting an imported function.

After chatting on IRC you were ok with this, if I switched to XPCOM_API, however I noticed that XPCOM_API uses extern "C" which makes this bad for external use like this.

http://mxr.mozilla.org/comm-central/source/mozilla/xpcom/base/nscore.h#261

Can you confirm that this is either ok, or if my patch is ok-as-is given that, (note I do NS_IMPORT_ and NS_EXPORT_ properly based on internal api) or if you know of another suiteable macro.

Comment 10

[Benjamin Smedberg]

Comment on attachment 585985 [details] [diff] [review]
v2.2

I misread this patch completely. I wish we had something better than macros. I think this is ok.

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 585985 [details] [diff] [review]
v2.2

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

::: xpcom/build/Services.h
@@ +67,5 @@
> +        return _external_Get##NAME();                               \
> +    }
> +
> +#include "ServiceList.h"
> +#undef MOZ_SERVICE

Nit: is it actually needed to duplicate these two lines?

Comment 12

[Benjamin Smedberg]

It's better to, because then your define/undefine of MOZ_SERVICE match.

Comment 13

[Justin Wood (:Callek)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> Comment on attachment 585985 [details] [diff] [review]
> v2.2
> 

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

Comment 14

[Justin Wood (:Callek)]

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

Comment 15

[[e-mail]]

This exports the symbols from xul itself, rather than any of the other various libraries that we've historically been linking to. Is that a reaonsable thing to do? If so, what's the Makefile magic to link to xul?