This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Always define a key function for std::bad_function_call in the dylib
ClosedPublic

Authored by var-const on Dec 1 2020, 8:45 AM.

Details

Summary

However, whether applications rely on the std::bad_function_call vtable
being in the dylib is still controlled by the ABI macro, since changing
that would be an ABI break.

Also separate preprocessor definitions for whether to use a key function
and whether to use a bad_function_call-specific what message
(what message is mandated by LWG2233).

Diff Detail

Event Timeline

ldionne created this revision.Dec 1 2020, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 8:45 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne requested review of this revision.Dec 1 2020, 8:45 AM
ldionne updated this revision to Diff 309317.Dec 3 2020, 11:10 AM

Update the ABI lists

ldionne updated this revision to Diff 383147.Oct 28 2021, 1:34 PM
ldionne added a subscriber: var-const.

Rebase onto main. @var-const in case you want to take a look

This revision was not accepted when it landed; it landed in state Needs Review.Nov 8 2021, 12:41 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
ldionne reopened this revision.Nov 8 2021, 6:34 AM
var-const commandeered this revision.Nov 12 2021, 6:37 PM
var-const added a reviewer: ldionne.

I'll be taking over this patch as we discussed.

  • separate defining a key function from providing a class-specific what message;
  • update the ABI list;
  • mark a test that produces a false ASan positive on Linux as unsupported with ASan.
var-const added inline comments.Nov 14 2021, 11:12 PM
libcxx/include/__functional/function.h
52–53 ↗(On Diff #387157)

Alternatively, we could also switch on whether _LIBCPP_ABI_BAD_FUNCTION_CALL_KEY_FUNCTION is defined and define what inline if it's not. That would prevent _LIBCPP_ABI_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE from unintentionally serving as another way of defining a key function.

libcxx/lib/abi/CHANGELOG.TXT
16 ↗(On Diff #387157)

Is this still the right version?

libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
3 ↗(On Diff #387157)

This is from running generate-cxx-abilist on Linux (in a Docker container). I'm not sure about exception::what, but some symbols below (e.g. _ZTVNSt3__120__time_get_c_storageIcEE) seem clearly unrelated -- perhaps the ABI list wasn't regenerated on Linux at some point?

ldionne requested changes to this revision.Nov 15 2021, 10:43 AM

There seems to be an issue in how you uploaded the patch and it's unable to apply the diff, which is why CI is failing right now. I suspect you uploaded a diff on top of my previous diff -- you should squash those commits.

libcxx/include/__config
92–94

I think you had found a LWG issue that discussed this? If it is relevant, you could also add a link to it here.

libcxx/include/__functional/function.h
39–42 ↗(On Diff #387157)

I don't think this commit is useful, since there is already one in __config:81. In case you feel the comment in __config is not sufficient, I would improve that one instead of adding a new one here.

However, I would add a comment here saying that when we don't define a key function, every TU that uses bad_function_call will get a weak definition of the vtable.

49–51 ↗(On Diff #387157)

Same for this comment -- I don't think this adds anything beyond the comment in __config:85. If there is information you want to add, I would do it where we document the macro, in __config.

52–53 ↗(On Diff #387157)

Hmm, interesting suggestion but I don't think this buys us anything since people who enable the good what() message need to be breaking the ABI anyway.

libcxx/lib/abi/CHANGELOG.TXT
16 ↗(On Diff #387157)

Yes, 14.0 is the next release, so this is accurate.

libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
3 ↗(On Diff #387157)

This is a bit strange indeed. I would suggest you don't add these unrelated symbols and see what the CI says. Normally, we're checking this list in the CI so it should never get out of sync.

libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.inv/invoke.pass.cpp
18–23 ↗(On Diff #387157)
This revision now requires changes to proceed.Nov 15 2021, 10:43 AM
var-const marked 5 inline comments as done.
  • squash two commits into one;
  • address feedback;
  • slightly expand the commit message.
ldionne accepted this revision.Nov 15 2021, 1:50 PM

This LGTM, but I don't understand why the ABI list doesn't contain _ZNKSt9exception4whatEv anymore. Let's try to figure that out before we land.

This seemingly simple patch is full of surprises.

This revision is now accepted and ready to land.Nov 15 2021, 1:50 PM
var-const updated this revision to Diff 387409.Nov 15 2021, 2:38 PM

Update the exported ABI symbols on Linux (copied from CI).

var-const added inline comments.Nov 15 2021, 2:41 PM
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
3 ↗(On Diff #387157)

I copied this file from the CI now, as we discussed. Most extra symbols are gone, but this one persists. Should I add it to the changelog?

This LGTM, but I don't understand why the ABI list doesn't contain _ZNKSt9exception4whatEv anymore. Let's try to figure that out before we land.

This seemingly simple patch is full of surprises.

Hmm, I don't know how exactly the generate-cxx-abilist build works, but I presumed that it defines _LIBCPP_BUILDING_LIBRARY but doesn't define GOOD_WHAT_MESSAGE, which I think would lead to the output we're seeing.

var-const updated this revision to Diff 387435.Nov 15 2021, 4:14 PM
  • mark the other two failing tests as unsupported with ASan.
var-const edited the summary of this revision. (Show Details)Nov 15 2021, 4:14 PM
var-const updated this revision to Diff 387473.Nov 15 2021, 6:46 PM
  • add one more ABI list from the CI.
ldionne accepted this revision.EditedNov 16 2021, 7:56 AM
ldionne added a subscriber: dim.

IMO this is safe to land, however please add std::exception::what() const to the ABI changelog for Linux.

libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
3 ↗(On Diff #387157)

Yeah, add it to the changelog for now.

I didn't understand why this symbol is marked as needed, so I did some investigation. It turns out that std::exception::what() const is defined in libc++abi. Previously, libc++ would NOT have any dependency on std::exception::what() const, because all the exception classes would define their own what() method. However, with this change, the bad_function_call vtable inside libc++.dylib contains std::exception::what() const since it is not overriden in bad_function_call, leading to an undefined reference to that symbol in libc++.dylib.

I don't think this is an issue, because folks should already be linking against a libc++abi.so that provides the symbol. @dim I think you folks are using libcxxrt as an ABI library -- can you confirm that you're providing std::exception::what() const in that ABI library?

  • mention exception::what in the ABI changelog for Linux;
  • rebase on main.
This revision was landed with ongoing or failed builds.Nov 16 2021, 11:26 AM
This revision was automatically updated to reflect the committed changes.
dim added a subscriber: emaste.Nov 17 2021, 1:23 AM
dim added inline comments.
libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist
3 ↗(On Diff #387157)