This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use __verbose_abort instead of std::abort in __throw_ functions
ClosedPublic

Authored by philnik on Jan 8 2023, 5:08 AM.

Details

Summary

This results in proper error messages instead of just an abort.

Diff Detail

Event Timeline

philnik created this revision.Jan 8 2023, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 5:08 AM
Herald added a subscriber: smeenai. · View Herald Transcript
philnik requested review of this revision.Jan 8 2023, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 8 2023, 5:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
philnik updated this revision to Diff 487182.Jan 8 2023, 6:48 AM

Try to fix CI

Mordante accepted this revision as: Mordante.Jan 8 2023, 7:49 AM

I really like this change! I think it would be good to let @ldionne to have a look too.

huixie90 added inline comments.Jan 8 2023, 4:09 PM
libcxx/include/__expected/expected.h
53

I wonder if we can remove this now. (Maybe we need to remove the public dependency too)

philnik added inline comments.Jan 8 2023, 4:10 PM
libcxx/include/__expected/expected.h
53

I've already got a patch for that locally.

ldionne added a subscriber: Restricted Project.Jan 9 2023, 9:52 AM

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

Oh actually I had created D131619, that's what I was looking for.

Oh actually I had created D131619, that's what I was looking for.

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?

philnik updated this revision to Diff 501551.Mar 1 2023, 9:39 AM
philnik marked 3 inline comments as done.

Address comments

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.

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.

Oh actually I had created D131619, that's what I was looking for.

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?

I don't think so. This would only give people the __throw_whatever functions, which seems not exactly useful.

ldionne accepted this revision.Mar 13 2023, 8:17 AM

LGTM w/ passing CI. Please ping me again if you have to change the patch non-trivially to make CI pass.

This revision is now accepted and ready to land.Mar 13 2023, 8:17 AM
philnik updated this revision to Diff 504786.Mar 13 2023, 12:01 PM

Try to fix CI

philnik updated this revision to Diff 505109.Mar 14 2023, 8:14 AM

Try to fix CI

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
----------------------------------------------------------------------
jgorbe added a subscriber: jgorbe.Mar 15 2023, 3:49 PM

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.

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.

+1 we are running into this too

Should we revert this?

D146227 is a fix diff that's up and waiting for CI.