Patchwork Make some comdats implicitly hidden

login
register
mail settings
Submitter Jan Hubicka
Date Aug. 28, 2013, 5:23 p.m.
Message ID <20130828172318.GB3513@kam.mff.cuni.cz>
Download mbox | patch
Permalink /patch/270609/
State New
Headers show

Comments

Jan Hubicka - Aug. 28, 2013, 5:23 p.m.
> On 08/26/2013 03:57 PM, Jan Hubicka wrote:
> >While analyzing the relocations of libreoffice I noticed that I can play
> >the same game w/o LTO at linker level.  Making those symbols hidden truns
> >external relocations to internal and should improve runtime, too: comdat
> >sharing by dynamic linker is expensive and won't save duplicated functions
> >from the binary.
> 
> Makes sense.
> 
> >There is ext/visibility/template2.C that fails with the patch. It tests that
> >visibility pragma does not bring the symbol local, but now we do so based
> >on logic above.
> >
> >Jason, do you have any idea how to fix the testcase? I tried to use different
> >visility but that doesn't work, since we do not have corresponding scan-not-*
> 
> Maybe change it to use a function that has its address taken, so
> this optimization doesn't apply.
OK, thanks.  This did not appear to me.  Does the following look resonable?
Jason Merrill - Aug. 28, 2013, 9:27 p.m.
Looks good.

Jason
Jan Hubicka - Aug. 29, 2013, 8:11 a.m.
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
Paolo Carlini - Aug. 29, 2013, 9:13 a.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.

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.
Jan Hubicka - Aug. 29, 2013, 12:19 p.m.
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.
>
Paolo Carlini - Aug. 29, 2013, 12:47 p.m.
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.
Jan Hubicka - Aug. 29, 2013, 12:54 p.m.
> 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
Jakub Jelinek - Aug. 29, 2013, 12:56 p.m.
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
Jan Hubicka - Aug. 29, 2013, 1:04 p.m.
> 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

Patch

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