XPCOM glue broken on Linux/PPC [monkeypox orange]

RESOLVED FIXED in mozilla1.8beta1

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

({crash, regression})

Trunk
mozilla1.8beta1
PowerPC
Linux
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

21.02 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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

13 years ago
Blocks: 267767
Severity: normal → major
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta
(Assignee)

Comment 1

13 years ago
Created attachment 166967 [details] [diff] [review]
v1 patch
(Assignee)

Comment 2

13 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

13 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

13 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

13 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

13 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

13 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

13 years ago
Comment on attachment 166967 [details] [diff] [review]
v1 patch

re-requesting review.
Attachment #166967 - Flags: review- → review?(bsmedberg)

Comment 9

13 years ago
Comment on attachment 166967 [details] [diff] [review]
v1 patch

ok
Attachment #166967 - Flags: review?(bsmedberg) → review+
(Assignee)

Comment 10

13 years ago
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

13 years ago
reopening since monkeypox is still orange :(

investigating...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 12

13 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

13 years ago
marking FIXED again since monkeypox finally did turn green as expected.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.