Bug 22592 - -fvisibility-inlines-hidden broken differently
-fvisibility-inlines-hidden broken differently
Status: RESOLVED INVALID
Product: gcc
Classification: Unclassified
Component: c++
4.0.2
: P2 normal
: ---
Assigned To: Not yet assigned to anyone
: visibility
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-07-21 16:18 UTC by Michael Matz
Modified: 2006-06-20 18:21 UTC (History)
5 users (show)

See Also:
Host: x86_64-linux
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Matz 2005-07-21 16:18:36 UTC
This is another instance of the visibility stuff being broken.  This time 
because a function isn't emitted, but called in a way as if it was. 
This code: 
--------------------------------- 
struct A { 
  virtual bool operator== (const A &a) const; 
  virtual bool operator!= (const A &a) const; 
}; 
inline bool A::operator!= ( const A &a) const 
{ return !(*this == a); } 
bool breakme(const A& newPath, A &b) 
{ return A(newPath) != b; } 
--------------------------------- 
 
Compile with (4.0.2 pre): 
% g++ -fPIC -DPIC -I/usr/lib/qt3/include -fvisibility=hidden \ 
-fvisibility-inlines-hidden -Wall -O2 -o libbla.so -shared testcase.ii 
ld: /tmp/ccWAn1go.o: relocation R_X86_64_PC32 against `A::operator!=(A const&) 
const' can not be used when making a shared object; recompile with -fPIC 
 
The chain of events goes like so: 
1) the operator is virtual, hence initially the call is through the vtable 
2) inlining happens before that is optimized, hence the operator call 
   is not inlined 
3) the virtual call is optimized to a direct call of the operator!=, because 
   of the explicit copy operation GCC knows the final type of the object 
   on which it is called (namely "A"), and hence can transform the indirect 
   to a direct call 
4) the call is emitted such that it expects a local (hidden) definition 
   of the function in the DSO (i.e. not over PLT but direct call) 
5) But no copy of that function is emitted in this DSO, because the 
   out-of-line copies of such inline function are generated only in the .o 
   file which contains the key method of the class (the first virtual func 
   in this case).  So nothing can be made hidden, and it anyway wouldn't 
   work even if, as there simply is nothing to call in this DSO. 
 
I believe this is a different error from all the other visibility problems 
like bug 19664 or bug 20218.  I don't know what GCC should do here. 
 
It either needs to emit an out-of-line copy of this operator, or generate the 
call over a PLT.  The first solution would be better, but could mean 
that we need to emit such out-of-line copies in every .o file where they 
are referenced.  The second solution might even not work at all, as probably 
the main library (containing the key method and hence the out-of-line 
versions) is also compiled with -fvisibility-inlines-hidden, and hence 
wouldn't even export those function which a call over PLT could resolve to. 
 
So, it seems that cgraph needs to be changed somehow to emit this function 
if referenced.  Hence CCing Honza.
Comment 1 Andrew Pinski 2005-07-21 16:39:16 UTC
No I think this code is in fact invalid and should error out like this.  Note you also declared operator== 
as being hidden too.  So if you call that, it would break too.
Comment 2 Michael Matz 2005-07-22 12:46:04 UTC
I don't understand.  The code itself is perfectly valid C++, I don't think  
you mean that it's invalid, right?  Yes, operator== is also hidden, but 
there is no definition for it in this unit, hence GCC generates the correct 
call type (over PLT).  (It should also be noted that because of the other bugs 
GCC can't emit the .hidden directives for undefined symbols, except when 
using HJs patches, but that's tangential and wouldn't make a difference, 
the crucial point is, that the correct call is emitted). 
 
And irrespective of that the error also happens without -fvisibility=hidden, 
i.e. when _only_ the inlines are hidden.  I still think this is a bug, 
which should be corrected by making GCC just emit an out-of-line copy of 
the inline function (in a linkonce section). 
Comment 3 Andrew Pinski 2005-07-26 20:51:18 UTC
I still don't think this is a bug as if I compile the library on ppc-darwin, we get the following link error 
even without -fvisibility=hidden/-fvisibility-inlines-hidden:
ld: Undefined symbols:
__ZNK1AneERKS_
__ZTV1A

this is at -O2.
Comment 4 Michael Matz 2005-07-27 13:46:19 UTC
Because these symbols indeed are not defined anywhere.  On linux this happens   
to work, but on darwin you need to link against something which provides them.  
  
So you would need to create a library which implements both operators  
out-of-line (and hence also the vtable), and us that to link _this_ library  
against.  
 
But that's not the issue at hand. 
  
This is the bigger picture, how this error can be seen in the real world: 
First, there is a base library (let's call it libbase) implementing the  
whole class, all methods, the vtable, everything.  
  
Then there's another library (libtwo), using that class in implementing some  
of it's functionality (breakme in our case).  It does so by including the  
header for that class (defining the inline operator !=, besides declaring  
the class), and linking against libbase.  Hence no unresolved symbols will  
occur.  
  
The libtwo exports only those symbols and class it wants exported, hence  
it switches the default visibility to hidden (including inlines), because all  
these are already defined in libbase, no need to export them too.  
  
This is all perfectly valid usage of shared libs.  But it doesn't work because  
libtwo can't be created due to the invalid call emitted to a method not  
defined in the same DSO.  
  
Perhaps I should have made more clear the bigger picture to not sidetrack  
others by the undefinedness of operator==.  In the real world it _will_ be  
defined, in a different shared lib.  So just for reference a little bit 
reformatted: 
 
--------------- libbase.ii -------------------- 
struct A { 
  virtual bool operator== (const A &a) const; 
  virtual bool operator!= (const A &a) const; 
}; 
inline bool A::operator!= ( const A &a) const 
{ return !(*this == a); } 
bool A::operator== (const A&a) const 
{ return true; } 
----------------------------------------------- 
 
Compile this with just "g++ -fPIC -shared -o libbase.so libbase.ii", and 
you have a shared lib you can use to link against when creating the 
second shared lib, from the source of the initial report here.  Note that 
the first few lines (including definition of operator !=) reflect a 
header file which declares class A, which is included in libbase.ii and 
testcase.ii. 
 
Comment 5 Mark Mitchell 2005-09-03 01:03:02 UTC
Frankly, I think -fvisibility-inlines-hidden is a bad idea.  I don't feel that
way about -fvisibility, but giving inline functions special linkage in this way
is a very fragile concept, and awards something with minimal C++ semantics
("inline") substantial observable semantics.

The documentation for -fv-i-h is not clear about this case.  All it really says
is that any inlines emitted will be hidden.  The compiler's meeting that
requirement, trivially, because it's not emitting any inlines.  

I don't think that we should do anything different in *this* source file because
-fv-i-h is present.  The thing we actually want to know is how the file
containing the vtable is going to be compiled, and we can't know that here. 
Therefore, it seems to me that the mistake in Michael's chain of events is:

4) the call is emitted such that it expects a local (hidden) definition 
   of the function in the DSO (i.e. not over PLT but direct call) 

Nothing in the semantics of -fv-i-h says that all inline functions in the
program will be in this DSO.  If we were to try to believe that, other things
(like "extern template") would probably break too.  Instead, the linker should
have the responsibility of binding the relocation within the DSO at DSO
link-time, if there happens to be a satisfying symbol at link time.

Jan, I don't think your patch is correct, as I don't think anything about the
optimization of this program should change based on -fv-i-h.
Comment 6 Jan Hubicka 2005-09-03 10:51:46 UTC
Subject: Re:  -fvisibility-inlines-hidden broken differently

> 
> ------- Additional Comments From mmitchel at gcc dot gnu dot org  2005-09-03 01:03 -------
> Frankly, I think -fvisibility-inlines-hidden is a bad idea.  I don't feel that
> way about -fvisibility, but giving inline functions special linkage in this way
> is a very fragile concept, and awards something with minimal C++ semantics
> ("inline") substantial observable semantics.
> 
> The documentation for -fv-i-h is not clear about this case.  All it really says
> is that any inlines emitted will be hidden.  The compiler's meeting that
> requirement, trivially, because it's not emitting any inlines.  

Is that even correct that we don't emit any inline?  (ie see bellow)
> 
> I don't think that we should do anything different in *this* source file because
> -fv-i-h is present.  The thing we actually want to know is how the file
> containing the vtable is going to be compiled, and we can't know that here. 
> Therefore, it seems to me that the mistake in Michael's chain of events is:
> 
> 4) the call is emitted such that it expects a local (hidden) definition 
>    of the function in the DSO (i.e. not over PLT but direct call) 
> 
> Nothing in the semantics of -fv-i-h says that all inline functions in the
> program will be in this DSO.  If we were to try to believe that, other things
> (like "extern template") would probably break too.  Instead, the linker should
> have the responsibility of binding the relocation within the DSO at DSO
> link-time, if there happens to be a satisfying symbol at link time.
> 
> Jan, I don't think your patch is correct, as I don't think anything about the
> optimization of this program should change based on -fv-i-h.

Hmm, OK, there are two problems I see.
One problem is that this testcase breaks. We can declare it invalid or
one other possibility I run across is to teach locally_bound predicate
in varasm to not believe that this hidden inline function is going to be
linked locally.  This is very similar to way -fvisibility works - ie
function is hidden only if defined in current unit and would result in
DSO to build and even link if the other DSO will export the inline
function (that won't happen with -fv-i-h on the other DSO, right?)

This is not quite trivial, becuase -fv-i-h is hanled in the frontend and
backend already don't see whether the particular inline was actually
declared hidden (where user should provide it in within same DSO) or
whether the hidden implied this way, but I guess we can simply be
conservative here and when flag is active, thread this way all comdats
with inline flag and hidden visibility.


But the more general problem however is that I think it is quite
incorrect to call comdat linkonce function directly without actually
outputting it into current compilation unit, at least from the
definition of comdat flag:

/* Used in a DECL to indicate that, even if it TREE_PUBLIC, it need
   not be put out unless it is needed in this translation unit.
   Entities like this are shared across translation units (like weak
   entities), but are guaranteed to be generated by any translation
   unit that needs them, and therefore need not be put out anywhere
   where they are not needed.  DECL_COMDAT is just a hint to the
   back-end; it is up to front-ends which set this flag to ensure
   that there will never be any harm, other than bloat, in putting out
   something which is DECL_COMDAT.  */

Making direct call the function probably makes function "needed in this
translation unit" even tought it wasn't originally.  This can happen
with and without the patch in this special case (COMDAT functio being
originally referenced by vtable not going to be output here).  Is there
something behind scenes making this safe?  (like is the argument that
the function will be provided by unit defining vtable anyway safe and if
so, why is not frontend emitting it as extern inline in all other units
saving object file sizes?)

If not, we probably should prevent the folding...

Honza
> 
> -- 
> 
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=22592
> 
> ------- You are receiving this mail because: -------
> You are on the CC list for the bug, or are watching someone who is.
Comment 7 Michael Matz 2005-09-05 07:35:32 UTC
I want to add a comment to Mark's comment #5: I also thought about multiple 
solutions to this problem, and in the end came to the conclusion that 
disabling devirtualization in the current state of the compiler is the best. 
My reasons for different options: 
  * don't support -fv-i-h anymore: that would trivially solve the problem, 
    but at the expense of deactivating a sometimes quite effective 
    optimization (especially with template heavy code, in the light of 
    non-class-member inlines), so I would like to retain the option itself 
    and support it as best as possible, even when this means to stretch 
    the C++ standard a bit in some corner, as this is just an option for 
    those who know what they are doing 
 
  * emit all referenced inlines in the compilation unit of reference, 
    as linkonce.  This also would solve the problem of the direct call, 
    as now the implicit condition is met, that the function is indeed 
    forced local.  But with the current infrastructure in 4.0 (and even 4.1), 
    this means that we would have to emit all _potentially_ referenced 
    inlines, because devirtualization can make random functions called 
    directly which weren't before.  This would increase the memory 
    footprint of GCC again and as such is suboptimal. 
 
  * only localize (and hide) inline functions which are not class members. 
    This would solve this problem, because only class member inlines are 
    in danger of suddenly being called directly, when they formerly were 
    only called over the vtable.  I haven't thought much about this solution, 
    but I think it's very feasible also.  At least it should retain 
    most of the meaning of the -fv-i-h option, in that it reduces the 
    exporting of many out-of-class inline functions, particularly templates. 
 
  * disable devirtualization.  Solves the problem, and has the least 
    effects regarding code quality.  AFAIK currently devirtualization 
    is not used for very many interesting things.  Inlining happens 
    before, so we don't get more inlining opportunities.  Attributes 
    like constness are available before, so optimizations relying on them 
    are also possible on the indirect vtable-call.  And what finally 
    convinced me was the fact that calls over the vtable are actually 
    faster than calls over the PLT.  The call over PLT includes the call 
    to the PLT stub, the fetch of the address and the jump to the final 
    address (at first call also the symbol resolution).  The vtable call 
    includes the fetch of the address and the call to it. 
 
So all in all I think the best trade off is the last option.  At the expense 
of a missed optimization (which doesn't happen very often, as we can see 
how often this bug happens, and if it happens then it's IMHO not very 
effective currently) he get's a program working with this option, and with 
the effects this option should have (less exported "fake" symbols). 
Comment 8 Jason Merrill 2006-06-13 23:28:03 UTC
Either 20218 is a bug or this is.  It seems to me that 20218 is the bug.

If you declare a function to be hidden, you are asserting that it will be defined in the current DSO.  From the GCC documentation:

"Two declarations of an object with hidden linkage refer to the same object
if they are in the same shared object."

Calling this function directly is a correct optimization, the bug is that you fail to define it (by defining the key method) in the same DSO.

If this class is imported from a library, it shouldn't have hidden linkage; the library's namespace should have explicit default linkage.
Comment 9 Jason Merrill 2006-06-20 18:21:36 UTC
Not a bug; see my earlier comment.