Open Bug 826567 Opened 11 years ago Updated 2 years ago

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

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
x86_64
Linux
Type:
defect

Tracking

(Not tracked)

People

(Reporter: espindola, Unassigned)

Details

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/.
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.
(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?
> > 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
(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.
> > 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.
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?
(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.
Bug 761135. There's no point in supporting dlclose on libxul. We don't actually ever call it currently anyway.
> 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.
(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.
We don't actually *call* that function from anywhere; it's just dead code.
(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.
Holy hell. I thought XPCOMGlueShutdown was dead code. Please remove it ASAP.
Product: Core → Firefox Build System
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.