Closed Bug 336083 Opened 19 years ago Closed 19 years ago

visibility issues with XPCOM glue

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: dbaron, Assigned: benjamin)

Details

I created a component that uses XPCOM glue using the strategy variously called dynamic or dependent in http://developer.mozilla.org/en/docs/XPCOM_Glue . I then ran readelf on this component, and found that it exports all of the XPCOM symbols in the glue. That doesn't seem like an ideal situation to me -- it seems like something that could be fixed by having xpcom/glue/Makefile.in define XPCOM_GLUE just like xpcom/standalone/glue/Makefile.in . It seems to me that this change should perhaps have been a part bug 266006. Before that point, xpcom/glue/ was where we built our internal copy of the glue; after that point, what was built in xpcom/glue/ (not xpcom/glue/standalone/) was what we linked into libxpcom.so (now libxpcom_core.so). I don't know whether or not this could actually cause the symbols to be used outside of the component that uses the dependent glue (if so, it could cause tons of weird bugs), but it certainly makes components using the dependent glue bigger and slower than they need to be.
It looks like most uses of the XPCOM_GLUE ifdef other than the ones in nscore.h really do mean standalone-glue-only, so perhaps a new XPCOM_DYNAMIC_GLUE macro would be better, with appropriate changes to nscore.h.
Then again, the visibility stuff wasn't there at all until recent changes for bug 305949.
No longer blocks: 266006
We don't need another macro: the lack of MOZILLA_INTERNAL_API and XPCOM_GLUE implies the dependent glue. I'll look through the macros more closely on Tuesday, I may have missed a change.
Assignee: nobody → benjamin
In that case, would you want this change in nscore.h ? #ifdef _IMPL_NS_COM #define XPCOM_API(type) EXPORT_XPCOM_API(type) -#elif defined(XPCOM_GLUE) +#elif defined(XPCOM_GLUE) || !defined(MOZILLA_INTERNAL_API) #define XPCOM_API(type) GLUE_XPCOM_API(type) #else #define XPCOM_API(type) IMPORT_XPCOM_API(type) #endif
No, that would change the linkage of the exported API. We need to change the linkage of NS_COM_GLUE.
Ah, right. Ugh. So this can't be fixed by tweaking existing macros, since NS_COM_GLUE goes in places that NS_HIDDEN can't. Effectively you need XPCOM_GLUE_API(type) macro, but of course that still doesn't fix vtables, and such a macro wouldn't work for constructors even though they can use NS_HIDDEN. Which basically means you need NS_HIDDEN_IF_XPCOM_GLUE (or something with a better name that does the same thing). Or we could just wait for solid support for visibility pragmas, although getting that in lowest-common-denominator compilers for binary-compatible extensions might be a while.
Summary: visibility issues with dynamic/dependent XPCOM glue → visibility issues with XPCOM glue
(I *think* __attribute__((visibility("hidden"))) applies to classes in newer gccs though -- we could have an autoconf test for that and use it in the newer compilers. And we could use it for NS_NO_VTABLE too.)
This is in fact fixed by the nscore.h bits in bug 334038, which I thought I had already checked in as part of an earlier bug.
Does that compile on gcc 3.2?
I believe so. The HAVE_VISIBILITY_ATTRIBUTE and HAVE_VISIBILITY_HIDDEN_ATTRIBUTE configure tests are separate; class-level visibility was added at the same time as visibility("default"), so the HAVE_VISIBILITY_ATTRIBUTE test should cover that case.
Are you sure? Class-level visibility was added in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=9283 (/ http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15000 ), and the early comments there seem to indicate that default already existed.
I don't know of a released GCC with visibility(default) but not class-level visibility... 4.0 had both, and the redhat 3.4 had neither.
I'm going to claim this is fixed, at least for gcc4. Reopen if there is evidence to the contrary.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.