public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 1/2] Remove unused remove_node_from_expr_list
@ 2022-07-19 15:15 Alexander Monakov
  2022-07-19 15:15 ` [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347] Alexander Monakov
  2022-07-20  7:34 ` [PATCH 1/2] Remove unused remove_node_from_expr_list Richard Biener
  0 siblings, 2 replies; 6+ messages in thread
From: Alexander Monakov @ 2022-07-19 15:15 UTC (permalink / raw)
  To: gcc-patches; +Cc: Alexander Monakov

This function remains unused since remove_node_from_insn_list was cloned
from it.

gcc/ChangeLog:

	* rtl.h (remove_node_from_expr_list): Remove declaration.
	* rtlanal.cc (remove_node_from_expr_list): Remove (no uses).
---
 gcc/rtl.h      |  1 -
 gcc/rtlanal.cc | 29 -----------------------------
 2 files changed, 30 deletions(-)

diff --git a/gcc/rtl.h b/gcc/rtl.h
index 488016bb4..645c009a3 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -3712,7 +3712,6 @@ extern unsigned hash_rtx_cb (const_rtx, machine_mode, int *, int *,
 extern rtx regno_use_in (unsigned int, rtx);
 extern int auto_inc_p (const_rtx);
 extern bool in_insn_list_p (const rtx_insn_list *, const rtx_insn *);
-extern void remove_node_from_expr_list (const_rtx, rtx_expr_list **);
 extern void remove_node_from_insn_list (const rtx_insn *, rtx_insn_list **);
 extern int loc_mentioned_in_p (rtx *, const_rtx);
 extern rtx_insn *find_first_parameter_load (rtx_insn *, rtx_insn *);
diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
index d78cc6024..ec95ecd6c 100644
--- a/gcc/rtlanal.cc
+++ b/gcc/rtlanal.cc
@@ -2878,35 +2878,6 @@ in_insn_list_p (const rtx_insn_list *listp, const rtx_insn *node)
   return false;
 }
 
