Closed
Bug 271630
Opened 21 years ago
Closed 21 years ago
XPCOM glue broken on Linux/PPC [monkeypox orange]
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta1
People
(Reporter: darin.moz, Assigned: darin.moz)
References
Details
(Keywords: crash, regression)
Attachments
(1 file)
21.02 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
XPCOM glue broken on Linux/PPC
The recent change to NS_GetFrozenFunctions causes trouble for applications using
the XPCOM glue. The problem occurs with the resolution of the non-"_P" symbols.
For example, NS_CStringContainerInit is defined both in the XPCOM glue library
and in the XPCOM stub library. As a result, when we call NS_GetFrozenFunctions,
we return the address of the NS_CStringContainerInit found in the XPCOM glue
library. This results in infinite recursion when NS_CStringContainerInit is called.
This explains why monkeypox is orange.
Since monkeypox is using GCC 2.95.2, there is no way to specify hidden
visibility. So, if we want to continue supporting that target, I think the only
solution is to avoid using the same name for these functions in both libraries.
Perhaps the best solution would be to move nsStringAPI.cpp into xpcom_core
and use a similar _P naming trick. We'd probably want to do this as part of
LibXUL anyways so we don't have to export the string classes.
Assignee | ||
Updated•21 years ago
|
Assignee | ||
Comment 1•21 years ago
|
||
Assignee | ||
Comment 2•21 years ago
|
||
Comment on attachment 166967 [details] [diff] [review]
v1 patch
contrary to what the diff may indicate, i'm actually just resurrecting
xpcom/build/nsStringAPI.cpp ;-)
I also intend to eventually mark all of the nsXPCOM.h functions as hidden when
XPCOM_GLUE is defined. This patch makes that change for nsStringAPI.h, but not
the others. I'll do the rest as a followup patch in another bug.
Attachment #166967 -
Flags: review?(bsmedberg)
Comment 3•21 years ago
|
||
Comment on attachment 166967 [details] [diff] [review]
v1 patch
I don't like it. The extra level of call indirection is going to hurt future
performance visibly. Instead, let's rename the *glue* functions using a define:
#ifdef XPCOM_GLUE
#define NS_CStringContainer_Init NS_CStringContainer_Init_Glue
etc...
This solves the problem with non-hidden symbols conflicting, without forcing
the extra layer of indirection.
Attachment #166967 -
Flags: review?(bsmedberg) → review-
Assignee | ||
Comment 4•21 years ago
|
||
bsmedberg: we can't change the names of symbols that already exist in people's
programs. moreover, i thought you wanted to make libxul only export frozen
symbols? as is, it will have to export portions of the internal string library.
Comment 5•21 years ago
|
||
Are we going to export the string API from libxul as frozen exports? What are
the symbol names going to be?
I want firefox, by version 1.5, to use only the frozen string API. The patch in
bug 270110 is a great step toward this, because it doesn't require drastic code
changes. However, it is an unacceptable performance penalty for every string
call to go through NS_StringFunc -> NS_StringFunc_P.
Since libxpcom.so depends on libxpcom_core.so, why can't we put all the frozen
exports in libxpcom_core.so, except for the one required by glue
(NS_GetFrozenFunctions)? Is there any previous code which we need to support
that dynamically links (using dlopen, dlsym) to other funtions in libxpcom.so ?
Assignee | ||
Comment 6•21 years ago
|
||
> Are we going to export the string API from libxul as frozen exports? What are
> the symbol names going to be?
I'm not sure how we are going to deal with this. Obviously, with libxul we are
starting with a clean slate. The only requirement is that we maintain the
frozen set of entry points into libxpcom.so. Existing embeddings and components
should not be broken.
> I want firefox, by version 1.5, to use only the frozen string API. The patch
> in bug 270110 is a great step toward this, because it doesn't require drastic
> code changes. However, it is an unacceptable performance penalty for every
> string call to go through NS_StringFunc -> NS_StringFunc_P.
Then perhaps Firefox should link directly to libxul... hmm...
> Since libxpcom.so depends on libxpcom_core.so, why can't we put all the
> frozen exports in libxpcom_core.so, except for the one required by glue
> (NS_GetFrozenFunctions)? Is there any previous code which we need to support
> that dynamically links (using dlopen, dlsym) to other funtions in libxpcom.so > ?
There are existing components and embedding applications that depend on the
frozen set of entry points into libxpcom.so. We cannot mess with those symbols.
If an embedding application or component links to non-frozen symbols in
libxpcom.so (from Moz 1.7), then it is already broken by the creation of
libxpcom_core.so.
To make things simple, I'd just start with having libxul be a private,
implementation detail. I'd expose libxpcom as the interface into libxul. If we
find that the overhead of thunking through libxpcom is unacceptable then we find
a way to optimize it. I don't think it will be a big problem.
Assignee | ||
Comment 7•21 years ago
|
||
bsmedberg:
I took a look at the code MSVC 6 generates for NS_GetComponentManager in
XPCOM.dll today, and it consists of a single jmp statement:
100011A1: FF 25 24 20 00 10 jmp dword ptr ds:[10002024h]
The jump-to address corresponds to the relocation table for xpcom_core.dll. I
would be surprised if we see this register on the performance meter. IIRC, GCC
employs a similar optimization.
Assignee | ||
Comment 8•21 years ago
|
||
Comment on attachment 166967 [details] [diff] [review]
v1 patch
re-requesting review.
Attachment #166967 -
Flags: review- → review?(bsmedberg)
Comment 9•21 years ago
|
||
Comment on attachment 166967 [details] [diff] [review]
v1 patch
ok
Attachment #166967 -
Flags: review?(bsmedberg) → review+
Assignee | ||
Comment 10•21 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•21 years ago
|
||
reopening since monkeypox is still orange :(
investigating...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•21 years ago
|
||
ah, monkeypox had not been updated yet in accordance with the new build rules.
so, it was not pulling SeamonkeyAll :-(
i went ahead and added MOZ_CO_PROJECT=suite and --enable-application=suite
bsmedberg: have you verified that all the rest of the tinderboxen are up-to-date?
i mark this bug fixed once i see monkeypox cycle green.
Assignee | ||
Comment 13•21 years ago
|
||
marking FIXED again since monkeypox finally did turn green as expected.
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•