271630 - XPCOM glue broken on Linux/PPC [monkeypox orange]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Darin Fisher]

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.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

[Darin Fisher]

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.

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Darin Fisher]

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

[Benjamin Smedberg]

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 ?

Comment 6

[Darin Fisher]

> 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.

Comment 7

[Darin Fisher]

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.

Comment 8

[Darin Fisher]

Comment on attachment 166967 [details] [diff] [review]
v1 patch

re-requesting review.

Comment 9

[Benjamin Smedberg]

Comment on attachment 166967 [details] [diff] [review]
v1 patch

ok

Comment 10

[Darin Fisher]

fixed-on-trunk

Comment 11

[Darin Fisher]

reopening since monkeypox is still orange :(

investigating...

Comment 12

[Darin Fisher]

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.

Comment 13

[Darin Fisher]

marking FIXED again since monkeypox finally did turn green as expected.