Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make __libcpp_verbose_abort() noexcept like std::terminate() #109151

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

dougsonos
Copy link
Contributor

Make __libcpp_verbose_abort() noexcept (it is already noreturn), to match std::terminate(). Clang's function effect analysis can use this to ignore such functions as being beyond its scope. (See #99656)

@dougsonos dougsonos requested a review from a team as a code owner September 18, 2024 14:53
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Sep 18, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 18, 2024

@llvm/pr-subscribers-libcxx

Author: Doug Wyatt (dougsonos)

Changes

Make __libcpp_verbose_abort() noexcept (it is already noreturn), to match std::terminate(). Clang's function effect analysis can use this to ignore such functions as being beyond its scope. (See #99656)


Full diff: https://github.com/llvm/llvm-project/pull/109151.diff

1 Files Affected:

  • (modified) libcxx/include/__verbose_abort (+2-1)
diff --git a/libcxx/include/__verbose_abort b/libcxx/include/__verbose_abort
index 244278aec652d2..45a0add493c17d 100644
--- a/libcxx/include/__verbose_abort
+++ b/libcxx/include/__verbose_abort
@@ -21,7 +21,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 // This function should never be called directly from the code -- it should only be called through
 // the _LIBCPP_VERBOSE_ABORT macro.
 [[__noreturn__]] _LIBCPP_AVAILABILITY_VERBOSE_ABORT _LIBCPP_OVERRIDABLE_FUNC_VIS
-_LIBCPP_ATTRIBUTE_FORMAT(__printf__, 1, 2) void __libcpp_verbose_abort(const char* __format, ...);
+_LIBCPP_ATTRIBUTE_FORMAT(__printf__, 1, 2) void __libcpp_verbose_abort(const char* __format, ...)
+_NOEXCEPT;
 
 // _LIBCPP_VERBOSE_ABORT(format, args...)
 //

Copy link

github-actions bot commented Sep 18, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@dougsonos dougsonos marked this pull request as draft September 18, 2024 14:57
@dougsonos
Copy link
Contributor Author

Oops, I forgot to make the implementation noexcept too. This highlights that the question here is whether changing this interface to noexcept is going to break anything...

@ldionne ldionne marked this pull request as ready for review September 18, 2024 16:02
Copy link
Member

@ldionne ldionne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, however this is technically a breaking change because in the past we have advertised __libcpp_verbose_abort as being overridable (that's why it was a weak def).

We'd also want to add a release note for this change in libcxx/docs/ReleaseNotes/20.rst, probably under Deprecations and Removals.

Pinging @llvm/libcxx-vendors for awareness.

@dougsonos
Copy link
Contributor Author

Any pointers on what's going on with the test builds? I click Details and just see "job failed".

@ldionne
Copy link
Member

ldionne commented Sep 23, 2024

Any pointers on what's going on with the test builds? I click Details and just see "job failed".

Our CI is currently in a really bad state. Workers are failing to connect. This has been going on for a few days and we're trying to resolve the issue. Sorry for the inconvenience.

@ldionne
Copy link
Member

ldionne commented Sep 25, 2024

Please rebase onto main to re-trigger CI. The CI instability should be resolved now.

match  std::terminate(). Clang's function effect analysis can use this
to ignore such functions as being beyond its scope.
@dougsonos
Copy link
Contributor Author

dougsonos commented Oct 5, 2024

I should have trusted the CI and seen this error!

# | /home/runner/_work/llvm-project/llvm-project/libcxx/test/libcxx/assertions/customize_verbose_abort.link-time.pass.cpp:18:52: error: expected function body after function declarator
# |    18 | void std::__libcpp_verbose_abort(char const*, ...) noexcept { std::exit(EXIT_SUCCESS); }
# |       |                                                    ^
# | 1 error generated.

This is in a C++03 test.

@dougsonos
Copy link
Contributor Author

Ping

@ldionne ldionne merged commit 9548366 into llvm:main Oct 17, 2024
65 checks passed
@BwL1289
Copy link

BwL1289 commented Oct 18, 2024

@dougsonos this breaks flang compilation because flang overrides this symbol and doesn't use noexcept attribute

@dougsonos
Copy link
Contributor Author

@dougsonos this breaks flang compilation because flang overrides this symbol and doesn't use noexcept attribute

Apologies. Can we fix flang to match?

@ldionne
Copy link
Member

ldionne commented Oct 18, 2024

@dougsonos this breaks flang compilation because flang overrides this symbol and doesn't use noexcept attribute

In fact, we want to move away from supporting overriding that function. The way of customizing the assertion handler is when configuring the library, as explained here: https://libcxx.llvm.org/Hardening.html#overriding-the-assertion-failure-handler

@mstorsjo
Copy link
Member

@dougsonos this breaks flang compilation because flang overrides this symbol and doesn't use noexcept attribute

In fact, we want to move away from supporting overriding that function. The way of customizing the assertion handler is when configuring the library, as explained here: https://libcxx.llvm.org/Hardening.html#overriding-the-assertion-failure-handler

@ldionne - The issue here from the Flang perspective, is not that flang want to override the function, they don't really care. The thing is that Flang uses some classes from the C++ standard library (from std::variant iirc) but don't want to link against the C++ standard library. When using the libc++ implementation of std::variant, they end up with undefined references to std::__libcpp_verbose_abort unless they actually override/provide it themselves.

@ldionne
Copy link
Member

ldionne commented Oct 21, 2024

Moving the discussion to #113059 (comment) to centralize things.

@Caslyn
Copy link
Contributor

Caslyn commented Oct 21, 2024

Hi @dougsonos @ldionne (sorry, typo on the author name)

Unfortunately this is breaking when trying to build Fuchsia with a clang version that includes this change. Here is a link to the build failure we're seeing:

https://luci-milo.appspot.com/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.arm64-debug/b8733819071213133329/overview

There are places in the Fuchsia checkout that provide their own definitions of this function:

https://cs.opensource.google/search?q=__libcpp_verbose_abort

Can you make this backwards compatible to enable us to transition to the noexcept version of this function?

EricWF pushed a commit to efcs/llvm-project that referenced this pull request Oct 22, 2024
…llvm#109151)

Make __libcpp_verbose_abort() noexcept (it is already noreturn), to
match std::terminate(). Clang's function effect analysis can use this to
ignore such functions as being beyond its scope. (See
llvm#99656).
ldionne added a commit that referenced this pull request Oct 23, 2024
…xcept again (#113310)

This allows a slightly smoother transition for people after #109151, as
requested on that PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants