| Message ID | 20130828172318.GB3513@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
Looks good. Jason
Paolo, there seems to be one extra issue about this patch. It causes quite a twist in libstdc++ exported symbols. It is purpose of the patch to remove those that are going to be generated in user programs, too. I am however bit confused about bad array. Perhaps it is an optimization difference dragging it in? Does the changes look sensible? Honza 21 added symbols 0 _ZNSt20bad_array_new_lengthD2Ev std::bad_array_new_length::~bad_array_new_length() version status: compatible CXXABI_1.3.8 type: function status: added 1 __cxa_throw_bad_array_length version status: compatible CXXABI_1.3.8 type: function status: added 2 _ZNKSt20bad_array_new_length4whatEv std::bad_array_new_length::what() const version status: compatible CXXABI_1.3.8 type: function status: added 3 _ZNSt20bad_array_new_lengthD0Ev std::bad_array_new_length::~bad_array_new_length() version status: compatible CXXABI_1.3.8 type: function status: added 4 _ZSt14get_unexpectedv std::get_unexpected() version status: compatible GLIBCXX_3.4.20 type: function status: added 5 _ZTVSt16bad_array_length vtable for std::bad_array_length version status: compatible CXXABI_1.3.8 type: object type size: 40 status: added 6 _ZTSSt16bad_array_length typeinfo name for std::bad_array_length version status: compatible CXXABI_1.3.8 type: object type size: 21 status: added 7 _ZNSt16bad_array_lengthD2Ev std::bad_array_length::~bad_array_length() version status: compatible CXXABI_1.3.8 type: function status: added 8 _ZSt15get_new_handlerv std::get_new_handler() version status: compatible GLIBCXX_3.4.20 type: function status: added 9 CXXABI_1.3.8 version status: compatible type: object type size: 0 status: added 10 _ZTVSt20bad_array_new_length vtable for std::bad_array_new_length version status: compatible CXXABI_1.3.8 type: object type size: 40 status: added 11 _ZTSSt20bad_array_new_length typeinfo name for std::bad_array_new_length version status: compatible CXXABI_1.3.8 type: object type size: 25 status: added 12 GLIBCXX_3.4.20 version status: compatible type: object type size: 0 status: added 13 _ZTISt20bad_array_new_length typeinfo for std::bad_array_new_length version status: compatible CXXABI_1.3.8 type: object type size: 24 status: added 14 _ZSt13get_terminatev std::get_terminate() version status: compatible GLIBCXX_3.4.20 type: function status: added 15 _ZNSt16bad_array_lengthD0Ev std::bad_array_length::~bad_array_length() version status: compatible CXXABI_1.3.8 type: function status: added 16 __cxa_throw_bad_array_new_length version status: compatible CXXABI_1.3.8 type: function status: added 17 _ZNSt16bad_array_lengthD1Ev std::bad_array_length::~bad_array_length() version status: compatible CXXABI_1.3.8 type: function status: added 18 _ZTISt16bad_array_length typeinfo for std::bad_array_length version status: compatible CXXABI_1.3.8 type: object type size: 24 status: added 19 _ZNSt20bad_array_new_lengthD1Ev std::bad_array_new_length::~bad_array_new_length() version status: compatible CXXABI_1.3.8 type: function status: added 20 _ZNKSt16bad_array_length4whatEv std::bad_array_length::what() const version status: compatible CXXABI_1.3.8 type: function status: added 17 missing symbols 0 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEE6xsputnEPKwl __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::xsputn(wchar_t const*, long) version status: unversioned type: function status: subtracted 1 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEED0Ev __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 2 _ZNKSt5ctypeIcE8do_widenEc std::ctype<char>::do_widen(char) const version status: unversioned type: function status: subtracted 3 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEE6xsgetnEPcl __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::xsgetn(char*, long) version status: unversioned type: function status: subtracted 4 _ZNSt15basic_stringbufIcSt11char_traitsIcESaIcEED0Ev std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 5 _ZNSt15basic_stringbufIwSt11char_traitsIwESaIwEED0Ev std::basic_stringbuf<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 6 _ZNK10__cxxabiv117__pbase_type_info15__pointer_catchEPKS0_PPvj __cxxabiv1::__pbase_type_info::__pointer_catch(__cxxabiv1::__pbase_type_info const*, void**, unsigned int) const version status: unversioned type: function status: subtracted 7 _ZNKSt5ctypeIcE8do_widenEPKcS2_Pc std::ctype<char>::do_widen(char const*, char const*, char*) const version status: unversioned type: function status: subtracted 8 _ZNSt15basic_stringbufIwSt11char_traitsIwESaIwEED1Ev std::basic_stringbuf<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 9 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEED1Ev __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 10 _ZNKSt5ctypeIcE9do_narrowEcc std::ctype<char>::do_narrow(char, char) const version status: unversioned type: function status: subtracted 11 _ZNSt15basic_stringbufIcSt11char_traitsIcESaIcEED1Ev std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 12 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEED1Ev __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 13 _ZNKSt5ctypeIcE9do_narrowEPKcS2_cPc std::ctype<char>::do_narrow(char const*, char const*, char, char*) const version status: unversioned type: function status: subtracted 14 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEE6xsgetnEPwl __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::xsgetn(wchar_t*, long) version status: unversioned type: function status: subtracted 15 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEED0Ev __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 16 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEE6xsputnEPKcl __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::xsputn(char const*, long) version status: unversioned type: function status: subtracted 2 undesignated symbols 0 _ZSt11__once_call std::__once_call version status: compatible GLIBCXX_3.4.11 type: tls type size: 8 status: undesignated 1 _ZSt15__once_callable std::__once_callable version status: compatible GLIBCXX_3.4.11 type: tls type size: 8 status: undesignated 17 incompatible symbols 0 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEE6xsputnEPKwl __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::xsputn(wchar_t const*, long) version status: unversioned type: function status: subtracted 1 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEED0Ev __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 2 _ZNKSt5ctypeIcE8do_widenEc std::ctype<char>::do_widen(char) const version status: unversioned type: function status: subtracted 3 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEE6xsgetnEPcl __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::xsgetn(char*, long) version status: unversioned type: function status: subtracted 4 _ZNSt15basic_stringbufIcSt11char_traitsIcESaIcEED0Ev std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 5 _ZNSt15basic_stringbufIwSt11char_traitsIwESaIwEED0Ev std::basic_stringbuf<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 6 _ZNK10__cxxabiv117__pbase_type_info15__pointer_catchEPKS0_PPvj __cxxabiv1::__pbase_type_info::__pointer_catch(__cxxabiv1::__pbase_type_info const*, void**, unsigned int) const version status: unversioned type: function status: subtracted 7 _ZNKSt5ctypeIcE8do_widenEPKcS2_Pc std::ctype<char>::do_widen(char const*, char const*, char*) const version status: unversioned type: function status: subtracted 8 _ZNSt15basic_stringbufIwSt11char_traitsIwESaIwEED1Ev std::basic_stringbuf<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 9 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEED1Ev __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 10 _ZNKSt5ctypeIcE9do_narrowEcc std::ctype<char>::do_narrow(char, char) const version status: unversioned type: function status: subtracted 11 _ZNSt15basic_stringbufIcSt11char_traitsIcESaIcEED1Ev std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::~basic_stringbuf() version status: unversioned type: function status: subtracted 12 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEED1Ev __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 13 _ZNKSt5ctypeIcE9do_narrowEPKcS2_cPc std::ctype<char>::do_narrow(char const*, char const*, char, char*) const version status: unversioned type: function status: subtracted 14 _ZN9__gnu_cxx18stdio_sync_filebufIwSt11char_traitsIwEE6xsgetnEPwl __gnu_cxx::stdio_sync_filebuf<wchar_t, std::char_traits<wchar_t> >::xsgetn(wchar_t*, long) version status: unversioned type: function status: subtracted 15 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEED0Ev __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::~stdio_sync_filebuf() version status: unversioned type: function status: subtracted 16 _ZN9__gnu_cxx18stdio_sync_filebufIcSt11char_traitsIcEE6xsputnEPKcl __gnu_cxx::stdio_sync_filebuf<char, std::char_traits<char> >::xsputn(char const*, long) version status: unversioned type: function status: subtracted
Hi, On 08/29/2013 10:11 AM, Jan Hubicka wrote: > Paolo, > there seems to be one extra issue about this patch. It causes quite a twist in libstdc++ exported symbols. > It is purpose of the patch to remove those that are going to be generated in user programs, too. > I am however bit confused about bad array. Perhaps it is an optimization difference dragging it in? I'm also confused because your patch isn't already in (at least I can't find such a ChangeLog in svn) and what you attached, as long as bad_array_* is concerned is just what we currently have, before the patch. Check point: 21 added symbols. In general, of course, inlining alone can make a difference, if for example you inline less and a pattern in the linker map isn't tight enough some symbols can become inadvertently exported. *But*, outside bad_array* I can see a lot of SUBTRACTED, those would be very bad regressions of course. check_abi cannot pass. In the past, when we (library people) had special issues with versioning, like we had to patch a mistake we made, Jakub promptly helped, I would recommend referring to him too, if you are seeing something weird, but the general rule as far as testing is concerned is very simple: *never* ignore check_abi failures, make sure additional symbols are exported only at the current ABI version, never add symbols at older versions, never remove symbols. Paolo.
M > Hi, > > On 08/29/2013 10:11 AM, Jan Hubicka wrote: > >Paolo, > >there seems to be one extra issue about this patch. It causes quite a twist in libstdc++ exported symbols. > >It is purpose of the patch to remove those that are going to be generated in user programs, too. > >I am however bit confused about bad array. Perhaps it is an optimization difference dragging it in? > I'm also confused because your patch isn't already in (at least I > can't find such a ChangeLog in svn) and what you attached, as long > as bad_array_* is concerned is just what we currently have, before > the patch. Check point: 21 added symbols. I see, I tought that adding a symbol will make ABI check to fail, so I got sidetracked by trying to understand why we need the bad array. > > In general, of course, inlining alone can make a difference, if for > example you inline less and a pattern in the linker map isn't tight > enough some symbols can become inadvertently exported. > > *But*, outside bad_array* I can see a lot of SUBTRACTED, those would > be very bad regressions of course. check_abi cannot pass. I see, it is purpose of the patch to cause regressions like this :) What I am tracking is the following testcase: jan@linux-9ure:~> cat t.C __attribute__ ((noinline)) inline void t(void) { asm("fsin"); } void q(void); main() { q(); t(); } jan@linux-9ure:~> cat t2.C __attribute__ ((noinline)) inline void t(void) { asm("fsin"); } void q(void) { t(); } jan@linux-9ure:~> gcc -O2 t2.C -fPIC --shared -o libt.so jan@linux-9ure:~> gcc -O2 t.C -lt -L. Now I have a shared library and main binary but defining function t(): jan@linux-9ure:~> objdump -d a.out | grep fsin 400720: d9 fe fsin jan@linux-9ure:~> objdump -d libt.so | grep fsin 720: d9 fe fsin Both define it in dynamic symbol table: jan@linux-9ure:~> objdump -t a.out | grep _Z1tv 0000000000400720 w F .text 0000000000000003 _Z1tv jan@linux-9ure:~> objdump -t libt.so | grep _Z1tv 0000000000000720 w F .text 0000000000000003 _Z1tv and libt has dynamic relocation on call to the function: jan@linux-9ure:~> objdump -R libt.so | grep _Z1tv 0000000000201020 R_X86_64_JUMP_SLOT _Z1tv jan@linux-9ure:~> objdump -R a.out | grep _Z1tv jan@linux-9ure:~> Both versions of t() (in libt and a.out) are known to be semantically equivalent. My patch makes an observation that is does not matter to what copy of t() we call. So it makes both symbols hidden. This makes them resovled at link-time rather than at dynamic link-time. Thinks should still work as previously, just we will dispatch at runtime into both copies. I believe it is little cost to pay for getting cheaper dynamic linking and better codegen (for locally bound calls). The symbol disappear from the external symbol table, but this transformation ought to be valid since the same would happen if the symbol was fully inlined or its uses optimized out. This optimization will disable itself when you take address of t() so comparing addresses works as expected. Also the optimization won't take place when you key a template since then the users of the library do not necesarily need to emit their own copy of the code. My longer term vision is to turn as many relocations as possible from external to IP relative. This by itself will help (i.e. from libreoffice, this patch cuts down important percentage of symbols). If something like Mike Homey's elfhack http://glandium.org/blog/?p=1177 ever land into official dynamic linker (I think it should), we will see a lot smaller binaries. > > In the past, when we (library people) had special issues with > versioning, like we had to patch a mistake we made, Jakub promptly > helped, I would recommend referring to him too, if you are seeing > something weird, but the general rule as far as testing is concerned > is very simple: *never* ignore check_abi failures, make sure > additional symbols are exported only at the current ABI version, > never add symbols at older versions, never remove symbols. So my belief is that it is safe to drop those symbols from libstdc++. Every program/DSO using them have to define its own copy of those symbols, so I believe removing them from libstdc++ won't cause issues. Honza > > Paolo. >
Hi, On 08/29/2013 02:19 PM, Jan Hubicka wrote: > So my belief is that it is safe to drop those symbols from libstdc++. > Every program/DSO using them have to define its own copy of those > symbols, so I believe removing them from libstdc++ won't cause issues. Really, you should check with Jakub before proceeding. I the change it's Ok with him, it's Ok with me too (the other library maintainers should be in CC however). At minimum the baselines would need updating. If I can ask you a personal courtesy, please don't commit anything causing regressions, like check_abi failing, either a complete change, or nothing. Thanks! Paolo.
> Hi, > > On 08/29/2013 02:19 PM, Jan Hubicka wrote: > >So my belief is that it is safe to drop those symbols from > >libstdc++. Every program/DSO using them have to define its own > >copy of those symbols, so I believe removing them from libstdc++ > >won't cause issues. > Really, you should check with Jakub before proceeding. I the change > it's Ok with him, it's Ok with me too (the other library maintainers > should be in CC however). At minimum the baselines would need > updating. > > If I can ask you a personal courtesy, please don't commit anything > causing regressions, like check_abi failing, either a complete > change, or nothing. No worries. I do not intend to commit the patch until the check_abi issue is discussed and solved. (check_abi tends to be easy to miss when one checks test results by hand, but I am trying to force myself to always use compare_tests, so hopefully I won't do that in future) Note that currently you will get the same abi_check failure if you try to LTO libstdc++.so. It would be really nice if we started to do that eventually. Honza
On Thu, Aug 29, 2013 at 02:47:46PM +0200, Paolo Carlini wrote: > On 08/29/2013 02:19 PM, Jan Hubicka wrote: > >So my belief is that it is safe to drop those symbols from > >libstdc++. Every program/DSO using them have to define its own > >copy of those symbols, so I believe removing them from libstdc++ > >won't cause issues. > Really, you should check with Jakub before proceeding. I the change > it's Ok with him, it's Ok with me too (the other library maintainers > should be in CC however). At minimum the baselines would need > updating. I'm very nervous about removing any exported symbols, apps could dlsym them or whatever. So, if the compiler makes those hidden, either there should be an option to restore the old behavior and libstdc++ should use it, or we need some renaming/alias hacks to restore those. Jakub
> On Thu, Aug 29, 2013 at 02:47:46PM +0200, Paolo Carlini wrote: > > On 08/29/2013 02:19 PM, Jan Hubicka wrote: > > >So my belief is that it is safe to drop those symbols from > > >libstdc++. Every program/DSO using them have to define its own > > >copy of those symbols, so I believe removing them from libstdc++ > > >won't cause issues. > > Really, you should check with Jakub before proceeding. I the change > > it's Ok with him, it's Ok with me too (the other library maintainers > > should be in CC however). At minimum the baselines would need > > updating. > > I'm very nervous about removing any exported symbols, apps could dlsym them > or whatever. So, if the compiler makes those hidden, either there should be > an option to restore the old behavior and libstdc++ should use it, or we > need some renaming/alias hacks to restore those. I can add an option to disable it ( would -fno-private-inlines work?) that will disable this transformation to hidden as well as LTO comdat localization when symbol is PREVAIL_EXT? Of course if we are affraid to remove the symbols from libstdc++, I wonder how safe we are about APIs of other C++ libraries. Do you think people dlsym inline functions and comdat vtables from dlopened DSOs? Honza > > Jakub
Index: testsuite/g++.dg/ext/visibility/template2.C =================================================================== --- testsuite/g++.dg/ext/visibility/template2.C (revision 202047) +++ testsuite/g++.dg/ext/visibility/template2.C (working copy) @@ -4,15 +4,16 @@ /* { dg-do compile } */ /* { dg-require-visibility "" } */ -/* { dg-final { scan-not-hidden "_ZN1SIiED1Ev" } } */ -/* { dg-final { scan-not-hidden "_ZN1SIiEC1ERKi" } } */ +/* { dg-final { scan-not-hidden "_ZN1SIiE9nothiddenEv" } } */ template <class T> struct S { S (const T &); + void nothidden(void); ~S (); T t; + void (S::* ptr) (void); }; template <class T> @@ -20,6 +21,13 @@ S<T>::S (const T &x) { t = x; } +template <class T> +void +S<T>::nothidden(void) +{ + if (t) + ptr=&S::nothidden; +} template <class T> S<T>::~S () @@ -30,6 +38,6 @@ S<T>::~S () struct U { S<int> s; - U () : s (6) { } + U () : s (6) { s.nothidden();} } u; #pragma GCC visibility pop