Created attachment 585564 [details] [diff] [review] 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.
Attachment #585564 - Flags: review?(benjamin)
Component: Build Config → XPCOM
Product: Toolkit → Core
QA Contact: build-config → xpcom
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.
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.
So do the standard trick here and have different symbol names for internal and external code.
Created attachment 585982 [details] [diff] [review] 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.
Created attachment 585983 [details] [diff] [review] v2.1 v2 approach, with a typo fixed.
Created attachment 585985 [details] [diff] [review] 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.
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.
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.
(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 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.
Attachment #585985 - Flags: review?(benjamin) → review+
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?
It's better to, because then your define/undefine of MOZ_SERVICE match.
(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
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla12
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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?
You need to log in before you can comment on or make changes to this bug.