[cfe-dev] [libc++] A proposal for getting rid of __always_inline__ in _LIBCPP_INLINE_VISIBILITY

[Eric Fiselier via cfe-dev]

On Tue, Jul 10, 2018 at 1:13 PM, Louis Dionne <ldionne at apple.com> wrote:

>
> On Jul 10, 2018, at 10:07, Eric Fiselier <eric at efcs.ca> wrote:
>
> I like the idea of `_LIBCPP_HIDE_FROM_ABI`, and I think the first step you
> proposed sounds reasonable.
>
> On Mon, Jul 9, 2018 at 10:33 AM, Louis Dionne via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
>
>> 1. Everything that is ABI stable is marked with a visibility macro like
>> today (no change here).
>> 2. Everything that is marked with `_LIBCPP_INLINE_VISIBILITY` today is
>> marked with a new `_LIBCPP_HIDE_FROM_ABI` macro instead. This macro can be
>> defined to `__attribute__((__visibility__("hidden")))` or to
>> `__attribute__((__visibility__("hidden"),internal_linkage))` depending
>> on whether we want to support only use case (1), or both use cases (1) and
>> (2), respectively.
>
>
> By ABI stable do you mean exported from the libc++ dylib, or all
> non-reserved names provided by the library? (2) suggests the former, since
> entities like std::sort are marked with _LIBCPP_INLINE_VISIBILITY.
> Any entity which a user can reasonably expect to externally instantiate
> cannot be part of the “hidden ABI”, since marking it with internal linkage
> will cause linkage failures.
>
>
> I think we'll be limited to applying `_LIBCPP_HIDE_FROM_ABI` to names
> which are (A) reserved and (B) cannot be externally instantiated as a
> member of a class.
>
>
> I mean exported from the libc++ dylib.
>
> For other readers:
> Regarding linkage failures, Eric and I talked privately and he explained
> that what he was worried about was a situation such as this one, where a
> symbol with internal linkage is included in an extern template declaration:
>
> ```
> // RUN: %cxx -c %s -o %t.first.o %flags %compile_flags
> // RUN: %cxx -c %s -o %t.second.o -DWITH_MAIN %flags %compile_flags
> // RUN: %cxx -o %t.exe %t.first.o %t.second.o %flags %link_flags
> // RUN: %run
>
> template <class T>
> class Foo {
> public:
>    __attribute__((internal_linkage)) void bar();
> };
>
> template <class T>
> void Foo<T>::bar() {}
>
> extern template class Foo<int>;
>
> #ifdef WITH_MAIN
> int main() {
>   Foo<int> f;
>   f.bar();
> }
> #else
> template class Foo<int>;
> #endif
> ```
>
> This indeed fails to link because the extern template declaration promises
> that an instantiation will be available in `t.first.o`, but that
> instantiation ends up having internal linkage and hence not being visible
> to `t.second.o`. However, if `bar` is marked inline instead, the example
> compiles because the C++ Standard requires the instantiation to still
> happen in `t.second.o` even though an extern instantiation is promised, per
> http://eel.is/c++draft/temp.explicit#12. I don’t think it is ever the
> case that we use `_LIBCPP_INLINE_VISIBILITY` on a function that is not
> marked inline in libc++, which is why my proposed solution works in
> practice. Indeed, if I remove the inline from a function marked as
> `_LIBCPP_INLINE_VISIBILITY` in a class that is externally instantiated, I
> do get a linker error when running libc++’s tests.
>
> So the gist of it is that Eric and I think Eric’s concern is not an issue
> for libc++, but by not much at all.
>
>
>
>> This would look something like:
>>
>>     #ifdef _LIBCPP_NO_INTERNAL_LINKAGE_FOR_ABI_UNSTABLE_SYMBOLS
>>     #   define _LIBCPP_HIDE_FROM_ABI __attribute__((__visibility__(
>> "hidden")))
>>     #else
>>     #   define _LIBCPP_HIDE_FROM_ABI __attribute__((__visibility__(
>> "hidden"),internal_linkage))
>>     #endif
>>
>> In the future, we can decide which default behavior we want, but for now,
>> I suggest we stick to what we have right now, which is support for both (1)
>> and (2). It would be fine to change this in the future if we make that
>> decision.
>>
>>
> When we don’t have internal linkage, I suspect we'll want to keep
> `__always_inline__` as to prevent static libraries from providing each
> other with incompatible function definitions (I think this could occur?).
>
>
> The idea here is that some people don’t care about interoperability of
> static libraries (or, equivalently, object files) built with different
> libc++ headers. And since they don’t care about this, they’d rather not pay
> the cost of inlining or internal linkage, which is code bloat.
>

The `__always_inline__` is always needed, at least when mixing ABI stable
and ABI hidden symbols in a explicitly instantiated template. Here's a
reproducer that should fail to link on GCC and Clang:
https://godbolt.org/g/LLZGjG


>
>
> Louis
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20180710/6466c567/attachment.html>