[libc++] Add a key function for bad_function_call

[smeenai]

Summary

bad_function_call is currently an empty class, so any object files using
that class will end up with their own copy of its typeinfo, typeinfo
name and vtable, leading to unnecessary duplication that has to be
resolved by the dynamic linker. Instead, give bad_function_call a key
function and put a definition for that key function in libc++ itself, to
centralize the typeinfo and vtable.

This is consistent with the behavior for other exception classes. The
key functions are defined in libc++ rather than libc++abi since the
class is defined in the libc++ versioning namespace, so ABI
compatibility with libstdc++ is not a concern.

Guard this change behind an ABI macro, since it isn't backwards
compatible (i.e., clients built against the new libc++ headers wouldn't
be able to run against an older libc++ library).

[smeenai]

lib/CMakeLists.txt

162 ↗

(On Diff #80191)

This may seem like a completely unrelated change, but it's actually important to force cmake to get re-run so that the glob for source files picks up on the new file.

[smeenai]

src/functional.cpp

1 ↗

(On Diff #80191)

Should I clang-format new files? I based the style of this file on the existing source files, but clang-format suggests something very different.

[EricWF]

I wonder if we should consider this a breaking ABI change and control it using a _LIBCPP_ABI macro. @mclow.lists thoughts?

[EricWF]

src/functional.cpp

1 ↗

(On Diff #80191)

That's at your discretion for source/headers. This file seems fine as-is.

[smeenai]

In D27387#612975, @EricWF wrote:

I wonder if we should consider this a breaking ABI change and control it using a _LIBCPP_ABI macro. @mclow.lists thoughts?

This is forward-compatible (as in clients built against an older libc++ would be happy with this version) but not backwards-compatible (as in clients built against this version would not be able to run against an older libc++). Has libc++ been aiming to maintain compatibility in both directions?

[EricWF]

In D27387#613071, @smeenai wrote:

In D27387#612975, @EricWF wrote:

I wonder if we should consider this a breaking ABI change and control it using a _LIBCPP_ABI macro. @mclow.lists thoughts?

This is forward-compatible (as in clients built against an older libc++ would be happy with this version) but not backwards-compatible (as in clients built against this version would not be able to run against an older libc++). Has libc++ been aiming to maintain compatibility in both directions?

Hmm, I'm not exactly sure. We don't make backward incompatible changes to existing code often. I wonder if vendors like Apple require such backwards compatibility. Maybe @dexonsmith can weigh in?

[EricWF]

OK. I would like to see this change introduced under a _LIBCPP_ABI flag. I'll take a look at this again after that.

[smeenai]

Guarding behind ABI macro

[smeenai]

Ping.

[EricWF]

include/functional

1393 ↗

(On Diff #88627)

What's the rationale for making the dtor out-of-line? Couldn't we just use what() as the key function?

lib/CMakeLists.txt

162 ↗

(On Diff #80191)

Glad you're on top of this :-)

src/functional.cpp

11 ↗

(On Diff #88627)

Nit, please put the guard inside of the namespace at the smallest possible scope.

Also there is no need to manually include __config before including functional.

[smeenai]

include/functional

1393 ↗

(On Diff #88627)

We could, but this way it's consistent with all the other exception classes.

src/functional.cpp

11 ↗

(On Diff #88627)

Will do.

[EricWF]

include/functional

1393 ↗

(On Diff #88627)

OK works for me. since it's a virtual function we probably won't see any benefits from inlining.

[smeenai]

Address comments

[smeenai]

Ping.

[jyknight]

Since this is only adding a new export to libc++, not changing/breaking existing ones, I don't think it should be grouped with the ABI-breaking changes in v2.

The usual promise is that upgrading libc++.so.1 will not break apps compiled against an older set of headers, not that you're able to compile against a new set of headers and then run on an old libc++.so.1.

That is, even if the use of the new export is put under a #ifdef, it seems like it ought to default "on".

[smeenai]

@jyknight http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161205/178830.html has the reasoning for guarding behind an ABI macro (with @dexonsmith from Apple weighing in).

[EricWF]

This LGTM, but I have a question: Should we add these dylib definitions right away so that it's easier to adopt the ABI change in future? This will allow legacy dylibs to support this ABI change in future. If you agree we can make that change as a follow up commit.

[smeenai]

In D27387#711866, @EricWF wrote:

This LGTM, but I have a question: Should we add these dylib definitions right away so that it's easier to adopt the ABI change in future? This will allow legacy dylibs to support this ABI change in future. If you agree we can make that change as a follow up commit.

As in, leave the header as-is, but make the definitions in functional.cpp unconditional? I'm fine with that.

libcxx/trunk/include/__config

 // provided under the alternate keyword __nullptr, which changes the mangling
 // of nullptr_t. This option is ABI incompatible with GCC in C++03 mode.
 #define _LIBCPP_ABI_ALWAYS_USE_CXX11_NULLPTR
 // Define the `pointer_safety` enum as a C++11 strongly typed enumeration
 // instead of as a class simulating an enum. If this option is enabled
 // `pointer_safety` and `get_pointer_safety()` will no longer be available
 // in C++03.
 #define _LIBCPP_ABI_POINTER_SAFETY_ENUM_TYPE
+// Define a key function for `bad_function_call` in the library, to centralize
+// its vtable and typeinfo to libc++ rather than having all other libraries
+// using that class define their own copies.
+#define _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
 #elif _LIBCPP_ABI_VERSION == 1
 #if !defined(_WIN32)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries.
 #define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS
 #endif
 // Feature macros for disabling pre ABI v1 features. All of these options
 // are deprecated.

libcxx/trunk/include/functional

 //                                FUNCTION
 //==============================================================================
 
 // bad_function_call
 
 class _LIBCPP_EXCEPTION_ABI bad_function_call
     : public exception
 {
+#ifdef _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
+public:
+    virtual ~bad_function_call() _NOEXCEPT;
+
+    virtual const char* what() const _NOEXCEPT;
+#endif
 };
 
 _LIBCPP_NORETURN inline _LIBCPP_ALWAYS_INLINE
 void __throw_bad_function_call()
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     throw bad_function_call();
 #else

libcxx/trunk/lib/CMakeLists.txt

add_link_flags(
       "-Wl,-force_symbols_not_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/notweak.exp"
       "-Wl,-force_symbols_weak_list,${CMAKE_CURRENT_SOURCE_DIR}/weak.exp")
   endif()
 endif()
 
 split_list(LIBCXX_COMPILE_FLAGS)
 split_list(LIBCXX_LINK_FLAGS)
 
-# Add a object library that contains the compiled source files.
+# Add an object library that contains the compiled source files.
 add_library(cxx_objects OBJECT ${exclude_from_all} ${LIBCXX_SOURCES} ${LIBCXX_HEADERS})
 if(WIN32 AND NOT MINGW)
   target_compile_definitions(cxx_objects
                              PRIVATE
                                # Ignore the -MSC_VER mismatch, as we may build
                                # with a different compatibility version.
                                _ALLOW_MSC_VER_MISMATCH
                                # Don't check the msvcprt iterator debug levels

libcxx/trunk/src/functional.cpp

+//===----------------------- functional.cpp -------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "functional"
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
+#ifdef _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION
+bad_function_call::~bad_function_call() _NOEXCEPT
+{
+}
+
+const char*
+bad_function_call::what() const _NOEXCEPT
+{
+    return "std::bad_function_call";
+}
+#endif
+
+_LIBCPP_END_NAMESPACE_STD