[cfe-commits] [patch][pr13124] Use strong symbols for functions that go in a strong vtables

[John McCall]

On Jun 27, 2012, at 10:32 AM, John McCall wrote:
> On Jun 27, 2012, at 10:24 AM, Rafael Espíndola wrote:
>> The tests in pr13124 shows a case where both gcc 4.7 and clang produce
>> an undefined reference to a destructor that we currently output (in
>> another TU) as linkonce_odr.
>> 
>> This works 99.9% of the time because we output a strong vtable that
>> references that destructor, preventing any of the existing
>> optimizations from dropping it, but there is no guarantee that that
>> cannot happen. For example, function merging could merge that
>> destructor with another one since they are unnamed_addr.
>> 
>> This also causes problems when globalopt marks linkonce_odr
>> unnamed_addr functions hidden and we are using the symbol across
>> shared libraries.
>> 
>> The attached patch ensures that we produce a strong symbol for
>> functions that go in an strong vtable. This in turn ensure that it is
>> safe to output those vtables as available_externally in other TUs (and
>> use clang compiled libraries with gcc 4.7 compiled programs).
> 
> You did not attach a patch, but that's okay, because the behavior it
> implements is illegal as described.  We cannot emit a strong symbol
> for something that can emitted as a weak symbol in another translation
> unit;  I know that the linux linker is quite permissive about this, but (e.g.)
> the Darwin linker is not, and I would expect that even the Linux linker is
> generally not going to do The Right Thing with COMDAT.

In addition, as I read the Itanium ABI, we have no guarantee that the
translation unit exporting the key function must also export all the inline
virtual functions.  That is, while that translation unit clearly has to emit
all the inline virtual functions, it could freely do so with hidden visibility,
or even as internal symbols (although that would likely be a
pessimization).  That's actually a useful optimization.

The bug here is the translation unit which emits an external reference
to the inline method.  That is never allowed under the Itanium ABI;
every translation unit which uses the method must define it.  If we're
going to emit available_externally vtables, we need to emit all the
inline methods they reference.

Obviously, we'd really want a way to do so "on demand", instead of
aggressively emitting all the declarations just in case.

John.