[libcxx] r281681 - [libc++] Add _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY to support GCC ABI compatibility

[Mehdi AMINI via cfe-commits]

2017-03-29 20:30 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:

>
> On Mar 29, 2017, at 20:16, Eric Fiselier <eric at efcs.ca> wrote:
>
>
>
> On Wed, Mar 29, 2017 at 9:00 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
>
>> Why are we propagating the use of always_inline?
>>
>
> The intent of this change wasn't to propagate or remove always_inline from
> any functions. It was to fix incompatible dylibs built by GCC.
> The way it did that is by removing the __attribute__((visibility("hidden")))
> part while building the dylib, because GCC was ignoring the
> `visibility("default")` on the extern template declarations for basic
> string.
>
>
>> In other places, we use always_inline to avoid affecting ABI (a
>> visibility hack).  But you're not removing basic_string from the dylib here.
>>
>
> That's a very good point. It's likely we should convert many of the
> `_LIBCPP_ALWAYS_INLINE`s to inline instead and the compiler should
> do the right thing. However that change is orthogonal to this one, and
> hence it wasn't made here.
>
>
> Reading comprehension issue on my part.  I think probably replied to the
> wrong commit.  Bisection pointed at r278356, but I saw that that commit was
> reverted and I couldn't find where it finally landed.  So then I did a
> careless git-blame for what I thought was the relevant line and ended up
> here.
>
> Also I think the doc for _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY is
> misleading. I'll try to update it in the coming days.
>
>
>
>
>> It has major downsides:
>> - It causes inlining at -O0, which causes major regressions in debugging
>> experiences.
>> - It causes inlining at -O0, which, without other optimizations, can blow
>> up stack size of static_initializers.
>>
>
> For some extra context: we tracked down a stack overflow to the related
> changes to add always_inline to basic_string::__init.  Our user had some
> string => enum value map declared statically, like in the attached
> stackoverflow.cpp.  At -Os (and higher), there's no difference.  But with
> -O0 and -O1, they started getting a stack overflow at launch.
>

That's not a bug but a feature ;-)
It was a good way to show the user he's not doing the right thing by using
a costly initialization routine!

-- 
Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170329/34960704/attachment.html>