168584 - XPCOM Glue Is broken on linux

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

The linux symbol loader is getting confused on the way NS_GetFrozenFunctions is
implemented.  

The way glue is suppose to work is that the functions defined in nsXPCOM.h are
stubbed out and are linked into the component or embedding application.  At
startup we the glue code calls NS_GetFrozenFunctions and set the stubbed out
code to call the appropriate functions.  So, basically there is a dynamic look
up of symbols so that clients do not have to link to xpcom.

The problem:  The loader is using the address of the function defined in the
application, not the "real" function defined by xpcom.  

I spoke to Drepper about this and he suggested the following patch.

Comment 1

[Doug Turner (:dougt)]

Attachment: proposed patch v.1

Comment 2

[Alec Flett]

Comment on attachment 99151 [details] [diff] [review]
proposed patch v.1

sr=alecf

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 99151 [details] [diff] [review]
proposed patch v.1

r=bryner

Comment 4

[Doug Turner (:dougt)]

Checking in nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.158; previous revision: 1.157
done

Thanks

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: possible fix for OS/2 bustage

This patch will, I think, fix the OS/2 bustage (although I don't understand why
OS/2 wasn't busted before with the old code).  The first change is a warning
fix.

Is there a reason |internal_UnregisterXPCOMExitRoutine| is (without this patch)
|nsresult NS_COM| rather than |static nsresult| like the rest of the new
functions?

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 99219 [details] [diff] [review]
possible fix for OS/2 bustage

no.

Patch looks good.  r=dougt

Comment 7

[Doug Turner (:dougt)]

I fixed up the unregister method.  dbaron, what platform are you seeing problems
with the PRStaticLinkTable?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

gcc gives a warning (about a partially-bracketed initializer)

Comment 9

[Doug Turner (:dougt)]

I checked in that fix for the warning..  that chris and dbaron.

Comment 10

[Doug Turner (:dougt)]

I think that there is a better way.  wtc pointed me at this:

    -B symbolic  In dynamic mode only, when  building  a  shared
                 object,  bind  references  to  global symbols to
                 their definitions within the object, if  defini-
                 tions  are  available.   Normally, references to
                 global symbols within  shared  objects  are  not
                 bound  until  runtime,  even  if definitions are
                 available, so that definitions of the same  sym-
                 bol in an executable or other shared objects can
                 override the object's own definition.   ld  will
                 issue  warnings  for undefined symbols unless -z
                 defs overrides.


I think that this is the way we need to be building the xpcom library to support
glue.  


Chris, do you foresee any problems with using IS_COMPONENT = 1 when building
XPCOM?  

Comment 11

[hacker formerly known as [e-mail]]

Yep, using IS_COMPONENT=1 causes all export symbols to be stripped out of the
library except NSGetModule & friends.  And I'd rather not start adding an
exception to the rules used by IS_COMPONENT to work around non-component libraries.

And...wasn't wtc the one who pointed out that using -Bsymbolic for non-component
libraries would cause problems for libraries that allowed the user to
dynamically load symbols to override the defaults used by the library.  In
particular, overriding the use malloc used by xpcom & nspr was brought up. 
*digging* See bug 76710.

Comment 12

[Doug Turner (:dougt)]

that is too bad... 

Comment 13

[Doug Turner (:dougt)]

Attachment: more love

fixes more linux problems.  after talking with wtc, i think that this is the
best solution.

Comment 14

[Alec Flett]

Comment on attachment 99743 [details] [diff] [review]
more love

sr=alecf

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 99743 [details] [diff] [review]
more love

this patch will break compatiblity if anyone was using dynamic symbol lookup on
windows.  i doubt anyone was.

Comment 16

[rpotts (gone)]

hey doug,

i'm wondering if we want each function table to hold a strong reference to the
xpcom DLL.  so that the DLL cannot be unloaded out from under them...

this would require a 'glue shutdown' routine that does the PR_FreeLibrary() call
on  xpcom...

i'm just afraid that if someone called NS_ShutdownXPCOM(...) (and the dlls were
actually unloaded) then any glue code would just start crashing ...

what do you think?

-- rick

Comment 17

[Doug Turner (:dougt)]

The assumption here is that the glue code has already called PR_LoadLibrary on
the xpcom library.  If you look at nsXPCOMGlue.cpp, the XPCOMGlueStartup
function calls  PR_LoadLibrary and XPCOMGlueShutdown calls PR_UnloadLibrary. 
Since only the glue code should call NS_GetFrozenFunctions, I didn't think we
needed to have the function table hold a reference to the library.  

Comment 18

[rpotts (gone)]

Comment on attachment 99743 [details] [diff] [review]
more love

sr=[e-mail]

Comment 19

[Doug Turner (:dougt)]

this still has problems.  we need to build xpcom with -B symbolic.

Comment 20

[Doug Turner (:dougt)]

Attachment: 60's style lovefest

assumes -B symbolic support on linux.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 100303 [details] [diff] [review]
60's style lovefest

