[libcxx-commits] [PATCH] D92397: [libc++] Always define a key function for std::bad_function_call in the dylib

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: libcxx/include/__config:85-87
+// Override the default return value of `what()` function that comes from
+// `std::exception` with a string that is specific to `bad_function_call`.
+#  define _LIBCPP_ABI_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE
----------------
I think you had found a LWG issue that discussed this? If it is relevant, you could also add a link to it here.


================
Comment at: libcxx/include/__functional/function.h:39-42
+// Using a key function allows emitting a vtable and typeinfo for the class in
+// a single translation unit. Originally `bad_function_call` didn't use a key
+// function, and because changing this behavior constitutes an ABI break,
+// it is controlled by a preprocessor definition.
----------------
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.


================
Comment at: libcxx/include/__functional/function.h:49-51
+// Whether to use a `what` message specific to `bad_function_call`. This
+// behavior is controlled by a preprocessor definition because changing it
+// constitutes an ABI break (with `what` serving as a key function).
----------------
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`.


================
Comment at: libcxx/include/__functional/function.h:52-53
+// constitutes an ABI break (with `what` serving as a key function).
+#ifdef _LIBCPP_ABI_BAD_FUNCTION_CALL_GOOD_WHAT_MESSAGE
     virtual const char* what() const _NOEXCEPT;
 #endif
----------------
var-const wrote:
> 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.
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.


================
Comment at: libcxx/lib/abi/CHANGELOG.TXT:16
 ------------
 Version 14.0
 ------------
----------------
var-const wrote:
> Is this still the right version?
Yes, 14.0 is the next release, so this is accurate.


================
Comment at: libcxx/lib/abi/x86_64-unknown-linux-gnu.libcxxabi.v1.stable.exceptions.no_new_in_libcxx.abilist:3
 {'is_defined': False, 'name': '_ZNKSt13runtime_error4whatEv', 'type': 'FUNC'}
+{'is_defined': False, 'name': '_ZNKSt9exception4whatEv', 'type': 'FUNC'}
 {'is_defined': False, 'name': '_ZNSt11logic_errorD2Ev', 'type': 'FUNC'}
----------------
var-const wrote:
> 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?
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.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.func/func.wrap.func.inv/invoke.pass.cpp:18-23
+// Address Sanitizer doesn't instrument weak symbols on Linux. Because the key
+// function is conditionally defined for bad_function_call, its typeinfo and
+// vtable will be defined as strong symbols in the library and weak symbols in
+// other translation units. Only the strong symbol will be instrumented,
+// increasing its size and leading to a serious ODR violation resulting in
+// a crash.
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92397/new/

https://reviews.llvm.org/D92397