* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-05-24 18:55 [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720] Jason Merrill
@ 2023-05-26 9:58 ` Jonathan Wakely
2023-06-08 13:13 ` Jonathan Wakely
2023-06-02 16:56 ` PING Re: [PATCH RFA (tree-eh)] " Jason Merrill
2023-06-03 14:33 ` [PATCH RFC] " Jeff Law
2 siblings, 1 reply; 12+ messages in thread
From: Jonathan Wakely @ 2023-05-26 9:58 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches, libstdc++
[-- Attachment #1: Type: text/plain, Size: 10170 bytes --]
On Wed, 24 May 2023 at 19:56, Jason Merrill via Libstdc++ <
libstdc++@gcc.gnu.org> wrote:
> Middle-end folks: any thoughts about how best to make the change described
> in
> the last paragraph below?
>
> Library folks: any thoughts on the changes to __cxa_call_terminate?
>
I see no harm in exporting it (with the adjusted signature). The "looks
standard but isn't" name is a little unfortunate, but not a big deal.
>
> -- 8< --
>
> [except.handle]/7 says that when we enter std::terminate due to a throw,
> that is considered an active handler. We already implemented that properly
> for the case of not finding a handler (__cxa_throw calls __cxa_begin_catch
> before std::terminate) and the case of finding a callsite with no landing
> pad (the personality function calls __cxa_call_terminate which calls
> __cxa_begin_catch), but for the case of a throw in a try/catch in a
> noexcept
> function, we were emitting a cleanup that calls std::terminate directly
> without ever calling __cxa_begin_catch to handle the exception.
>
> A straightforward way to fix this seems to be calling __cxa_call_terminate
> instead. However, that requires exporting it from libstdc++, which we have
> not previously done. Despite the name, it isn't actually part of the ABI
> standard. Nor is __cxa_call_unexpected, as far as I can tell, but that one
> is also used by clang. For this case they use __clang_call_terminate; it
> seems reasonable to me for us to stick with __cxa_call_terminate.
>
> I also change __cxa_call_terminate to take void* for simplicity in the
> front
> end (and consistency with __cxa_call_unexpected) but that isn't necessary
> if
> it's undesirable for some reason.
>
> This patch does not fix the issue that representing the noexcept as a
> cleanup is wrong, and confuses the handler search; since it looks like a
> cleanup in the EH tables, the unwinder keeps looking until it finds the
> catch in main(), which it should never have gotten to. Without the
> try/catch in main, the unwinder would reach the end of the stack and say no
> handler was found. The noexcept is a handler, and should be treated as
> one,
> as it is when the landing pad is omitted.
>
> The best fix for that issue seems to me to be to represent an
> ERT_MUST_NOT_THROW after an ERT_TRY in an action list as though it were an
> ERT_ALLOWED_EXCEPTIONS (since indeed it is an exception-specification).
> The
> actual code generation shouldn't need to change (apart from the change made
> by this patch), only the action table entry.
>
> PR c++/97720
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (enum cp_tree_index): Add CPTI_CALL_TERMINATE_FN.
> (call_terminate_fn): New macro.
> * cp-gimplify.cc (gimplify_must_not_throw_expr): Use it.
> * except.cc (init_exception_processing): Set it.
> (cp_protect_cleanup_actions): Return it.
>
> gcc/ChangeLog:
>
> * tree-eh.cc (lower_resx): Pass the exception pointer to the
> failure_decl.
> * except.h: Tweak comment.
>
> libstdc++-v3/ChangeLog:
>
> * libsupc++/eh_call.cc (__cxa_call_terminate): Take void*.
> * config/abi/pre/gnu.ver: Add it.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/eh/terminate2.C: New test.
> ---
> gcc/cp/cp-tree.h | 2 ++
> gcc/except.h | 2 +-
> gcc/cp/cp-gimplify.cc | 2 +-
> gcc/cp/except.cc | 5 ++++-
> gcc/testsuite/g++.dg/eh/terminate2.C | 30 ++++++++++++++++++++++++++++
> gcc/tree-eh.cc | 16 ++++++++++++++-
> libstdc++-v3/libsupc++/eh_call.cc | 4 +++-
> libstdc++-v3/config/abi/pre/gnu.ver | 7 +++++++
> 8 files changed, 63 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/eh/terminate2.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index a1b882f11fe..a8465a988b5 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -217,6 +217,7 @@ enum cp_tree_index
> definitions. */
> CPTI_ALIGN_TYPE,
> CPTI_TERMINATE_FN,
> + CPTI_CALL_TERMINATE_FN,
> CPTI_CALL_UNEXPECTED_FN,
>
> /* These are lazily inited. */
> @@ -358,6 +359,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
> /* Exception handling function declarations. */
> #define terminate_fn cp_global_trees[CPTI_TERMINATE_FN]
> #define call_unexpected_fn
> cp_global_trees[CPTI_CALL_UNEXPECTED_FN]
> +#define call_terminate_fn
> cp_global_trees[CPTI_CALL_TERMINATE_FN]
> #define get_exception_ptr_fn
> cp_global_trees[CPTI_GET_EXCEPTION_PTR_FN]
> #define begin_catch_fn
> cp_global_trees[CPTI_BEGIN_CATCH_FN]
> #define end_catch_fn cp_global_trees[CPTI_END_CATCH_FN]
> diff --git a/gcc/except.h b/gcc/except.h
> index 5ecdbc0d1dc..378a9e4cb77 100644
> --- a/gcc/except.h
> +++ b/gcc/except.h
> @@ -155,7 +155,7 @@ struct GTY(()) eh_region_d
> struct eh_region_u_must_not_throw {
> /* A function decl to be invoked if this region is actually
> reachable
> from within the function, rather than implementable from the
> runtime.
> - The normal way for this to happen is for there to be a CLEANUP
> region
> + The normal way for this to happen is for there to be a TRY region
> contained within this MUST_NOT_THROW region. Note that if the
> runtime handles the MUST_NOT_THROW region, we have no control over
> what termination function is called; it will be decided by the
> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> index 216a6231d6f..853b1e44236 100644
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -343,7 +343,7 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq
> *pre_p)
> gimple *mnt;
>
> gimplify_and_add (body, &try_);
> - mnt = gimple_build_eh_must_not_throw (terminate_fn);
> + mnt = gimple_build_eh_must_not_throw (call_terminate_fn);
> gimple_seq_add_stmt_without_update (&catch_, mnt);
> mnt = gimple_build_try (try_, catch_, GIMPLE_TRY_CATCH);
>
> diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> index 91a5e049860..b04eb00d220 100644
> --- a/gcc/cp/except.cc
> +++ b/gcc/cp/except.cc
> @@ -64,6 +64,9 @@ init_exception_processing (void)
> tmp = build_function_type_list (void_type_node, ptr_type_node,
> NULL_TREE);
> call_unexpected_fn
> = push_throw_library_fn (get_identifier ("__cxa_call_unexpected"),
> tmp);
> + call_terminate_fn
> + = push_library_fn (get_identifier ("__cxa_call_terminate"), tmp,
> NULL_TREE,
> + ECF_NORETURN | ECF_COLD | ECF_NOTHROW);
> }
>
> /* Returns an expression to be executed if an unhandled exception is
> @@ -76,7 +79,7 @@ cp_protect_cleanup_actions (void)
>
> When the destruction of an object during stack unwinding exits
> using an exception ... void terminate(); is called. */
> - return terminate_fn;
> + return call_terminate_fn;
> }
>
> static tree
> diff --git a/gcc/testsuite/g++.dg/eh/terminate2.C
> b/gcc/testsuite/g++.dg/eh/terminate2.C
> new file mode 100644
> index 00000000000..1c69dab95f8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/eh/terminate2.C
> @@ -0,0 +1,30 @@
> +// PR c++/97720
> +// { dg-do run }
> +
> +// Test that there is an active exception when we reach the terminate
> handler.
> +
> +#include <exception>
> +#include <cstdlib>
> +
> +void bad_guy() throw() {
> + try { throw 0; }
> + catch (float) { }
> + // Don't catch int.
> +}
> +
> +void level1() {
> + bad_guy();
> + throw "dead code";
> +}
> +
> +void my_term()
> +{
> + try { throw; }
> + catch(...) { std::exit(0); }
> +}
> +
> +int main() {
> + std::set_terminate (my_term);
> + try { level1(); }
> + catch (int) { }
> +}
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 934209d205f..e8ceff36cc6 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -3382,8 +3382,22 @@ lower_resx (basic_block bb, gresx *stmt,
> lab = gimple_block_label (new_bb);
> gsi2 = gsi_start_bb (new_bb);
>
> + /* Handle failure fns that expect either no arguments or the
> + exception pointer. */
> fn = dst_r->u.must_not_throw.failure_decl;
> - x = gimple_build_call (fn, 0);
> + if (TYPE_ARG_TYPES (TREE_TYPE (fn)) != void_list_node)
> + {
> + tree epfn = builtin_decl_implicit (BUILT_IN_EH_POINTER);
> + src_nr = build_int_cst (integer_type_node, src_r->index);
> + x = gimple_build_call (epfn, 1, src_nr);
> + tree var = create_tmp_var (ptr_type_node);
> + var = make_ssa_name (var, x);
> + gimple_call_set_lhs (x, var);
> + gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
> + x = gimple_build_call (fn, 1, var);
> + }
> + else
> + x = gimple_build_call (fn, 0);
> gimple_set_location (x, dst_r->u.must_not_throw.failure_loc);
> gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
>
> diff --git a/libstdc++-v3/libsupc++/eh_call.cc
> b/libstdc++-v3/libsupc++/eh_call.cc
> index 3cfc71a4ef1..2bec4e83a7d 100644
> --- a/libstdc++-v3/libsupc++/eh_call.cc
> +++ b/libstdc++-v3/libsupc++/eh_call.cc
> @@ -36,8 +36,10 @@ using namespace __cxxabiv1;
> // terminate.
>
> extern "C" void
> -__cxa_call_terminate(_Unwind_Exception* ue_header) throw ()
> +__cxa_call_terminate(void* ue_header_in) throw ()
> {
> + _Unwind_Exception* ue_header
> + = reinterpret_cast<_Unwind_Exception*>(ue_header_in);
>
> if (ue_header)
> {
> diff --git a/libstdc++-v3/config/abi/pre/gnu.ver
> b/libstdc++-v3/config/abi/pre/gnu.ver
> index 768cd4a4a6c..a2e5f3b4e74 100644
> --- a/libstdc++-v3/config/abi/pre/gnu.ver
> +++ b/libstdc++-v3/config/abi/pre/gnu.ver
> @@ -2841,6 +2841,13 @@ CXXABI_1.3.14 {
>
> } CXXABI_1.3.13;
>
> +CXXABI_1.3.15 {
> +
> + global:
> + __cxa_call_terminate;
> +
> +} CXXABI_1.3.14;
> +
> # Symbols in the support library (libsupc++) supporting transactional
> memory.
> CXXABI_TM_1 {
>
>
> base-commit: 2738955004256c2e9753364d78a7be340323b74b
> --
> 2.31.1
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-05-26 9:58 ` Jonathan Wakely
@ 2023-06-08 13:13 ` Jonathan Wakely
2023-06-08 14:03 ` Jason Merrill
2023-06-09 9:02 ` Richard Biener
0 siblings, 2 replies; 12+ messages in thread
From: Jonathan Wakely @ 2023-06-08 13:13 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches, libstdc++
[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]
On Fri, 26 May 2023 at 10:58, Jonathan Wakely wrote:
>
>
> On Wed, 24 May 2023 at 19:56, Jason Merrill via Libstdc++ <
> libstdc++@gcc.gnu.org> wrote:
>
>> Middle-end folks: any thoughts about how best to make the change
>> described in
>> the last paragraph below?
>>
>> Library folks: any thoughts on the changes to __cxa_call_terminate?
>>
>
> I see no harm in exporting it (with the adjusted signature). The "looks
> standard but isn't" name is a little unfortunate, but not a big deal.
>
Jason, do you have any objection to exporting __cxa_call_terminate for GCC
13.2 as well, even though the FE won't use it?
Currently both gcc-13 and trunk are at the same library version,
libstdc++.so.6.0.32
But with this addition to trunk we need to bump that .32 to .33, meaning
that gcc-13 and trunk diverge. If we want to backport any new symbols from
trunk to gcc-13 that gets trickier once they've diverged.
If we added __cxa_call_terminate to gcc-13, making it another new addition
to libstdc++.so.6.0.32, then it would simplify a few things.
In theory it could be a problem for distros already shipping gcc-13.1.1
with that new libstdc++.so.6.0.32 version, but since the
__cxa_call_terminate symbol won't actually be used by the gcc-13.1.1
compilers, I don't think it will be a problem.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-08 13:13 ` Jonathan Wakely
@ 2023-06-08 14:03 ` Jason Merrill
2023-06-09 9:02 ` Richard Biener
1 sibling, 0 replies; 12+ messages in thread
From: Jason Merrill @ 2023-06-08 14:03 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: gcc-patches, libstdc++
[-- Attachment #1: Type: text/plain, Size: 776 bytes --]
On Thu, Jun 8, 2023 at 9:13 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On Fri, 26 May 2023 at 10:58, Jonathan Wakely wrote:
>
>>
>>
>> On Wed, 24 May 2023 at 19:56, Jason Merrill via Libstdc++ <
>> libstdc++@gcc.gnu.org> wrote:
>>
>>> Middle-end folks: any thoughts about how best to make the change
>>> described in
>>> the last paragraph below?
>>>
>>> Library folks: any thoughts on the changes to __cxa_call_terminate?
>>>
>>
>> I see no harm in exporting it (with the adjusted signature). The "looks
>> standard but isn't" name is a little unfortunate, but not a big deal.
>>
>
> Jason, do you have any objection to exporting __cxa_call_terminate for GCC
> 13.2 as well, even though the FE won't use it?
>
That sounds fine.
Jason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-08 13:13 ` Jonathan Wakely
2023-06-08 14:03 ` Jason Merrill
@ 2023-06-09 9:02 ` Richard Biener
2023-06-09 9:06 ` Jonathan Wakely
2023-06-09 9:09 ` Jakub Jelinek
1 sibling, 2 replies; 12+ messages in thread
From: Richard Biener @ 2023-06-09 9:02 UTC (permalink / raw)
To: Jonathan Wakely; +Cc: Jason Merrill, gcc-patches, libstdc++
On Thu, Jun 8, 2023 at 3:14 PM Jonathan Wakely via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Fri, 26 May 2023 at 10:58, Jonathan Wakely wrote:
>
> >
> >
> > On Wed, 24 May 2023 at 19:56, Jason Merrill via Libstdc++ <
> > libstdc++@gcc.gnu.org> wrote:
> >
> >> Middle-end folks: any thoughts about how best to make the change
> >> described in
> >> the last paragraph below?
> >>
> >> Library folks: any thoughts on the changes to __cxa_call_terminate?
> >>
> >
> > I see no harm in exporting it (with the adjusted signature). The "looks
> > standard but isn't" name is a little unfortunate, but not a big deal.
> >
>
> Jason, do you have any objection to exporting __cxa_call_terminate for GCC
> 13.2 as well, even though the FE won't use it?
>
> Currently both gcc-13 and trunk are at the same library version,
> libstdc++.so.6.0.32
>
> But with this addition to trunk we need to bump that .32 to .33, meaning
> that gcc-13 and trunk diverge. If we want to backport any new symbols from
> trunk to gcc-13 that gets trickier once they've diverged.
But if you backport any new used symbol you have to bump the version
anyway. So why not bump now (on trunk)?
> If we added __cxa_call_terminate to gcc-13, making it another new addition
> to libstdc++.so.6.0.32, then it would simplify a few things.
>
> In theory it could be a problem for distros already shipping gcc-13.1.1
> with that new libstdc++.so.6.0.32 version, but since the
> __cxa_call_terminate symbol won't actually be used by the gcc-13.1.1
> compilers, I don't think it will be a problem.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-09 9:02 ` Richard Biener
@ 2023-06-09 9:06 ` Jonathan Wakely
2023-06-09 9:09 ` Jakub Jelinek
1 sibling, 0 replies; 12+ messages in thread
From: Jonathan Wakely @ 2023-06-09 9:06 UTC (permalink / raw)
To: Richard Biener; +Cc: Jason Merrill, gcc-patches, libstdc++
[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]
On Fri, 9 Jun 2023 at 10:03, Richard Biener <richard.guenther@gmail.com>
wrote:
> On Thu, Jun 8, 2023 at 3:14 PM Jonathan Wakely via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Fri, 26 May 2023 at 10:58, Jonathan Wakely wrote:
> >
> > >
> > >
> > > On Wed, 24 May 2023 at 19:56, Jason Merrill via Libstdc++ <
> > > libstdc++@gcc.gnu.org> wrote:
> > >
> > >> Middle-end folks: any thoughts about how best to make the change
> > >> described in
> > >> the last paragraph below?
> > >>
> > >> Library folks: any thoughts on the changes to __cxa_call_terminate?
> > >>
> > >
> > > I see no harm in exporting it (with the adjusted signature). The "looks
> > > standard but isn't" name is a little unfortunate, but not a big deal.
> > >
> >
> > Jason, do you have any objection to exporting __cxa_call_terminate for
> GCC
> > 13.2 as well, even though the FE won't use it?
> >
> > Currently both gcc-13 and trunk are at the same library version,
> > libstdc++.so.6.0.32
> >
> > But with this addition to trunk we need to bump that .32 to .33, meaning
> > that gcc-13 and trunk diverge. If we want to backport any new symbols
> from
> > trunk to gcc-13 that gets trickier once they've diverged.
>
> But if you backport any new used symbol you have to bump the version
> anyway. So why not bump now (on trunk)?
>
We've already bumped it once since 13.1, and until 13.2 is released we
aren't committed to freezing the new version. I think we can add this
__cxa_call_terminate symbol to the version currently used by 13.1.1 without
problems. And if we want to backport another new symbol before 13.2, we can
do that too (unless it would be too difficult for distros already shipping
13.1.1, but I don't think that applies in this case).
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-09 9:02 ` Richard Biener
2023-06-09 9:06 ` Jonathan Wakely
@ 2023-06-09 9:09 ` Jakub Jelinek
2023-06-09 12:15 ` Jonathan Wakely
1 sibling, 1 reply; 12+ messages in thread
From: Jakub Jelinek @ 2023-06-09 9:09 UTC (permalink / raw)
To: Richard Biener; +Cc: Jonathan Wakely, Jason Merrill, gcc-patches, libstdc++
On Fri, Jun 09, 2023 at 11:02:48AM +0200, Richard Biener via Gcc-patches wrote:
> > Currently both gcc-13 and trunk are at the same library version,
> > libstdc++.so.6.0.32
> >
> > But with this addition to trunk we need to bump that .32 to .33, meaning
> > that gcc-13 and trunk diverge. If we want to backport any new symbols from
> > trunk to gcc-13 that gets trickier once they've diverged.
>
> But if you backport any new used symbol you have to bump the version
> anyway. So why not bump now (on trunk)?
We've already done that in 13.1.1. So, before 13.2 is released, we can add
further symbols to the GLIBCXX_3.4.32 symbol version.
Though, I don't see a problem bumping libstdc++ to libstdc++.so.6.0.33
on the trunk now and put __cxa_call_terminate to GLIBCXX_3.4.33.
The ABI on the trunk is certainly not stable at this point.
If we come up with a need to introduce another symbol to 13.2, we can just
add it to GLIBCXX_3.4.32 on the trunk and then backport that change to the
branch. If nothing in 13 will use the new symbol, seems like a waste to add
it to libstdc++.so.6.0.32.
> > If we added __cxa_call_terminate to gcc-13, making it another new addition
> > to libstdc++.so.6.0.32, then it would simplify a few things.
> >
> > In theory it could be a problem for distros already shipping gcc-13.1.1
> > with that new libstdc++.so.6.0.32 version, but since the
> > __cxa_call_terminate symbol won't actually be used by the gcc-13.1.1
> > compilers, I don't think it will be a problem.
Jakub
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-09 9:09 ` Jakub Jelinek
@ 2023-06-09 12:15 ` Jonathan Wakely
0 siblings, 0 replies; 12+ messages in thread
From: Jonathan Wakely @ 2023-06-09 12:15 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Richard Biener, Jason Merrill, gcc-patches, libstdc++
[-- Attachment #1: Type: text/plain, Size: 1023 bytes --]
On Fri, 9 Jun 2023 at 10:09, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jun 09, 2023 at 11:02:48AM +0200, Richard Biener via Gcc-patches
> wrote:
> > > Currently both gcc-13 and trunk are at the same library version,
> > > libstdc++.so.6.0.32
> > >
> > > But with this addition to trunk we need to bump that .32 to .33,
> meaning
> > > that gcc-13 and trunk diverge. If we want to backport any new symbols
> from
> > > trunk to gcc-13 that gets trickier once they've diverged.
> >
> > But if you backport any new used symbol you have to bump the version
> > anyway. So why not bump now (on trunk)?
>
> We've already done that in 13.1.1. So, before 13.2 is released, we can add
> further symbols to the GLIBCXX_3.4.32 symbol version.
> Though, I don't see a problem bumping libstdc++ to libstdc++.so.6.0.33
> on the trunk now
OK, done at r14-1649-g9a3558cf1fb40b
> and put __cxa_call_terminate to GLIBCXX_3.4.33.
>
Well it's already in CXXABI_1.3.15, we just didn't bump the library version
when adding that.
^ permalink raw reply [flat|nested] 12+ messages in thread
* PING Re: [PATCH RFA (tree-eh)] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-05-24 18:55 [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720] Jason Merrill
2023-05-26 9:58 ` Jonathan Wakely
@ 2023-06-02 16:56 ` Jason Merrill
2023-06-05 6:09 ` Richard Biener
2023-06-03 14:33 ` [PATCH RFC] " Jeff Law
2 siblings, 1 reply; 12+ messages in thread
From: Jason Merrill @ 2023-06-02 16:56 UTC (permalink / raw)
To: gcc-patches; +Cc: libstdc++
Since Jonathan approved the library change, I'm looking for middle-end
approval for the tree-eh change, even without advice on the potential
follow-up.
On 5/24/23 14:55, Jason Merrill wrote:
> Middle-end folks: any thoughts about how best to make the change described in
> the last paragraph below?
>
> Library folks: any thoughts on the changes to __cxa_call_terminate?
>
> -- 8< --
>
> [except.handle]/7 says that when we enter std::terminate due to a throw,
> that is considered an active handler. We already implemented that properly
> for the case of not finding a handler (__cxa_throw calls __cxa_begin_catch
> before std::terminate) and the case of finding a callsite with no landing
> pad (the personality function calls __cxa_call_terminate which calls
> __cxa_begin_catch), but for the case of a throw in a try/catch in a noexcept
> function, we were emitting a cleanup that calls std::terminate directly
> without ever calling __cxa_begin_catch to handle the exception.
>
> A straightforward way to fix this seems to be calling __cxa_call_terminate
> instead. However, that requires exporting it from libstdc++, which we have
> not previously done. Despite the name, it isn't actually part of the ABI
> standard. Nor is __cxa_call_unexpected, as far as I can tell, but that one
> is also used by clang. For this case they use __clang_call_terminate; it
> seems reasonable to me for us to stick with __cxa_call_terminate.
>
> I also change __cxa_call_terminate to take void* for simplicity in the front
> end (and consistency with __cxa_call_unexpected) but that isn't necessary if
> it's undesirable for some reason.
>
> This patch does not fix the issue that representing the noexcept as a
> cleanup is wrong, and confuses the handler search; since it looks like a
> cleanup in the EH tables, the unwinder keeps looking until it finds the
> catch in main(), which it should never have gotten to. Without the
> try/catch in main, the unwinder would reach the end of the stack and say no
> handler was found. The noexcept is a handler, and should be treated as one,
> as it is when the landing pad is omitted.
>
> The best fix for that issue seems to me to be to represent an
> ERT_MUST_NOT_THROW after an ERT_TRY in an action list as though it were an
> ERT_ALLOWED_EXCEPTIONS (since indeed it is an exception-specification). The
> actual code generation shouldn't need to change (apart from the change made
> by this patch), only the action table entry.
>
> PR c++/97720
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (enum cp_tree_index): Add CPTI_CALL_TERMINATE_FN.
> (call_terminate_fn): New macro.
> * cp-gimplify.cc (gimplify_must_not_throw_expr): Use it.
> * except.cc (init_exception_processing): Set it.
> (cp_protect_cleanup_actions): Return it.
>
> gcc/ChangeLog:
>
> * tree-eh.cc (lower_resx): Pass the exception pointer to the
> failure_decl.
> * except.h: Tweak comment.
>
> libstdc++-v3/ChangeLog:
>
> * libsupc++/eh_call.cc (__cxa_call_terminate): Take void*.
> * config/abi/pre/gnu.ver: Add it.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/eh/terminate2.C: New test.
> ---
> gcc/cp/cp-tree.h | 2 ++
> gcc/except.h | 2 +-
> gcc/cp/cp-gimplify.cc | 2 +-
> gcc/cp/except.cc | 5 ++++-
> gcc/testsuite/g++.dg/eh/terminate2.C | 30 ++++++++++++++++++++++++++++
> gcc/tree-eh.cc | 16 ++++++++++++++-
> libstdc++-v3/libsupc++/eh_call.cc | 4 +++-
> libstdc++-v3/config/abi/pre/gnu.ver | 7 +++++++
> 8 files changed, 63 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/eh/terminate2.C
>
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index a1b882f11fe..a8465a988b5 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -217,6 +217,7 @@ enum cp_tree_index
> definitions. */
> CPTI_ALIGN_TYPE,
> CPTI_TERMINATE_FN,
> + CPTI_CALL_TERMINATE_FN,
> CPTI_CALL_UNEXPECTED_FN,
>
> /* These are lazily inited. */
> @@ -358,6 +359,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
> /* Exception handling function declarations. */
> #define terminate_fn cp_global_trees[CPTI_TERMINATE_FN]
> #define call_unexpected_fn cp_global_trees[CPTI_CALL_UNEXPECTED_FN]
> +#define call_terminate_fn cp_global_trees[CPTI_CALL_TERMINATE_FN]
> #define get_exception_ptr_fn cp_global_trees[CPTI_GET_EXCEPTION_PTR_FN]
> #define begin_catch_fn cp_global_trees[CPTI_BEGIN_CATCH_FN]
> #define end_catch_fn cp_global_trees[CPTI_END_CATCH_FN]
> diff --git a/gcc/except.h b/gcc/except.h
> index 5ecdbc0d1dc..378a9e4cb77 100644
> --- a/gcc/except.h
> +++ b/gcc/except.h
> @@ -155,7 +155,7 @@ struct GTY(()) eh_region_d
> struct eh_region_u_must_not_throw {
> /* A function decl to be invoked if this region is actually reachable
> from within the function, rather than implementable from the runtime.
> - The normal way for this to happen is for there to be a CLEANUP region
> + The normal way for this to happen is for there to be a TRY region
> contained within this MUST_NOT_THROW region. Note that if the
> runtime handles the MUST_NOT_THROW region, we have no control over
> what termination function is called; it will be decided by the
> diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> index 216a6231d6f..853b1e44236 100644
> --- a/gcc/cp/cp-gimplify.cc
> +++ b/gcc/cp/cp-gimplify.cc
> @@ -343,7 +343,7 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq *pre_p)
> gimple *mnt;
>
> gimplify_and_add (body, &try_);
> - mnt = gimple_build_eh_must_not_throw (terminate_fn);
> + mnt = gimple_build_eh_must_not_throw (call_terminate_fn);
> gimple_seq_add_stmt_without_update (&catch_, mnt);
> mnt = gimple_build_try (try_, catch_, GIMPLE_TRY_CATCH);
>
> diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> index 91a5e049860..b04eb00d220 100644
> --- a/gcc/cp/except.cc
> +++ b/gcc/cp/except.cc
> @@ -64,6 +64,9 @@ init_exception_processing (void)
> tmp = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
> call_unexpected_fn
> = push_throw_library_fn (get_identifier ("__cxa_call_unexpected"), tmp);
> + call_terminate_fn
> + = push_library_fn (get_identifier ("__cxa_call_terminate"), tmp, NULL_TREE,
> + ECF_NORETURN | ECF_COLD | ECF_NOTHROW);
> }
>
> /* Returns an expression to be executed if an unhandled exception is
> @@ -76,7 +79,7 @@ cp_protect_cleanup_actions (void)
>
> When the destruction of an object during stack unwinding exits
> using an exception ... void terminate(); is called. */
> - return terminate_fn;
> + return call_terminate_fn;
> }
>
> static tree
> diff --git a/gcc/testsuite/g++.dg/eh/terminate2.C b/gcc/testsuite/g++.dg/eh/terminate2.C
> new file mode 100644
> index 00000000000..1c69dab95f8
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/eh/terminate2.C
> @@ -0,0 +1,30 @@
> +// PR c++/97720
> +// { dg-do run }
> +
> +// Test that there is an active exception when we reach the terminate handler.
> +
> +#include <exception>
> +#include <cstdlib>
> +
> +void bad_guy() throw() {
> + try { throw 0; }
> + catch (float) { }
> + // Don't catch int.
> +}
> +
> +void level1() {
> + bad_guy();
> + throw "dead code";
> +}
> +
> +void my_term()
> +{
> + try { throw; }
> + catch(...) { std::exit(0); }
> +}
> +
> +int main() {
> + std::set_terminate (my_term);
> + try { level1(); }
> + catch (int) { }
> +}
> diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> index 934209d205f..e8ceff36cc6 100644
> --- a/gcc/tree-eh.cc
> +++ b/gcc/tree-eh.cc
> @@ -3382,8 +3382,22 @@ lower_resx (basic_block bb, gresx *stmt,
> lab = gimple_block_label (new_bb);
> gsi2 = gsi_start_bb (new_bb);
>
> + /* Handle failure fns that expect either no arguments or the
> + exception pointer. */
> fn = dst_r->u.must_not_throw.failure_decl;
> - x = gimple_build_call (fn, 0);
> + if (TYPE_ARG_TYPES (TREE_TYPE (fn)) != void_list_node)
> + {
> + tree epfn = builtin_decl_implicit (BUILT_IN_EH_POINTER);
> + src_nr = build_int_cst (integer_type_node, src_r->index);
> + x = gimple_build_call (epfn, 1, src_nr);
> + tree var = create_tmp_var (ptr_type_node);
> + var = make_ssa_name (var, x);
> + gimple_call_set_lhs (x, var);
> + gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
> + x = gimple_build_call (fn, 1, var);
> + }
> + else
> + x = gimple_build_call (fn, 0);
> gimple_set_location (x, dst_r->u.must_not_throw.failure_loc);
> gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
>
> diff --git a/libstdc++-v3/libsupc++/eh_call.cc b/libstdc++-v3/libsupc++/eh_call.cc
> index 3cfc71a4ef1..2bec4e83a7d 100644
> --- a/libstdc++-v3/libsupc++/eh_call.cc
> +++ b/libstdc++-v3/libsupc++/eh_call.cc
> @@ -36,8 +36,10 @@ using namespace __cxxabiv1;
> // terminate.
>
> extern "C" void
> -__cxa_call_terminate(_Unwind_Exception* ue_header) throw ()
> +__cxa_call_terminate(void* ue_header_in) throw ()
> {
> + _Unwind_Exception* ue_header
> + = reinterpret_cast<_Unwind_Exception*>(ue_header_in);
>
> if (ue_header)
> {
> diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
> index 768cd4a4a6c..a2e5f3b4e74 100644
> --- a/libstdc++-v3/config/abi/pre/gnu.ver
> +++ b/libstdc++-v3/config/abi/pre/gnu.ver
> @@ -2841,6 +2841,13 @@ CXXABI_1.3.14 {
>
> } CXXABI_1.3.13;
>
> +CXXABI_1.3.15 {
> +
> + global:
> + __cxa_call_terminate;
> +
> +} CXXABI_1.3.14;
> +
> # Symbols in the support library (libsupc++) supporting transactional memory.
> CXXABI_TM_1 {
>
>
> base-commit: 2738955004256c2e9753364d78a7be340323b74b
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: PING Re: [PATCH RFA (tree-eh)] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-02 16:56 ` PING Re: [PATCH RFA (tree-eh)] " Jason Merrill
@ 2023-06-05 6:09 ` Richard Biener
2023-06-05 19:10 ` Jason Merrill
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2023-06-05 6:09 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches, libstdc++
On Fri, Jun 2, 2023 at 6:57 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Since Jonathan approved the library change, I'm looking for middle-end
> approval for the tree-eh change, even without advice on the potential
> follow-up.
>
> On 5/24/23 14:55, Jason Merrill wrote:
> > Middle-end folks: any thoughts about how best to make the change described in
> > the last paragraph below?
> >
> > Library folks: any thoughts on the changes to __cxa_call_terminate?
> >
> > -- 8< --
> >
> > [except.handle]/7 says that when we enter std::terminate due to a throw,
> > that is considered an active handler. We already implemented that properly
> > for the case of not finding a handler (__cxa_throw calls __cxa_begin_catch
> > before std::terminate) and the case of finding a callsite with no landing
> > pad (the personality function calls __cxa_call_terminate which calls
> > __cxa_begin_catch), but for the case of a throw in a try/catch in a noexcept
> > function, we were emitting a cleanup that calls std::terminate directly
> > without ever calling __cxa_begin_catch to handle the exception.
> >
> > A straightforward way to fix this seems to be calling __cxa_call_terminate
> > instead. However, that requires exporting it from libstdc++, which we have
> > not previously done. Despite the name, it isn't actually part of the ABI
> > standard. Nor is __cxa_call_unexpected, as far as I can tell, but that one
> > is also used by clang. For this case they use __clang_call_terminate; it
> > seems reasonable to me for us to stick with __cxa_call_terminate.
> >
> > I also change __cxa_call_terminate to take void* for simplicity in the front
> > end (and consistency with __cxa_call_unexpected) but that isn't necessary if
> > it's undesirable for some reason.
> >
> > This patch does not fix the issue that representing the noexcept as a
> > cleanup is wrong, and confuses the handler search; since it looks like a
> > cleanup in the EH tables, the unwinder keeps looking until it finds the
> > catch in main(), which it should never have gotten to. Without the
> > try/catch in main, the unwinder would reach the end of the stack and say no
> > handler was found. The noexcept is a handler, and should be treated as one,
> > as it is when the landing pad is omitted.
> >
> > The best fix for that issue seems to me to be to represent an
> > ERT_MUST_NOT_THROW after an ERT_TRY in an action list as though it were an
> > ERT_ALLOWED_EXCEPTIONS (since indeed it is an exception-specification). The
> > actual code generation shouldn't need to change (apart from the change made
> > by this patch), only the action table entry.
> >
> > PR c++/97720
> >
> > gcc/cp/ChangeLog:
> >
> > * cp-tree.h (enum cp_tree_index): Add CPTI_CALL_TERMINATE_FN.
> > (call_terminate_fn): New macro.
> > * cp-gimplify.cc (gimplify_must_not_throw_expr): Use it.
> > * except.cc (init_exception_processing): Set it.
> > (cp_protect_cleanup_actions): Return it.
> >
> > gcc/ChangeLog:
> >
> > * tree-eh.cc (lower_resx): Pass the exception pointer to the
> > failure_decl.
> > * except.h: Tweak comment.
> >
> > libstdc++-v3/ChangeLog:
> >
> > * libsupc++/eh_call.cc (__cxa_call_terminate): Take void*.
> > * config/abi/pre/gnu.ver: Add it.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/eh/terminate2.C: New test.
> > ---
> > gcc/cp/cp-tree.h | 2 ++
> > gcc/except.h | 2 +-
> > gcc/cp/cp-gimplify.cc | 2 +-
> > gcc/cp/except.cc | 5 ++++-
> > gcc/testsuite/g++.dg/eh/terminate2.C | 30 ++++++++++++++++++++++++++++
> > gcc/tree-eh.cc | 16 ++++++++++++++-
> > libstdc++-v3/libsupc++/eh_call.cc | 4 +++-
> > libstdc++-v3/config/abi/pre/gnu.ver | 7 +++++++
> > 8 files changed, 63 insertions(+), 5 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/eh/terminate2.C
> >
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index a1b882f11fe..a8465a988b5 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -217,6 +217,7 @@ enum cp_tree_index
> > definitions. */
> > CPTI_ALIGN_TYPE,
> > CPTI_TERMINATE_FN,
> > + CPTI_CALL_TERMINATE_FN,
> > CPTI_CALL_UNEXPECTED_FN,
> >
> > /* These are lazily inited. */
> > @@ -358,6 +359,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
> > /* Exception handling function declarations. */
> > #define terminate_fn cp_global_trees[CPTI_TERMINATE_FN]
> > #define call_unexpected_fn cp_global_trees[CPTI_CALL_UNEXPECTED_FN]
> > +#define call_terminate_fn cp_global_trees[CPTI_CALL_TERMINATE_FN]
> > #define get_exception_ptr_fn cp_global_trees[CPTI_GET_EXCEPTION_PTR_FN]
> > #define begin_catch_fn cp_global_trees[CPTI_BEGIN_CATCH_FN]
> > #define end_catch_fn cp_global_trees[CPTI_END_CATCH_FN]
> > diff --git a/gcc/except.h b/gcc/except.h
> > index 5ecdbc0d1dc..378a9e4cb77 100644
> > --- a/gcc/except.h
> > +++ b/gcc/except.h
> > @@ -155,7 +155,7 @@ struct GTY(()) eh_region_d
> > struct eh_region_u_must_not_throw {
> > /* A function decl to be invoked if this region is actually reachable
> > from within the function, rather than implementable from the runtime.
> > - The normal way for this to happen is for there to be a CLEANUP region
> > + The normal way for this to happen is for there to be a TRY region
I only wondered about this, whether it shouldn't say CLEANUP or TRY instead
of just TRY? Do you know of other frontends making use of MUST_NOT_THROW?
Otherwise looks OK.
Thanks,
Richard.
> > contained within this MUST_NOT_THROW region. Note that if the
> > runtime handles the MUST_NOT_THROW region, we have no control over
> > what termination function is called; it will be decided by the
> > diff --git a/gcc/cp/cp-gimplify.cc b/gcc/cp/cp-gimplify.cc
> > index 216a6231d6f..853b1e44236 100644
> > --- a/gcc/cp/cp-gimplify.cc
> > +++ b/gcc/cp/cp-gimplify.cc
> > @@ -343,7 +343,7 @@ gimplify_must_not_throw_expr (tree *expr_p, gimple_seq *pre_p)
> > gimple *mnt;
> >
> > gimplify_and_add (body, &try_);
> > - mnt = gimple_build_eh_must_not_throw (terminate_fn);
> > + mnt = gimple_build_eh_must_not_throw (call_terminate_fn);
> > gimple_seq_add_stmt_without_update (&catch_, mnt);
> > mnt = gimple_build_try (try_, catch_, GIMPLE_TRY_CATCH);
> >
> > diff --git a/gcc/cp/except.cc b/gcc/cp/except.cc
> > index 91a5e049860..b04eb00d220 100644
> > --- a/gcc/cp/except.cc
> > +++ b/gcc/cp/except.cc
> > @@ -64,6 +64,9 @@ init_exception_processing (void)
> > tmp = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE);
> > call_unexpected_fn
> > = push_throw_library_fn (get_identifier ("__cxa_call_unexpected"), tmp);
> > + call_terminate_fn
> > + = push_library_fn (get_identifier ("__cxa_call_terminate"), tmp, NULL_TREE,
> > + ECF_NORETURN | ECF_COLD | ECF_NOTHROW);
> > }
> >
> > /* Returns an expression to be executed if an unhandled exception is
> > @@ -76,7 +79,7 @@ cp_protect_cleanup_actions (void)
> >
> > When the destruction of an object during stack unwinding exits
> > using an exception ... void terminate(); is called. */
> > - return terminate_fn;
> > + return call_terminate_fn;
> > }
> >
> > static tree
> > diff --git a/gcc/testsuite/g++.dg/eh/terminate2.C b/gcc/testsuite/g++.dg/eh/terminate2.C
> > new file mode 100644
> > index 00000000000..1c69dab95f8
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/eh/terminate2.C
> > @@ -0,0 +1,30 @@
> > +// PR c++/97720
> > +// { dg-do run }
> > +
> > +// Test that there is an active exception when we reach the terminate handler.
> > +
> > +#include <exception>
> > +#include <cstdlib>
> > +
> > +void bad_guy() throw() {
> > + try { throw 0; }
> > + catch (float) { }
> > + // Don't catch int.
> > +}
> > +
> > +void level1() {
> > + bad_guy();
> > + throw "dead code";
> > +}
> > +
> > +void my_term()
> > +{
> > + try { throw; }
> > + catch(...) { std::exit(0); }
> > +}
> > +
> > +int main() {
> > + std::set_terminate (my_term);
> > + try { level1(); }
> > + catch (int) { }
> > +}
> > diff --git a/gcc/tree-eh.cc b/gcc/tree-eh.cc
> > index 934209d205f..e8ceff36cc6 100644
> > --- a/gcc/tree-eh.cc
> > +++ b/gcc/tree-eh.cc
> > @@ -3382,8 +3382,22 @@ lower_resx (basic_block bb, gresx *stmt,
> > lab = gimple_block_label (new_bb);
> > gsi2 = gsi_start_bb (new_bb);
> >
> > + /* Handle failure fns that expect either no arguments or the
> > + exception pointer. */
> > fn = dst_r->u.must_not_throw.failure_decl;
> > - x = gimple_build_call (fn, 0);
> > + if (TYPE_ARG_TYPES (TREE_TYPE (fn)) != void_list_node)
> > + {
> > + tree epfn = builtin_decl_implicit (BUILT_IN_EH_POINTER);
> > + src_nr = build_int_cst (integer_type_node, src_r->index);
> > + x = gimple_build_call (epfn, 1, src_nr);
> > + tree var = create_tmp_var (ptr_type_node);
> > + var = make_ssa_name (var, x);
> > + gimple_call_set_lhs (x, var);
> > + gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
> > + x = gimple_build_call (fn, 1, var);
> > + }
> > + else
> > + x = gimple_build_call (fn, 0);
> > gimple_set_location (x, dst_r->u.must_not_throw.failure_loc);
> > gsi_insert_after (&gsi2, x, GSI_CONTINUE_LINKING);
> >
> > diff --git a/libstdc++-v3/libsupc++/eh_call.cc b/libstdc++-v3/libsupc++/eh_call.cc
> > index 3cfc71a4ef1..2bec4e83a7d 100644
> > --- a/libstdc++-v3/libsupc++/eh_call.cc
> > +++ b/libstdc++-v3/libsupc++/eh_call.cc
> > @@ -36,8 +36,10 @@ using namespace __cxxabiv1;
> > // terminate.
> >
> > extern "C" void
> > -__cxa_call_terminate(_Unwind_Exception* ue_header) throw ()
> > +__cxa_call_terminate(void* ue_header_in) throw ()
> > {
> > + _Unwind_Exception* ue_header
> > + = reinterpret_cast<_Unwind_Exception*>(ue_header_in);
> >
> > if (ue_header)
> > {
> > diff --git a/libstdc++-v3/config/abi/pre/gnu.ver b/libstdc++-v3/config/abi/pre/gnu.ver
> > index 768cd4a4a6c..a2e5f3b4e74 100644
> > --- a/libstdc++-v3/config/abi/pre/gnu.ver
> > +++ b/libstdc++-v3/config/abi/pre/gnu.ver
> > @@ -2841,6 +2841,13 @@ CXXABI_1.3.14 {
> >
> > } CXXABI_1.3.13;
> >
> > +CXXABI_1.3.15 {
> > +
> > + global:
> > + __cxa_call_terminate;
> > +
> > +} CXXABI_1.3.14;
> > +
> > # Symbols in the support library (libsupc++) supporting transactional memory.
> > CXXABI_TM_1 {
> >
> >
> > base-commit: 2738955004256c2e9753364d78a7be340323b74b
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: PING Re: [PATCH RFA (tree-eh)] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-06-05 6:09 ` Richard Biener
@ 2023-06-05 19:10 ` Jason Merrill
0 siblings, 0 replies; 12+ messages in thread
From: Jason Merrill @ 2023-06-05 19:10 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches, libstdc++
On 6/5/23 02:09, Richard Biener wrote:
> On Fri, Jun 2, 2023 at 6:57 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Since Jonathan approved the library change, I'm looking for middle-end
>> approval for the tree-eh change, even without advice on the potential
>> follow-up.
>>
>> On 5/24/23 14:55, Jason Merrill wrote:
>>> Middle-end folks: any thoughts about how best to make the change described in
>>> the last paragraph below?
>>>
>>> Library folks: any thoughts on the changes to __cxa_call_terminate?
>>>
>>> -- 8< --
>>>
>>> [except.handle]/7 says that when we enter std::terminate due to a throw,
>>> that is considered an active handler. We already implemented that properly
>>> for the case of not finding a handler (__cxa_throw calls __cxa_begin_catch
>>> before std::terminate) and the case of finding a callsite with no landing
>>> pad (the personality function calls __cxa_call_terminate which calls
>>> __cxa_begin_catch), but for the case of a throw in a try/catch in a noexcept
>>> function, we were emitting a cleanup that calls std::terminate directly
>>> without ever calling __cxa_begin_catch to handle the exception.
>>>
>>> A straightforward way to fix this seems to be calling __cxa_call_terminate
>>> instead. However, that requires exporting it from libstdc++, which we have
>>> not previously done. Despite the name, it isn't actually part of the ABI
>>> standard. Nor is __cxa_call_unexpected, as far as I can tell, but that one
>>> is also used by clang. For this case they use __clang_call_terminate; it
>>> seems reasonable to me for us to stick with __cxa_call_terminate.
>>>
>>> I also change __cxa_call_terminate to take void* for simplicity in the front
>>> end (and consistency with __cxa_call_unexpected) but that isn't necessary if
>>> it's undesirable for some reason.
>>>
>>> This patch does not fix the issue that representing the noexcept as a
>>> cleanup is wrong, and confuses the handler search; since it looks like a
>>> cleanup in the EH tables, the unwinder keeps looking until it finds the
>>> catch in main(), which it should never have gotten to. Without the
>>> try/catch in main, the unwinder would reach the end of the stack and say no
>>> handler was found. The noexcept is a handler, and should be treated as one,
>>> as it is when the landing pad is omitted.
>>>
>>> The best fix for that issue seems to me to be to represent an
>>> ERT_MUST_NOT_THROW after an ERT_TRY in an action list as though it were an
>>> ERT_ALLOWED_EXCEPTIONS (since indeed it is an exception-specification). The
>>> actual code generation shouldn't need to change (apart from the change made
>>> by this patch), only the action table entry.
>>>
>>> PR c++/97720
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> * cp-tree.h (enum cp_tree_index): Add CPTI_CALL_TERMINATE_FN.
>>> (call_terminate_fn): New macro.
>>> * cp-gimplify.cc (gimplify_must_not_throw_expr): Use it.
>>> * except.cc (init_exception_processing): Set it.
>>> (cp_protect_cleanup_actions): Return it.
>>>
>>> gcc/ChangeLog:
>>>
>>> * tree-eh.cc (lower_resx): Pass the exception pointer to the
>>> failure_decl.
>>> * except.h: Tweak comment.
>>>
>>> libstdc++-v3/ChangeLog:
>>>
>>> * libsupc++/eh_call.cc (__cxa_call_terminate): Take void*.
>>> * config/abi/pre/gnu.ver: Add it.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> * g++.dg/eh/terminate2.C: New test.
>>> ---
>>> gcc/cp/cp-tree.h | 2 ++
>>> gcc/except.h | 2 +-
>>> gcc/cp/cp-gimplify.cc | 2 +-
>>> gcc/cp/except.cc | 5 ++++-
>>> gcc/testsuite/g++.dg/eh/terminate2.C | 30 ++++++++++++++++++++++++++++
>>> gcc/tree-eh.cc | 16 ++++++++++++++-
>>> libstdc++-v3/libsupc++/eh_call.cc | 4 +++-
>>> libstdc++-v3/config/abi/pre/gnu.ver | 7 +++++++
>>> 8 files changed, 63 insertions(+), 5 deletions(-)
>>> create mode 100644 gcc/testsuite/g++.dg/eh/terminate2.C
>>>
>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>>> index a1b882f11fe..a8465a988b5 100644
>>> --- a/gcc/cp/cp-tree.h
>>> +++ b/gcc/cp/cp-tree.h
>>> @@ -217,6 +217,7 @@ enum cp_tree_index
>>> definitions. */
>>> CPTI_ALIGN_TYPE,
>>> CPTI_TERMINATE_FN,
>>> + CPTI_CALL_TERMINATE_FN,
>>> CPTI_CALL_UNEXPECTED_FN,
>>>
>>> /* These are lazily inited. */
>>> @@ -358,6 +359,7 @@ extern GTY(()) tree cp_global_trees[CPTI_MAX];
>>> /* Exception handling function declarations. */
>>> #define terminate_fn cp_global_trees[CPTI_TERMINATE_FN]
>>> #define call_unexpected_fn cp_global_trees[CPTI_CALL_UNEXPECTED_FN]
>>> +#define call_terminate_fn cp_global_trees[CPTI_CALL_TERMINATE_FN]
>>> #define get_exception_ptr_fn cp_global_trees[CPTI_GET_EXCEPTION_PTR_FN]
>>> #define begin_catch_fn cp_global_trees[CPTI_BEGIN_CATCH_FN]
>>> #define end_catch_fn cp_global_trees[CPTI_END_CATCH_FN]
>>> diff --git a/gcc/except.h b/gcc/except.h
>>> index 5ecdbc0d1dc..378a9e4cb77 100644
>>> --- a/gcc/except.h
>>> +++ b/gcc/except.h
>>> @@ -155,7 +155,7 @@ struct GTY(()) eh_region_d
>>> struct eh_region_u_must_not_throw {
>>> /* A function decl to be invoked if this region is actually reachable
>>> from within the function, rather than implementable from the runtime.
>>> - The normal way for this to happen is for there to be a CLEANUP region
>>> + The normal way for this to happen is for there to be a TRY region
>
> I only wondered about this, whether it shouldn't say CLEANUP or TRY instead
> of just TRY? Do you know of other frontends making use of MUST_NOT_THROW?
If there are only CLEANUPs within the MUST_NOT_THROW, we optimize them
away and omit the landing pad, so the region is not actually reachable.
Jason
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720]
2023-05-24 18:55 [PATCH RFC] c++: use __cxa_call_terminate for MUST_NOT_THROW [PR97720] Jason Merrill
2023-05-26 9:58 ` Jonathan Wakely
2023-06-02 16:56 ` PING Re: [PATCH RFA (tree-eh)] " Jason Merrill
@ 2023-06-03 14:33 ` Jeff Law
2 siblings, 0 replies; 12+ messages in thread
From: Jeff Law @ 2023-06-03 14:33 UTC (permalink / raw)
To: Jason Merrill, gcc-patches; +Cc: libstdc++
On 5/24/23 12:55, Jason Merrill via Gcc-patches wrote:
> Middle-end folks: any thoughts about how best to make the change described in
> the last paragraph below?
>
> Library folks: any thoughts on the changes to __cxa_call_terminate?
>
> -- 8< --
>
> [except.handle]/7 says that when we enter std::terminate due to a throw,
> that is considered an active handler. We already implemented that properly
> for the case of not finding a handler (__cxa_throw calls __cxa_begin_catch
> before std::terminate) and the case of finding a callsite with no landing
> pad (the personality function calls __cxa_call_terminate which calls
> __cxa_begin_catch), but for the case of a throw in a try/catch in a noexcept
> function, we were emitting a cleanup that calls std::terminate directly
> without ever calling __cxa_begin_catch to handle the exception.
>
> A straightforward way to fix this seems to be calling __cxa_call_terminate
> instead. However, that requires exporting it from libstdc++, which we have
> not previously done. Despite the name, it isn't actually part of the ABI
> standard. Nor is __cxa_call_unexpected, as far as I can tell, but that one
> is also used by clang. For this case they use __clang_call_terminate; it
> seems reasonable to me for us to stick with __cxa_call_terminate.
>
> I also change __cxa_call_terminate to take void* for simplicity in the front
> end (and consistency with __cxa_call_unexpected) but that isn't necessary if
> it's undesirable for some reason.
>
> This patch does not fix the issue that representing the noexcept as a
> cleanup is wrong, and confuses the handler search; since it looks like a
> cleanup in the EH tables, the unwinder keeps looking until it finds the
> catch in main(), which it should never have gotten to. Without the
> try/catch in main, the unwinder would reach the end of the stack and say no
> handler was found. The noexcept is a handler, and should be treated as one,
> as it is when the landing pad is omitted.
>
> The best fix for that issue seems to me to be to represent an
> ERT_MUST_NOT_THROW after an ERT_TRY in an action list as though it were an
> ERT_ALLOWED_EXCEPTIONS (since indeed it is an exception-specification). The
> actual code generation shouldn't need to change (apart from the change made
> by this patch), only the action table entry.
>
> PR c++/97720
>
> gcc/cp/ChangeLog:
>
> * cp-tree.h (enum cp_tree_index): Add CPTI_CALL_TERMINATE_FN.
> (call_terminate_fn): New macro.
> * cp-gimplify.cc (gimplify_must_not_throw_expr): Use it.
> * except.cc (init_exception_processing): Set it.
> (cp_protect_cleanup_actions): Return it.
>
> gcc/ChangeLog:
>
> * tree-eh.cc (lower_resx): Pass the exception pointer to the
> failure_decl.
> * except.h: Tweak comment.
>
> libstdc++-v3/ChangeLog:
>
> * libsupc++/eh_call.cc (__cxa_call_terminate): Take void*.
> * config/abi/pre/gnu.ver: Add it.
>
> gcc/testsuite/ChangeLog:
>
> * g++.dg/eh/terminate2.C: New test.
OK on the middle end bits. And I'd tend to agree MUST_NOT_THROW is just
a special case of an exception-specification, so if you can make your
proposal work it seems like a reasonable approach.
jeff
^ permalink raw reply [flat|nested] 12+ messages in thread