154206 - hack to make existing linux flashplayer & real binaries work with gcc 3.1 builds

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

On x86 linux, the current flashplayer plugin (version 5.0r48) expects 
a couple of builtin symbols from libgcc which were available in some older
versions of gcc.  However, they're _NOT_ available in gcc 3.1, so if we 
want that plugin to work with a gcc-3.1 built binary, we need to provide these
symbols.

It turns out that all the rest of the C++ goop in flashplayer is linked
statically, so there are no ABI issues here.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1

First cut at a patch.  In some sense, this code should really live in
nsPluginsDirUnix.cpp.  However, when I put it there, the symbols show up in the
.so as expected (according to nm), but ld.so doesn't link them to
flashplayer.so for whatever reason.  Putting it in nsAppRunner.cpp, like this
patch does, works.

Comments?  Does anyone object too strenuously to a hack of this sort being
checked in?

Comment 2

[Gilles Durys]

*** Bug 154205 has been marked as a duplicate of this bug. ***

Comment 3

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

Comment on attachment 89120 [details] [diff] [review]
patch, v1

>+    if (aPtr)
>+      free(aPtr);

Can you use something that doesn't make this a mismatch with your
__builtin_vec_new?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

dbaron: that's actually the exact way that __builtin_vec_new in the old libgcc2
does it.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: code from egcs 1.1.2, for comparison

Comment 6

[hacker formerly known as [e-mail]]

I think that components-version-script is redefining those __builtin_vec symbols
to be local to the plugin dll and that's why the plugin can't find them.  
Can you try disabling that script (or adding those symbols to the script) and
placing the code snippet back in nsPluginsDirUnix.cpp?  

I think we need a better configure name than old-plugins-hack but I can't think
of one atm.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

I actually built with --enable-debug (in addition to --enable-optimize), so
components-version-script is not even getting run.  Reading the linker man page
sure made it sound promising, though, so I tried adding -Wl,-E to the link line,
but that didn't help either.

Next I turned on ld.so debugging, and it appears that the dynamic linker only
attempts to bind symbols in flashplayer.so to (the transitive closure of) the
libraries directly linked to the final executable (mozilla-bin).  I played with
a bunch of linker switches, and was unable to find any that would change this
behavior.  Rather than sinking more time into this, I'm inclined to leave the
hack in nsAppRunner.cpp.

I've been thinking over the mismatched allocator thing.  On the one hand, it
makes me pretty uncomfortable, because I wonder if the reason it happens to work
in egcs is because of some underlying guarantees that I don't know about due to
the fact that it lives in a compiler-internal place.  Or maybe it never worked.
 On the other hand, I'm a little leery of changing the semantics by making the
allocators match.  Any opinions on this one?

As far as autoconf feature names, I'm happy to take suggestions, but I haven't
been able to think of one that's both succinct enough and high-level enough.

Comment 8

[Bradley Baetz (:bbaetz)]

Oh.

There was a very large thread on this late last month on the gcc mailing list. See 
http://gcc.gnu.org/ml/gcc/2002-05, specifically the threads 'duplicate data
objects in shared libraries', 'Treat RTLD_LOCAL like Solaris', 'Minimal
GCC/Linux shared lib + EH bug example', and a few others. The most important
ones from this POV were probably on
htp://sources.redhat.com/ml/libc-alpha/2002-05/ too, but the backround is
interesting too. I don't know what conclusion (if any) was reached, though.

However, (assuming that the behaviour in libc stays as is) what if the
mozilla-bin binary wasn't linked against libstdc++? Ie, instead of this patch,
compile with CXX=gcc, or try '-nostdlib', and add -lgcc -lc to the link flags.
You may need to add -static-libgcc in either or both cases.

This would then require the old libstdc++ library  to be present on the user's
system for flash to work - I don't think that that is an issue, though.

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

bbaetz: I didn't read all those threads, but I think I read enough to conclude
that they're discussing a problem that is very closely-related to what I'm
seeing, but not exactly the same.  In particular, Jason Merrill's summary at 
http://gcc.gnu.org/ml/gcc/2002-05/msg01970.html seems to support this.  For one
thing, I suspect that our component loader isn't causing RTLD_NOW to be used,
anyhow.  But it has given me some ideas about other things to try.  It would be
nice to have this in the plugins library, because then embeddors (eg Galeon)
could get this hack automagically as well.

