This results in proper error messages instead of just an abort.
Details
- Reviewers
ldionne Mordante var-const huixie90 - Group Reviewers
Restricted Project - Commits
- rG16d1b0e1059e: [libc++] Use __verbose_abort instead of std::abort in __throw_ functions
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I really like this change! I think it would be good to let @ldionne to have a look too.
libcxx/include/__expected/expected.h | ||
---|---|---|
53 | I wonder if we can remove this now. (Maybe we need to remove the public dependency too) |
libcxx/include/__expected/expected.h | ||
---|---|---|
53 | I've already got a patch for that locally. |
This is great. I actually thought I had a local patch doing this, but I can't find it. At least it's been on my mind for a while, and in fact it was the main reason why I generalized from __libcpp_assertion_handler to __libcpp_verbose_abort.
One thing to keep in mind is that calling __libcpp_verbose_abort may generate larger code than calling abort() did, which could be annoying for folks compiling with -fno-exceptions. I think we may want to e.g. mark __libcpp_verbose_abort as cold to help the compiler optimize it better when possible, but at the end of the day, the calling convention for variadic functions on some architectures like arm64 isn't super size-friendly. I don't think this should prevent us from making this change, but size regressions should be on our radar.
Pinging vendors just for awareness, in particular I think Google doesn't use exceptions so they may be able to give us some early feedback on this change.
libcxx/include/future | ||
---|---|---|
536–537 | You're using the %s format specifier but you're passing a std::error_code, which seems wrong. | |
537 | ||
libcxx/include/ios | ||
455 |
Looking at that patch again, I feel like the additional
_LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI inline void __exception_abort(char const* __file, int __line, char const* __msg) { std::__libcpp_verbose_abort("%s:%d: Exception thrown with exceptions disabled: %s\n", __file, __line, __msg); }
and
_LIBCPP_NORETURN _LIBCPP_HIDE_FROM_ABI inline void __assertion_abort(char const* __file, int __line, char const* __expr, char const* __msg) { std::__libcpp_verbose_abort("%s:%d: Assertion %s failed: %s\n", __file, __line, __expr, __msg); }
did make sense, no?
The compiler already considers [[noreturn]] functions to be extremely unlikely: https://godbolt.org/z/9xoYWfK4T. So I don't think we have to give the compiler extra hints.
I don't think so. This would only give people the __throw_whatever functions, which seems not exactly useful.
LGTM w/ passing CI. Please ping me again if you have to change the patch non-trivially to make CI pass.
I believe this change broke the LLDB tests. Can you please fix or revert?
Details: https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52383/
====================================================================== FAIL: test_dsym (TestDbgInfoContentVectorFromStdModule.TestDbgInfoContentVector) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1707, in test_method return attrvalue(self) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 159, in wrapper return func(*args, **kwargs) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 159, in wrapper return func(*args, **kwargs) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py", line 64, in test self.expect_expr("a.at(0).a", result_type="int", result_value="2") File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2463, in expect_expr value_check.check_value(self, eval_result, str(eval_result)) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 287, in check_value test_base.assertSuccess(val.GetError()) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2499, in assertSuccess self.fail(self._formatMessage(msg, AssertionError: 'expression failed to parse: error: Couldn't lookup symbols: __ZNSt3__122__libcpp_verbose_abortEPKcz ' is not success Config=x86_64-/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang ====================================================================== FAIL: test_dwarf (TestDbgInfoContentVectorFromStdModule.TestDbgInfoContentVector) ---------------------------------------------------------------------- Traceback (most recent call last): File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1707, in test_method return attrvalue(self) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 159, in wrapper return func(*args, **kwargs) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/decorators.py", line 159, in wrapper return func(*args, **kwargs) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/test/API/commands/expression/import-std-module/vector-dbg-info-content/TestDbgInfoContentVectorFromStdModule.py", line 64, in test self.expect_expr("a.at(0).a", result_type="int", result_value="2") File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2463, in expect_expr value_check.check_value(self, eval_result, str(eval_result)) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 287, in check_value test_base.assertSuccess(val.GetError()) File "/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2499, in assertSuccess self.fail(self._formatMessage(msg, AssertionError: 'expression failed to parse: error: Couldn't lookup symbols: __ZNSt3__122__libcpp_verbose_abortEPKcz ' is not success Config=x86_64-/Users/buildslave/jenkins/workspace/lldb-cmake/lldb-build/bin/clang ----------------------------------------------------------------------
There's a comment next to the __libcpp_verbose_abort definition that says
// This function should never be called directly from the code -- it should only be called through // the _LIBCPP_VERBOSE_ABORT macro.
Does this apply here? For context, we have a libc-free project that uses variant and optional and this change pulls in vfprintf. We'd like to just provide our own _LIBCPP_VERBOSE_ABORT, or build with -D_LIBCPP_HAS_NO_VERBOSE_ABORT_IN_LIBRARY but it doesn't look like we can do that if the code calls __libcpp_verbose_abort directly.
I wonder if we can remove this now. (Maybe we need to remove the public dependency too)