[2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347]
Commit Message
The testcase in the PR demonstrates how it is possible for one
__builtin_setjmp_receiver label to appear in
nonlocal_goto_handler_labels list twice (after the block with
__builtin_setjmp_setup referring to it was duplicated).
remove_node_from_insn_list did not account for this possibility and
removed only the first copy from the list. Add an assert verifying that
duplicates are not present.
To avoid adding a label to the list twice, move registration of the
label from __builtin_setjmp_setup handling to __builtin_setjmp_receiver.
gcc/ChangeLog:
PR rtl-optimization/101347
* builtins.cc (expand_builtin) [BUILT_IN_SETJMP_SETUP]: Move
population of nonlocal_goto_handler_labels from here ...
(expand_builtin) [BUILT_IN_SETJMP_RECEIVER]: ... to here.
* rtlanal.cc (remove_node_from_insn_list): Verify that a
duplicate is not present in the remainder of the list.
---
gcc/builtins.cc | 15 +++++++--------
gcc/rtlanal.cc | 1 +
2 files changed, 8 insertions(+), 8 deletions(-)
Comments
On Tue, Jul 19, 2022 at 5:17 PM Alexander Monakov via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The testcase in the PR demonstrates how it is possible for one
> __builtin_setjmp_receiver label to appear in
> nonlocal_goto_handler_labels list twice (after the block with
> __builtin_setjmp_setup referring to it was duplicated).
>
> remove_node_from_insn_list did not account for this possibility and
> removed only the first copy from the list. Add an assert verifying that
> duplicates are not present.
>
> To avoid adding a label to the list twice, move registration of the
> label from __builtin_setjmp_setup handling to __builtin_setjmp_receiver.
Eric is probably most familiar with this, but can you make sure to bootstrap
and test this on a SJLJ EH target? I'm not sure --enable-sjlj-exceptions is
well tested anywhere but on targets not supporting DWARF EH and the
configury is a bit odd suggesting the option is mostly ignored ...
Richard.
> gcc/ChangeLog:
>
> PR rtl-optimization/101347
> * builtins.cc (expand_builtin) [BUILT_IN_SETJMP_SETUP]: Move
> population of nonlocal_goto_handler_labels from here ...
> (expand_builtin) [BUILT_IN_SETJMP_RECEIVER]: ... to here.
> * rtlanal.cc (remove_node_from_insn_list): Verify that a
> duplicate is not present in the remainder of the list.
> ---
> gcc/builtins.cc | 15 +++++++--------
> gcc/rtlanal.cc | 1 +
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/builtins.cc b/gcc/builtins.cc
> index e6816d5c8..12a688dd8 100644
> --- a/gcc/builtins.cc
> +++ b/gcc/builtins.cc
> @@ -7467,15 +7467,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
> tree label = TREE_OPERAND (CALL_EXPR_ARG (exp, 1), 0);
> rtx_insn *label_r = label_rtx (label);
>
> - /* This is copied from the handling of non-local gotos. */
> expand_builtin_setjmp_setup (buf_addr, label_r);
> - nonlocal_goto_handler_labels
> - = gen_rtx_INSN_LIST (VOIDmode, label_r,
> - nonlocal_goto_handler_labels);
> - /* ??? Do not let expand_label treat us as such since we would
> - not want to be both on the list of non-local labels and on
> - the list of forced labels. */
> - FORCED_LABEL (label) = 0;
> return const0_rtx;
> }
> break;
> @@ -7488,6 +7480,13 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
> rtx_insn *label_r = label_rtx (label);
>
> expand_builtin_setjmp_receiver (label_r);
> + nonlocal_goto_handler_labels
> + = gen_rtx_INSN_LIST (VOIDmode, label_r,
> + nonlocal_goto_handler_labels);
> + /* ??? Do not let expand_label treat us as such since we would
> + not want to be both on the list of non-local labels and on
> + the list of forced labels. */
> + FORCED_LABEL (label) = 0;
> return const0_rtx;
> }
> break;
> diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
> index ec95ecd6c..56da7435a 100644
> --- a/gcc/rtlanal.cc
> +++ b/gcc/rtlanal.cc
> @@ -2899,6 +2899,7 @@ remove_node_from_insn_list (const rtx_insn *node, rtx_insn_list **listp)
> else
> *listp = temp->next ();
>
> + gcc_checking_assert (!in_insn_list_p (temp->next (), node));
> return;
> }
>
> --
> 2.35.1
>
> Eric is probably most familiar with this, but can you make sure to bootstrap
> and test this on a SJLJ EH target? I'm not sure --enable-sjlj-exceptions
> is well tested anywhere but on targets not supporting DWARF EH and the
> configury is a bit odd suggesting the option is mostly ignored ...
This is a specific circuitry for __builtln_setjmp so it is *not* exercised by
the SJLJ exception scheme. It used to be exercised by the GNAT bootstrap, but
that's no longer the case either.
I think that the fix is sensible, assuming that it passes the C testsuite.
On Wed, 20 Jul 2022, Eric Botcazou wrote:
> > Eric is probably most familiar with this, but can you make sure to bootstrap
> > and test this on a SJLJ EH target? I'm not sure --enable-sjlj-exceptions
> > is well tested anywhere but on targets not supporting DWARF EH and the
> > configury is a bit odd suggesting the option is mostly ignored ...
>
> This is a specific circuitry for __builtln_setjmp so it is *not* exercised by
> the SJLJ exception scheme. It used to be exercised by the GNAT bootstrap, but
> that's no longer the case either.
>
> I think that the fix is sensible, assuming that it passes the C testsuite.
Yes, it passes the usual regtest. Thanks, applying to trunk.
Alexander
@@ -7467,15 +7467,7 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
tree label = TREE_OPERAND (CALL_EXPR_ARG (exp, 1), 0);
rtx_insn *label_r = label_rtx (label);
- /* This is copied from the handling of non-local gotos. */
expand_builtin_setjmp_setup (buf_addr, label_r);
- nonlocal_goto_handler_labels
- = gen_rtx_INSN_LIST (VOIDmode, label_r,
- nonlocal_goto_handler_labels);
- /* ??? Do not let expand_label treat us as such since we would
- not want to be both on the list of non-local labels and on
- the list of forced labels. */
- FORCED_LABEL (label) = 0;
return const0_rtx;
}
break;
@@ -7488,6 +7480,13 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
rtx_insn *label_r = label_rtx (label);
expand_builtin_setjmp_receiver (label_r);
+ nonlocal_goto_handler_labels
+ = gen_rtx_INSN_LIST (VOIDmode, label_r,
+ nonlocal_goto_handler_labels);
+ /* ??? Do not let expand_label treat us as such since we would
+ not want to be both on the list of non-local labels and on
+ the list of forced labels. */
+ FORCED_LABEL (label) = 0;
return const0_rtx;
}
break;
@@ -2899,6 +2899,7 @@ remove_node_from_insn_list (const rtx_insn *node, rtx_insn_list **listp)
else
*listp = temp->next ();
+ gcc_checking_assert (!in_insn_list_p (temp->next (), node));
return;
}