[libc++] Stop marking interface symbols always_inline + hidden when building for unstable ABI

[eugenis]

Summary

ABI stability is not an issue when building for unstable ABI.
Other than that, always_inline is mostly harmful:

• it does not completely solve the ABI stability problem, because there is no guarantee that an always_inline function will be inlined.
• it really harms -O0 where indiscriminate inlining results in huge stack frames.
• check-libcxx becomes faster by 18% when always_inline is removed

[EricWF]

I'm not sure about this patch quite yet.

However I am sure windows still needs these even in an unstable build.

[EricWF]

• I would love to have some documentation in libc++ about always-inline, the ABI problem it is trying to solve, and how it falls short. Because you tried to fix these semantics in clang would you be willing to write up a short summary of when always-inline does and doesn't work.
• If this is about always-inline semantics why are we turning off the hidden-visibility part of the macro?
• Can you offer other approaches for hiding internal symbols such as helper functions.? For example

inline _LIBCPP_INLINE_VISIBILITY __foo_impl(); // current way.
static inline _LIBCPP_HIDDEN __foo_impl(); // better way?
namespace { inline __foo_impl(); } // a (probably bad) possibility

• The test suite speedup is amazing. Is it possible to safely remove the __always_inline__ attribute from functions in the stable ABI configuration?

It seems like always-inline semantics can hurt our users in debug builds and we would benefit from drastically reducing its usage. However I need to understand exactly what __always_inline__ buys us in ABI stability before I can make an informed decision on changing its usage.

[EricWF]

Setting "Request Changes" so that phabricator bins this correctly.

[eugenis]

I've wrote down my understanding of the problem in ABIVersioning.rst.
AFAIK, Howard added the attribute so he might know more about it.

Why does windows need this attributes in the unstable abi build?

Hidden visibility and always_inline work together for the same goal, so I'm removing them both. Keeping hidden visibility but removing always inline would break things because user code will expect external definitions for class members that would not be there.

Other approaches don't work for class members. AFAIK, there is no way in C++ to declare a class member function to have an internal linkage. In C this is done with the "static" keywords, but C++ went ahead an redefined it to mean something completely different.

One idea that we had for this was to introduce a attribute((internal_linkage)) and use it in place of always_inline with the supporting compiler.

I remeasured the test suite speed up on a different, faster machine and it is just 10%. Still quite a lot.

docs/DesignDocs/ABIVersioning.rst

 To support both cases, libc++ allows specifying the ABI version at the
 build time.  The version is defined with a cmake option
 LIBCXX_ABI_VERSION. Another option LIBCXX_ABI_UNSTABLE can be used to
 include all present ABI breaking features. These options translate
 into C++ macro definitions _LIBCPP_ABI_VERSION, _LIBCPP_ABI_UNSTABLE.
 
 Any ABI-changing feature is placed under it's own macro, _LIBCPP_ABI_XXX, which is enabled
 based on the value of _LIBCPP_ABI_VERSION. _LIBCPP_ABI_UNSTABLE, if set, enables all features at once.
+
+========================
+Always_inline attributes
+========================
+
+To ensure ABI stability, libc++ attempts to tightly control the set of
+symbols exported from libc++.so.  This is achieved with a "trick" of
+marking all functions and class methods defined in the library headers
+on Linux/MacOSX with::
+
+  __attribute__((always_inline, visibility("hidden")))
+
+All direct calls to functions marked with the always_inline attribute
+are inlined and are never emitted as an external symbol
+reference. Hidden visibility removes the symbol from the shared
+library exports list.
+
+This approach has a number of drawbacks.
+
+* always_inline functions are not always inlined. Inlining is an
+  optimization, and there are multiple cases when a compiler can
+  decide against it.
+
+  * As of version 3.7, Clang can sometimes emit an **unreachable**
+    call to an always_inline function as an external symbol reference.
+  * Inlining may be impossible between functions with incompatible
+    attributes. For example, ``__attribute__((no_sanitize_address))``,
+    which suppresses AddressSanitizer instrumentation in a function,
+    prevents inlining of functions that do not have the same
+    attribute.
+
+* At -O0 optimization level indiscriminate inlining results in very
+  large stack frames. As a consequence, removing these attributes
+  speeds up libc++ test suite by 10%.
+
+These attributes are removed when building with
+LIBCXX_ABI_UNSTABLE=ON.

include/__config

 #ifndef _LIBCPP_ABI_VERSION
 #define _LIBCPP_ABI_VERSION 1
 #endif
 
 #if defined(_LIBCPP_ABI_UNSTABLE) || _LIBCPP_ABI_VERSION >= 2
 #define _LIBCPP_ABI_ALTERNATE_STRING_LAYOUT
 #endif
 
+#if defined(_LIBCPP_ABI_UNSTABLE)
+// The only purpose of the always_inline thing is ABI stability.
+// Disable it when building for unstable ABI.
+#define _LIBCPP_INLINE_VISIBILITY
+#define _LIBCPP_ALWAYS_INLINE
+#endif
+
 #define _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_X##_LIBCPP_Y
 #define _LIBCPP_CONCAT(_LIBCPP_X,_LIBCPP_Y) _LIBCPP_CONCAT1(_LIBCPP_X,_LIBCPP_Y)
 
 #define _LIBCPP_NAMESPACE _LIBCPP_CONCAT(__,_LIBCPP_ABI_VERSION)
 
 
 #ifndef __has_attribute
 #define __has_attribute(__x) 0