-/* Search LISTP (an EXPR_LIST) for an entry whose first operand is NODE and
-   remove that entry from the list if it is found.
-
-   A simple equality test is used to determine if NODE matches.  */
-
-void
-remove_node_from_expr_list (const_rtx node, rtx_expr_list **listp)
-{
-  rtx_expr_list *temp = *listp;
-  rtx_expr_list *prev = NULL;
-
-  while (temp)
-    {
-      if (node == temp->element ())
-	{
-	  /* Splice the node out of the list.  */
-	  if (prev)
-	    XEXP (prev, 1) = temp->next ();
-	  else
-	    *listp = temp->next ();
-
-	  return;
-	}
-
-      prev = temp;
-      temp = temp->next ();
-    }
-}
-
 /* Search LISTP (an INSN_LIST) for an entry whose first operand is NODE and
    remove that entry from the list if it is found.
 
-- 
2.35.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347]
  2022-07-19 15:15 [PATCH 1/2] Remove unused remove_node_from_expr_list Alexander Monakov
@ 2022-07-19 15:15 ` Alexander Monakov
  2022-07-20  7:39   ` Richard Biener
  2022-07-20  7:34 ` [PATCH 1/2] Remove unused remove_node_from_expr_list Richard Biener
  1 sibling, 1 reply; 6+ messages in thread
From: Alexander Monakov @ 2022-07-19 15:15 UTC (permalink / raw)
  To: gcc-patches; +Cc: Alexander Monakov

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(-)

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] Remove unused remove_node_from_expr_list
  2022-07-19 15:15 [PATCH 1/2] Remove unused remove_node_from_expr_list Alexander Monakov
  2022-07-19 15:15 ` [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347] Alexander Monakov
@ 2022-07-20  7:34 ` Richard Biener
  1 sibling, 0 replies; 6+ messages in thread
From: Richard Biener @ 2022-07-20  7:34 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: GCC Patches

On Tue, Jul 19, 2022 at 5:16 PM Alexander Monakov via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This function remains unused since remove_node_from_insn_list was cloned
> from it.

OK.

> gcc/ChangeLog:
>
>         * rtl.h (remove_node_from_expr_list): Remove declaration.
>         * rtlanal.cc (remove_node_from_expr_list): Remove (no uses).
> ---
>  gcc/rtl.h      |  1 -
>  gcc/rtlanal.cc | 29 -----------------------------
>  2 files changed, 30 deletions(-)
>
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index 488016bb4..645c009a3 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -3712,7 +3712,6 @@ extern unsigned hash_rtx_cb (const_rtx, machine_mode, int *, int *,
>  extern rtx regno_use_in (unsigned int, rtx);
>  extern int auto_inc_p (const_rtx);
>  extern bool in_insn_list_p (const rtx_insn_list *, const rtx_insn *);
> -extern void remove_node_from_expr_list (const_rtx, rtx_expr_list **);
>  extern void remove_node_from_insn_list (const rtx_insn *, rtx_insn_list **);
>  extern int loc_mentioned_in_p (rtx *, const_rtx);
>  extern rtx_insn *find_first_parameter_load (rtx_insn *, rtx_insn *);
> diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
> index d78cc6024..ec95ecd6c 100644
> --- a/gcc/rtlanal.cc
> +++ b/gcc/rtlanal.cc
> @@ -2878,35 +2878,6 @@ in_insn_list_p (const rtx_insn_list *listp, const rtx_insn *node)
>    return false;
>  }
>
> -/* Search LISTP (an EXPR_LIST) for an entry whose first operand is NODE and
> -   remove that entry from the list if it is found.
> -
> -   A simple equality test is used to determine if NODE matches.  */
> -
> -void
> -remove_node_from_expr_list (const_rtx node, rtx_expr_list **listp)
> -{
> -  rtx_expr_list *temp = *listp;
> -  rtx_expr_list *prev = NULL;
> -
> -  while (temp)
> -    {
> -      if (node == temp->element ())
> -       {
> -         /* Splice the node out of the list.  */
> -         if (prev)
> -           XEXP (prev, 1) = temp->next ();
> -         else
> -           *listp = temp->next ();
> -
> -         return;
> -       }
> -
> -      prev = temp;
> -      temp = temp->next ();
> -    }
> -}
> -
>  /* Search LISTP (an INSN_LIST) for an entry whose first operand is NODE and
>     remove that entry from the list if it is found.
>
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347]
  2022-07-19 15:15 ` [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347] Alexander Monakov
@ 2022-07-20  7:39   ` Richard Biener
  2022-07-20  8:31     ` Eric Botcazou
  0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2022-07-20  7:39 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: GCC Patches, Eric Botcazou

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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347]
  2022-07-20  7:39   ` Richard Biener
@ 2022-07-20  8:31     ` Eric Botcazou
  2022-07-20 13:13       ` Alexander Monakov
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2022-07-20  8:31 UTC (permalink / raw)
  To: Richard Biener; +Cc: Alexander Monakov, gcc-patches, GCC Patches

> 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.

-- 
Eric Botcazou



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347]
  2022-07-20  8:31     ` Eric Botcazou
@ 2022-07-20 13:13       ` Alexander Monakov
  0 siblings, 0 replies; 6+ messages in thread
From: Alexander Monakov @ 2022-07-20 13:13 UTC (permalink / raw)
  To: Eric Botcazou; +Cc: Richard Biener, GCC Patches


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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-07-20 13:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 15:15 [PATCH 1/2] Remove unused remove_node_from_expr_list Alexander Monakov
2022-07-19 15:15 ` [PATCH 2/2] Avoid registering __builtin_setjmp_receiver label twice [PR101347] Alexander Monakov
2022-07-20  7:39   ` Richard Biener
2022-07-20  8:31     ` Eric Botcazou
2022-07-20 13:13       ` Alexander Monakov
2022-07-20  7:34 ` [PATCH 1/2] Remove unused remove_node_from_expr_list Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).