Page MenuHomePhabricator

[libc++] Add a key function for bad_function_call
ClosedPublic

Authored by smeenai on Dec 3 2016, 2:19 PM.

Details

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).

Diff Detail

Repository
rL LLVM

Event Timeline

smeenai updated this revision to Diff 80191.Dec 3 2016, 2:19 PM
smeenai retitled this revision from to [libc++] Add a key function for bad_function_call.
smeenai updated this object.
smeenai added reviewers: EricWF, mclow.lists.
smeenai added a subscriber: cfe-commits.
smeenai added inline comments.Dec 3 2016, 2:20 PM
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 added inline comments.Dec 3 2016, 2:25 PM
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 edited edge metadata.Dec 4 2016, 12:45 PM

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

EricWF added inline comments.Dec 4 2016, 12:49 PM
src/functional.cpp
1 ↗(On Diff #80191)

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

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 added a subscriber: dexonsmith.

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 requested changes to this revision.Dec 10 2016, 11:56 PM
EricWF edited edge metadata.

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

This revision now requires changes to proceed.Dec 10 2016, 11:56 PM
smeenai updated this revision to Diff 88627.Feb 15 2017, 4:31 PM
smeenai edited edge metadata.
smeenai edited the summary of this revision. (Show Details)
smeenai removed a reviewer: dexonsmith.
smeenai removed a subscriber: dexonsmith.

Guarding behind ABI macro

EricWF added inline comments.Feb 28 2017, 9:32 PM
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 added inline comments.Feb 28 2017, 11:13 PM
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 added inline comments.Feb 28 2017, 11:23 PM
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 updated this revision to Diff 90524.Mar 3 2017, 1:08 PM

Address comments

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".

@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 accepted this revision.Mar 27 2017, 9:53 PM

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.

This revision is now accepted and ready to land.Mar 27 2017, 9:53 PM

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.

This revision was automatically updated to reflect the committed changes.