As far as the linking without libstdc++ stuff goes, I've been under the
impression that our portability man (scc) is interested in seeing Mozilla move
towards using libstdc++ more in the app, rather than less.  So I'm not convinced
this is worth spending time on.

See also http://hints.linuxfromscratch.org/hints/mozilla.txt as well as bug 
124006 (which this appears to be a DUP of).

Comment 10

[Bradley Baetz (:bbaetz)]

This is a dupe of several bugs...

libstdc++ has explicitly refused to freeze the binary API yet, so I suspect that
that would be a losing idea.

If this is in the plugin api, though, what about out code which calls
__builtin_vector_new? Won't we pick up that definition? thers not even a
guarantee that the memory allocators are similar, although since libstdc++ uses
libc's, that is true on linux, but I don't think its true on all unix platforms.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

> libstdc++ has explicitly refused to freeze the binary API yet, so I suspect
> that that would be a losing idea.

Well, we'd be no worse off than we are today.  

Additionally, we could statically link about libstdc++ (since we think that's
finally allowable from a license point of view), and hope that by the time
there's enough usage in Mozilla to add a significant amount of bloat, libstdc++
will finally have frozen.

In any case, building without libstdc++ is hardly a supported configuration
today, so I'm inclined to not worry about this now.  If we really intend to make
this a support configuration, let's file a separate bug for that.

> If this is in the plugin api, though, what about out code which calls
> __builtin_vector_new? Won't we pick up that definition? 

gcc 3 doesn't have __builtin_vec_new and friends.  Also note the configure test
which explicitly prevents this from happening.

> thers not even a guarantee that the memory allocators are similar, although 
> since libstdc++ uses libc's, that is true on linux, but I don't think its true
> on all unix platforms.

This hack is only enabled by default on linux anyway, so I don't think that's
likely to be a problem.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Various minor tweaks, and changed the option name from "old-plugins-hack" to
"compat-mem-wrappers".	The idea here is that with the old-plugins-hack name,
folks might have beeen tempted to turn it on other platforms without actually
knowing what it does ("yeah, i want old plugins to work").  Anyone here care to
r / sr ?

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v3

This version now includes a few more symbols to make the RealPlayer 8 plugin
happy as well.	Renamed the configure option yet again to accomodate these new
symbols.

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 90140 [details] [diff] [review]
patch, v3

r=bryner

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4

Fix syntax error introduced in previous patch rev.

Comment 16

[serge (gone)]

Comment on attachment 90443 [details] [diff] [review]
patch, v4

r=serge

Comment 17

[Christopher Blizzard (:blizzard)]

Comment on attachment 90443 [details] [diff] [review]
patch, v4

Is this on by default?	My reading is that it is.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

blizzard: it's on by default for x86 linux only.  and then only if the relevant
symbols are missing at compile time.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 124006 has been marked as a duplicate of this bug. ***

Comment 20

[Christopher Blizzard (:blizzard)]

Comment on attachment 90443 [details] [diff] [review]
patch, v4

sr=blizzard

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v5

I misunderstood the way autoconf creates HAVE_* #defines, so the last patch
failed to link under egcs 1.1.2.  This version corrects that, and seems to do
the right thing on egcs 1.1.2, gcc 2.96, and gcc 3.1.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 91154 [details] [diff] [review]
patch, v5

The only change between the last rev and this one was the symbol names in the
#ifndef statements.  Carrying forward r= and sr=.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 91154 [details] [diff] [review]
patch, v5

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.

Comment 25

[serge (gone)]

*** Bug 91470 has been marked as a duplicate of this bug. ***

Comment 26

[shrirang khanzode]

.

Comment 27

[Jason Clinton]

I just noticed that the Java Sun people have FINALLY (March 13, '03) realized
this is a problem and are going to begin distributing a GCC 3.2 JRE.

See here:
http://developer.java.sun.com/developer/bugParade/bugs/4687814.html

Yay!