couple of notes:
ingore the xpcom/io change.
Most of thie patch backs out changes caused by previous patches.
We now correctly support XPCOM Glue in the case of generic modules.

Comment 22

[Alec Flett]

I'm so lost. It feels like we're right back where we started? 
Can you give us a quick summary on where we were, what worked when, and where we
are now?

Comment 23

[hacker formerly known as [e-mail]]

Attachment: Reuse existing ifdefs to add -Bsymbolic

Comment 24

[Doug Turner (:dougt)]

Well, the short story is that if we link XPCOM with a different option, then the
linker will resolve something like &FOO locally relative to the object file
rather than jumping to the application global symbol table.

I have tried just about everything to try to support XPCOM Glue without having
to change the link option (well, that isn't really true.  I didn't know about
this link option until recently).  Nonetheless, I finally got stuck when I had
to support component that had no idea where the XPCOM library actually exists on
disk.  The solution before this point was to just use PR_FindSymbol.

Now, I thought I could support these component using the static library support
that nspr has.  (this support allows you to name a function pointer then later
request them with PR_FindSymbol).  The problem with using this support is that
you still need to take the address of the function which you wanted named.
Chicken and Egg.

Sorry for the snipe hunt, but cls's patch and mine will do the trick.  

Comment 25

[Alec Flett]

Comment on attachment 100303 [details] [diff] [review]
60's style lovefest

dougt: cool, thanks for the explanation.

sr=alecf

Comment 26

[Doug Turner (:dougt)]

okay okay, I think that I another solution that avoids having to compile xpcom
with -Bsymbolic!

The problem that forced me into suggesting -Bsymbolic was that there are some
components that don't know where the XPCOM Library is.  If these components
don't know where XPCOM lib lives, they can't do call PR_LoadLibrary.  

So, the alternative is to provide these component the path (nsIFile) to the
xpcom library.  I think this is a much better way to do things.  

Comment 27

[Doug Turner (:dougt)]

oh.  Colin tells me -Bsymbolic will kill OpenVMS.

Comment 28

[Doug Turner (:dougt)]

Attachment: 60's are over.

This is a much better solution for components.

Comment 29

[rpotts (gone)]

Comment on attachment 100444 [details] [diff] [review]
60's are over.

sr=[e-mail]

Comment 30

[hacker formerly known as [e-mail]]

Comment on attachment 100444 [details] [diff] [review]
60's are over.

+// seawood tells me there isn't a better way...

You could define XPCOM_DLL in configure.in which would handle any $(LIB_PREFIX)
discrepancies.	Probably not necessary unless you really don't want to hardcode
those values into the source.

However, you will need to use MOZ_DLL_SUFFIX instead of hardcoding .so
otherwise you'll break at least hpux & osx mach-o builds.

Comment 31

[Doug Turner (:dougt)]

hmm. this code was copied from another source file.  I just assumed that it was
okay.  My bad.

So, how exactly do I get XPCOM_DLL define via configure.in?  Or should I just
add #ifdefs for hpux & osx mach-o?

Comment 32

[hacker formerly known as [e-mail]]

If you want to define XPCOM_DLL in configure.in, you should follow the method
used by WIDGET_DLL.  Otherwise, you could just fix that XPCOM_DLL define by
replacing the ".so" with MOZ_DLL_SUFFIX (see lxr).  Separate ifdefs should not
be needed.

Comment 33

[Doug Turner (:dougt)]

Attachment: seawood's comment

Just a new #define for the default xpcom library name per chris.

Comment 34

[Doug Turner (:dougt)]

Comment on attachment 100582 [details] [diff] [review]
seawood's comment

carryover sr.

Comment 35

[Alec Flett]

Comment on attachment 100582 [details] [diff] [review]
seawood's comment

r=alecf

Comment 36

[Doug Turner (:dougt)]

Checking in build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.164; previous revision: 1.163
done
Checking in glue/nsGenericFactory.cpp;
/cvsroot/mozilla/xpcom/glue/nsGenericFactory.cpp,v  <--  nsGenericFactory.cpp
new revision: 1.46; previous revision: 1.45
done
Checking in glue/nsIGenericFactory.h;
/cvsroot/mozilla/xpcom/glue/nsIGenericFactory.h,v  <--  nsIGenericFactory.h
new revision: 1.33; previous revision: 1.32
done
Checking in glue/standalone/nsXPCOMGlue.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v  <--  nsXPCOMGlue.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in io/Makefile.in;
/cvsroot/mozilla/xpcom/io/Makefile.in,v  <--  Makefile.in
new revision: 1.69; previous revision: 1.68
done
Checking in io/nsDirectoryService.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryService.h,v  <--  nsDirectoryService.h
new revision: 1.22; previous revision: 1.21
done
Checking in io/nsDirectoryServiceDefs.h;
/cvsroot/mozilla/xpcom/io/nsDirectoryServiceDefs.h,v  <--  nsDirectoryServiceDefs.h
new revision: 1.19; previous revision: 1.18
done

finally.  ;-)

thanks all.