Add 'inline' but not _LIBCPP_INLINE_VISIBILITY to basic_string's destructor

[EricWF]

Summary

r280944 attempted to add both inline and _LIBCPP_INLINE_VISIBILITY to basic_string's destructor in order to achieve better inlining behavior. Unfortunately applying _LIBCPP_INLINE_VISIBILITY to the destructor triggers a Clang bug. This patch attempts to still get better inlining behavior while working around the Clang bug by simply applying inline without the attribute.

Unfortunately applying only inline causes the destructor to be inlined at -O2 or greater whereas the attribute results in inlining at all optimization levels. However this is still better than no inlining at all. See https://godbolt.org/g/8UbKwb for example assembly.

My only concern with this patch is marking the function as inline without making the visibility hidden. I don't think that should have any weird consequences but I'm not 100%.

[mclow.lists]

Any reason we shouldn't just revert r280944, wait for the LLVM bug to be fixed, and then re-apply it?

[hiraditya]

@EricWF, since inline is only a hint, the compiler would not inline in many cases, it might give the inliner a little bit of push to inline. When we were working on this patch, adding inline wasn't enough and hence we added the _LIBCPP_INLINE_VISIBILITY flag. The compiler crash seems to be in the Verifier which does not allow aliases to available_externally functions.

[EricWF]

In D24599#543849, @mclow.lists wrote:

Any reason we shouldn't just revert r280944, wait for the LLVM bug to be fixed, and then re-apply it?

I would like to put some time between fixing the Clang bug and re-introducing the reproducer into libc++.
Like it would be nice if 3.9 + libc++ still self hosted. I already reverted r280944 and I think we should put it
back eventually, but maybe not right after it's fixed.

In D24599#543917, @hiraditya wrote:

@EricWF, since inline is only a hint, the compiler would not inline in many cases, it might give the inliner a little bit of push to inline. When we were working on this patch, adding inline wasn't enough and hence we added the _LIBCPP_INLINE_VISIBILITY flag. The compiler crash seems to be in the Verifier which does not allow aliases to available_externally functions.

I'm aware. As I mentioned in the summary Clang only listens to inline at -O2 or greater. However without inline it won't even get inlined then. This is more of a bandage than your complete solution.

[hiraditya]

Please also see: D24682

[hiraditya]

I think now the original patch can be re-applied now that the bug: https://llvm.org/bugs/show_bug.cgi?id=30341
has been fixed in D24682

[EricWF]

Another version of this has already been committed.

include/string

 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__insert_c(this);
 #endif
 }
 
 #endif  // _LIBCPP_HAS_NO_GENERALIZED_INITIALIZERS
 
 template <class _CharT, class _Traits, class _Allocator>
+inline // _LIBCPP_INLINE_VISIBILITY FIXME: http://llvm.org/PR30341
 basic_string<_CharT, _Traits, _Allocator>::~basic_string()
 {
 #if _LIBCPP_DEBUG_LEVEL >= 2
     __get_db()->__erase_c(this);
 #endif
     if (__is_long())
         __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
 }