826567 - Make sure we don't have STB_GNU_UNIQUE symbols being exported by libxul.so

Status: NEW

(Firefox Build System :: General, defect)

Description

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I just noticed that libxul.so is never unloaded because it export

_ZN9__gnu_cxx21_Hashtable_prime_listImE16__stl_prime_listE

which is  STB_GNU_UNIQUE. It is coming from ipc/chromium/src/base/.

Comment 1

[Mike Hommey [:glandium]]

The only way to reliably avoid these is to use gold and link with --no-gnu-unique, or to rebuild gcc with --disable-gnu-unique-object.

FWIW, AFAICS, we don't export this symbol on our builds.

That being said, considering we dlclose libxul right before quitting, I don't see how that makes much difference that we export such symbol or not. As a matter of fact, doing exit() will dlclose it anyways.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Mike Hommey [:glandium] from comment #1)
> The only way to reliably avoid these is to use gold and link with
> --no-gnu-unique, or to rebuild gcc with --disable-gnu-unique-object.

I was thinking of a build test, like we have to libstdc++ symbol versions.

> FWIW, AFAICS, we don't export this symbol on our builds.
> 
> That being said, considering we dlclose libxul right before quitting, I
> don't see how that makes much difference that we export such symbol or not.
> As a matter of fact, doing exit() will dlclose it anyways.

The point is that dlclose does nothing right now because of this symbol, but yes, there is not much difference. Maybe it would be better to just not call dlclose?

Comment 3

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> > FWIW, AFAICS, we don't export this symbol on our builds.

I just tested the current nightly and we don't have this symbol, but we have:

0000000001fe7b67 u std::basic_string<char, std::char_traits<char>, pool_allocator<char> >::_Rep::_S_terminal
0000000002e93a20 u std::basic_string<char, std::char_traits<char>, pool_allocator<char> >::_Rep::_S_empty_rep_storage
0000000001f6ad50 u std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >::npos
0000000001f6ad58 u std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >::_Rep::_S_max_size
0000000001f6ad60 u std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >::_Rep::_S_terminal
0000000002e85c00 u std::basic_string<unsigned short, base::string16_char_traits, std::allocator<unsigned short> >::_Rep::_S_empty_rep_storage

Comment 4

[Mike Hommey [:glandium]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #2)
> (In reply to Mike Hommey [:glandium] from comment #1)
> > The only way to reliably avoid these is to use gold and link with
> > --no-gnu-unique, or to rebuild gcc with --disable-gnu-unique-object.
> 
> I was thinking of a build test, like we have to libstdc++ symbol versions.

But we're actively making things compatible with older libstdc++. Do we want to make things compatible with glibc < 2.11, and can we?

> > FWIW, AFAICS, we don't export this symbol on our builds.
> > 
> > That being said, considering we dlclose libxul right before quitting, I
> > don't see how that makes much difference that we export such symbol or not.
> > As a matter of fact, doing exit() will dlclose it anyways.
> 
> The point is that dlclose does nothing right now because of this symbol, but
> yes, there is not much difference. Maybe it would be better to just not call
> dlclose?

IIRC, bsmedberg was saying we should remove the XPCOMGlueShutdown calls.

Comment 5

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> > I was thinking of a build test, like we have to libstdc++ symbol versions.
> 
> But we're actively making things compatible with older libstdc++. Do we want
> to make things compatible with glibc < 2.11, and can we?

The point is is not added backward compatibility. It is just that this bugs gives us the worse of two options: we call dlclose, and it does nothing. It would be very surprising for someone changing a use of string or hash to suddenly see libxul.so being unloaded. 

> > The point is that dlclose does nothing right now because of this symbol, but
> > yes, there is not much difference. Maybe it would be better to just not call
> > dlclose?
> 
> IIRC, bsmedberg was saying we should remove the XPCOMGlueShutdown calls.

Cool! That would probably be the best. CCing him.

Comment 6

[Mike Hommey [:glandium]]

Here is something interesting: we have these symbols on 17.0.2esr while we didn't have them on 17.0.1esr, and I dout there have been changes to code on the esr17 branch that would justify this. Which bears the question: what changed on the esr17 build environment?

Comment 7

[Mike Hommey [:glandium]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #5)
> The point is is not added backward compatibility. It is just that this bugs
> gives us the worse of two options: we call dlclose, and it does nothing. It
> would be very surprising for someone changing a use of string or hash to
> suddenly see libxul.so being unloaded.

dlclose is not expected to unload a library and has never been to. The most stupid example of that is having two different things calling dlopen() and only one calling dlclose(). Another is dlopen()ing a library that was already pulled as a dependency of another library.

Comment 8

[Benjamin Smedberg]

Bug 761135. There's no point in supporting dlclose on libxul. We don't actually ever call it currently anyway.

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> dlclose is not expected to unload a library and has never been to. The most
> stupid example of that is having two different things calling dlopen() and
> only one calling dlclose(). Another is dlopen()ing a library that was
> already pulled as a dependency of another library.

Sure, the problem is the brittleness. Unrelated changes like the one you noticed on esr can cause it to unload it or not.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Bug 761135. There's no point in supporting dlclose on libxul. We don't
> actually ever call it currently anyway.

We do:
        dlclose(sXULLibHandle);

in xpcom/glue/standalone/nsGlueLinkingDlopen.cpp.

Comment 11

[Benjamin Smedberg]

We don't actually *call* that function from anywhere; it's just dead code.

Comment 12

[Mike Hommey [:glandium]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #10)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> > Bug 761135. There's no point in supporting dlclose on libxul. We don't
> > actually ever call it currently anyway.
> 
> We do:
>         dlclose(sXULLibHandle);
> 
> in xpcom/glue/standalone/nsGlueLinkingDlopen.cpp.

That's called from XPCOMGlueShutdown.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> We don't actually *call* that function from anywhere; it's just dead code.

We still do, until bug 761135.

Comment 13

[Benjamin Smedberg]

Holy hell. I thought XPCOMGlueShutdown was dead code. Please remove it ASAP.