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

[Louis Dionne via cfe-dev]

> On Jul 10, 2018, at 16:46, Eric Fiselier <eric at efcs.ca> wrote:
> 
> On Tue, Jul 10, 2018 at 1:13 PM, Louis Dionne <ldionne at apple.com <mailto:ldionne at apple.com>> wrote:
> 
>> On Jul 10, 2018, at 10:07, Eric Fiselier <eric at efcs.ca <mailto: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 <mailto: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 <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 <https://godbolt.org/g/LLZGjG>
I don’t believe this example is conforming. Per http://eel.is/c++draft/temp.explicit#12 <http://eel.is/c++draft/temp.explicit#12>, `__init` should still be instantiated in `main.o`, because `__init` is marked inline. I must be wrong because otherwise both GCC and Clang are compiling this wrong.

Louis

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