[PATCH v3 0/2] generate EH info for asm statements (PR93981)

[PATCH v3 0/2] generate EH info for asm statements (PR93981)

[J.W. Jagersma]

As there were no more comments in the previous thread, I am submitting
a new revision of this patch.

The patch is split up in two parts now.  The first covers only volatile
asms and is mostly identical to the last revision.  There are no code
changes, only the test cases and documentation are expanded.

The second part extends this to cover also non-volatile asms in the
generated EH tables.  I have separated this since I do not know if this
change might affect optimization.  I'm hoping someone here can clarify
this.  If it does not disable (m)any optimizations, then I expect there
will be no objections to also include this second part.

[PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

The following patch extends the generation of exception handling
information, so that it is possible to catch exceptions thrown from
volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
of the gcc code already suggested this should be possible, but it was
never fully implemented.

Two new test cases are added.  The target-dependent test should pass on
platforms where throwing from a signal handler is allowed.  The only
platform I am aware of where that is the case is *-linux-gnu, so it is
set to XFAIL on all others.

gcc/
2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>

	PR inline-asm/93981
	* tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
	* tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
	Assign register output operands to temporaries.
	* doc/extend.texi: Document that volatile asms can now throw.

gcc/testsuite/
2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>

	PR inline-asm/93981
	* g++.target/i386/pr93981.C: New test.
	* g++.dg/eh/pr93981.C: New test.
---
 gcc/doc/extend.texi                     |  5 +++
 gcc/testsuite/g++.dg/eh/pr93981.C       | 18 ++++++++
 gcc/testsuite/g++.target/i386/pr93981.C | 55 +++++++++++++++++++++++++
 gcc/tree-cfg.c                          |  2 +
 gcc/tree-eh.c                           | 32 ++++++++++++++
 5 files changed, 112 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/eh/pr93981.C
 create mode 100644 gcc/testsuite/g++.target/i386/pr93981.C

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e0e7f540c21..b51e34c617a 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -9577,6 +9577,11 @@ errors during compilation if your @code{asm} code defines symbols or labels.
 Using @samp{%=} 
 (@pxref{AssemblerTemplate}) may help resolve this problem.
 
+When non-call exceptions (@option{-fnon-call-exceptions}) are enabled, a
+@code{volatile asm} statement is also allowed to throw exceptions.  If it does,
+then its register output operands are assumed to be clobbered and will not be
+used.  Memory operands, however, are always considered valid.
+
 @anchor{AssemblerTemplate}
 @subsubsection Assembler Template
 @cindex @code{asm} assembler template
diff --git a/gcc/testsuite/g++.dg/eh/pr93981.C b/gcc/testsuite/g++.dg/eh/pr93981.C
new file mode 100644
index 00000000000..a9adb5c069e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/eh/pr93981.C
@@ -0,0 +1,18 @@
+// PR inline-asm/93981
+// { dg-do compile }
+// { dg-options "-fnon-call-exceptions" }
+
+void
+f ()
+{
+  try
+    {
+      asm ("#try");
+    }
+  catch (...)
+    {
+      asm ("#catch");
+    }
+}
+
+// { dg-final { scan-assembler "#catch" } }
diff --git a/gcc/testsuite/g++.target/i386/pr93981.C b/gcc/testsuite/g++.target/i386/pr93981.C
new file mode 100644
index 00000000000..7a3117901f9
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/pr93981.C
@@ -0,0 +1,55 @@
+// PR inline-asm/93981
+// { dg-do run }
+// { dg-options "-fnon-call-exceptions -O3" }
+// { dg-xfail-if "" { ! *-linux-gnu } }
+// { dg-xfail-run-if "" { ! *-linux-gnu } }
+
+#include <signal.h>
+
+struct illegal_opcode { };
+
+extern "C" void
+sigill (int)
+{
+  throw illegal_opcode ( );
+}
+
+int
+test_mem ()
+{
+  int i = 2;
+  try
+    {
+      asm volatile ("mov%z0 $1, %0; ud2" : "=m" (i));
+    }
+  catch (const illegal_opcode&)
+    {
+      if (i == 1) return 0;
+    }
+  return i;
+}
+
+int
+test_reg ()
+{
+  int i = 8;
+  try
+    {
+      asm volatile ("mov%z0 $4, %0; ud2" : "=r" (i));
+    }
+  catch (const illegal_opcode&)
+    {
+      if (i == 8) return 0;
+    }
+  return i;
+}
+
+int
+main ()
+{
+  struct sigaction sa = { };
+  sa.sa_handler = sigill;
+  sa.sa_flags = SA_NODEFER;
+  sigaction (SIGILL, &sa, 0);
+  return test_mem () | test_reg ();
+}
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index f7b817d94e6..c21a7978493 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -913,6 +913,8 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
       break;
 
     case GIMPLE_ASM:
+      if (stmt_can_throw_internal (cfun, last))
+	make_eh_edges (last);
       make_gimple_asm_edges (bb);
       fallthru = true;
       break;
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 2a409dcaffe..58b16aa763a 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
 	    DECL_GIMPLE_REG_P (tmp) = 1;
 	  gsi_insert_after (gsi, s, GSI_SAME_STMT);
 	}
+
+record_throwing_stmt:
       /* Look for things that can throw exceptions, and record them.  */
       if (state->cur_region && stmt_could_throw_p (cfun, stmt))
 	{
@@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
 	}
       break;
 
+    case GIMPLE_ASM:
+      {
+	/* As above with GIMPLE_ASSIGN.  Change each register output operand
+	   to a temporary and insert a new stmt to assign this to the original
+	   operand.  */
+	gasm *asm_stmt = as_a <gasm *> (stmt);
+	if (stmt_could_throw_p (cfun, stmt)
+	    && gimple_asm_noutputs (asm_stmt) > 0
+	    && gimple_stmt_may_fallthru (stmt))
+	  {
+	    for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
+	      {
+		tree op = gimple_asm_output_op (asm_stmt, i);
+		tree opval = TREE_VALUE (op);
+		if (tree_could_throw_p (opval)
+		    || !is_gimple_reg_type (TREE_TYPE (opval))
+		    || !is_gimple_reg (get_base_address (opval)))
+		  continue;
+
+		tree tmp = create_tmp_reg (TREE_TYPE (opval));
+		gimple *s = gimple_build_assign (opval, tmp);
+		gimple_set_location (s, gimple_location (stmt));
+		gimple_set_block (s, gimple_block (stmt));
+		TREE_VALUE (op) = tmp;
+		gsi_insert_after (gsi, s, GSI_SAME_STMT);
+	      }
+	  }
+      }
+      goto record_throwing_stmt;
+
     case GIMPLE_COND:
     case GIMPLE_GOTO:
     case GIMPLE_RETURN:
-- 
2.25.1

[PATCH v3 2/2] generate EH info for all asm statements (PR93981)

[J.W. Jagersma]

This patch extends the generation of exception handling information to
cover all asm statements, when -fnon-call-exceptions is given.  The
previous patch only enabled this for volatile asms.

The previously added test cases are adjusted to cover this change.

gcc/
2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>

	PR inline-asm/93981
	* tree-eh.c (tree_could_trap_p): Return true for case ASM_EXPR.
	(stmt_could_throw_p): Likewise, for GIMPLE_ASM.
	* rtlanal.c (may_trap_p_1): Likewise, for ASM_OPERANDS.
	* doc/extend.texi: Document this change in the appropriate
	subsection.

gcc/testsuite/
2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>

	PR inline-asm/93981
	* g++.dg/eh/pr93981.C: Cover non-volatile throwing asms.
	* g++.target/i386/pr93981.C: Likewise.
---
 gcc/doc/extend.texi                     | 10 +++----
 gcc/rtlanal.c                           |  4 +--
 gcc/testsuite/g++.dg/eh/pr93981.C       | 22 ++++++++++++---
 gcc/testsuite/g++.target/i386/pr93981.C | 36 ++++++++++++++++++++++---
 gcc/tree-eh.c                           |  4 +--
 5 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index b51e34c617a..43929d9b129 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -9454,6 +9454,11 @@ printf("%d\n", dst);
 
 This code copies @code{src} to @code{dst} and add 1 to @code{dst}.
 
+When non-call exceptions (@option{-fnon-call-exceptions}) are enabled, an
+@code{asm} statement is also allowed to throw exceptions.  If it does, then its
+register output operands are assumed to be clobbered and will not be used.
+Memory operands, however, are always considered valid.
+
 @anchor{Volatile}
 @subsubsection Volatile
 @cindex volatile @code{asm}
@@ -9577,11 +9582,6 @@ errors during compilation if your @code{asm} code defines symbols or labels.
 Using @samp{%=} 
 (@pxref{AssemblerTemplate}) may help resolve this problem.
 
-When non-call exceptions (@option{-fnon-call-exceptions}) are enabled, a
-@code{volatile asm} statement is also allowed to throw exceptions.  If it does,
-then its register output operands are assumed to be clobbered and will not be
-used.  Memory operands, however, are always considered valid.
-
 @anchor{AssemblerTemplate}
 @subsubsection Assembler Template
 @cindex @code{asm} assembler template
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index c7ab86e228b..e7eb2dad11b 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -2831,13 +2831,11 @@ may_trap_p_1 (const_rtx x, unsigned flags)
       return targetm.unspec_may_trap_p (x, flags);
 
     case UNSPEC_VOLATILE:
+    case ASM_OPERANDS:
     case ASM_INPUT:
     case TRAP_IF:
       return 1;
 
-    case ASM_OPERANDS:
-      return MEM_VOLATILE_P (x);
-
       /* Memory ref can trap unless it's a static var or a stack slot.  */
     case MEM:
       /* Recognize specific pattern of stack checking probes.  */
diff --git a/gcc/testsuite/g++.dg/eh/pr93981.C b/gcc/testsuite/g++.dg/eh/pr93981.C
index a9adb5c069e..2e0d1cb881c 100644
--- a/gcc/testsuite/g++.dg/eh/pr93981.C
+++ b/gcc/testsuite/g++.dg/eh/pr93981.C
@@ -3,7 +3,7 @@
 // { dg-options "-fnon-call-exceptions" }
 
 void
-f ()
+foo ()
 {
   try
     {
@@ -11,8 +11,24 @@ f ()
     }
   catch (...)
     {
-      asm ("#catch");
+      asm ("#catch-v");
     }
 }
 
-// { dg-final { scan-assembler "#catch" } }
+int
+bar ()
+{
+  int i;
+  try
+    {
+      asm ("#try" : "=rm" (i));
+    }
+  catch (...)
+    {
+      asm ("#catch-nv");
+    }
+  return i;
+}
+
+// { dg-final { scan-assembler "#catch-v" } }
+// { dg-final { scan-assembler "#catch-nv" } }
diff --git a/gcc/testsuite/g++.target/i386/pr93981.C b/gcc/testsuite/g++.target/i386/pr93981.C
index 7a3117901f9..e4b5d7cd90f 100644
--- a/gcc/testsuite/g++.target/i386/pr93981.C
+++ b/gcc/testsuite/g++.target/i386/pr93981.C
@@ -15,7 +15,7 @@ sigill (int)
 }
 
 int
-test_mem ()
+test_mem_v ()
 {
   int i = 2;
   try
@@ -30,7 +30,7 @@ test_mem ()
 }
 
 int
-test_reg ()
+test_reg_v ()
 {
   int i = 8;
   try
@@ -44,6 +44,36 @@ test_reg ()
   return i;
 }
 
+int
+test_mem_nv ()
+{
+  int i = 32;
+  try
+    {
+      asm ("mov%z0 $16, %0; ud2" : "=m" (i));
+    }
+  catch (const illegal_opcode&)
+    {
+      if (i == 16) return 0;
+    }
+  return i;
+}
+
+int
+test_reg_nv ()
+{
+  int i = 128;
+  try
+    {
+      asm ("mov%z0 $64, %0; ud2" : "=r" (i));
+    }
+  catch (const illegal_opcode&)
+    {
+      if (i == 128) return 0;
+    }
+  return i;
+}
+
 int
 main ()
 {
@@ -51,5 +81,5 @@ main ()
   sa.sa_handler = sigill;
   sa.sa_flags = SA_NODEFER;
   sigaction (SIGILL, &sa, 0);
-  return test_mem () | test_reg ();
+  return test_mem_v () | test_reg_v () | test_mem_nv () | test_reg_nv ();
 }
diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 58b16aa763a..b2e17099320 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -2760,7 +2760,7 @@ tree_could_trap_p (tree expr)
       return !TREE_THIS_NOTRAP (expr);
 
     case ASM_EXPR:
-      return TREE_THIS_VOLATILE (expr);
+      return true;
 
     case CALL_EXPR:
       t = get_callee_fndecl (expr);
@@ -2964,7 +2964,7 @@ stmt_could_throw_p (function *fun, gimple *stmt)
     case GIMPLE_ASM:
       if (fun && !fun->can_throw_non_call_exceptions)
         return false;
-      return gimple_asm_volatile_p (as_a <gasm *> (stmt));
+      return true;
 
     default:
       return false;
-- 
2.25.1

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Jeff Law]

On 3/11/20 6:38 PM, J.W. Jagersma via Gcc-patches wrote:
> The following patch extends the generation of exception handling
> information, so that it is possible to catch exceptions thrown from
> volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
> of the gcc code already suggested this should be possible, but it was
> never fully implemented.
>
> Two new test cases are added.  The target-dependent test should pass on
> platforms where throwing from a signal handler is allowed.  The only
> platform I am aware of where that is the case is *-linux-gnu, so it is
> set to XFAIL on all others.
>
> gcc/
> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>
> 	PR inline-asm/93981
> 	* tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
> 	* tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
> 	Assign register output operands to temporaries.
> 	* doc/extend.texi: Document that volatile asms can now throw.
>
> gcc/testsuite/
> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>
> 	PR inline-asm/93981
> 	* g++.target/i386/pr93981.C: New test.
> 	* g++.dg/eh/pr93981.C: New test.

Is this the final version of the patch?  Do we have agreement on the
sematics for output operands, particularly memory operands?  The last
few messages in the March thread lead me to believe that's still not
settled.


Jeff

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Richard Biener]

On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> The following patch extends the generation of exception handling
> information, so that it is possible to catch exceptions thrown from
> volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
> of the gcc code already suggested this should be possible, but it was
> never fully implemented.
>
> Two new test cases are added.  The target-dependent test should pass on
> platforms where throwing from a signal handler is allowed.  The only
> platform I am aware of where that is the case is *-linux-gnu, so it is
> set to XFAIL on all others.
>
> gcc/
> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>
>         PR inline-asm/93981
>         * tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
>         * tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
>         Assign register output operands to temporaries.
>         * doc/extend.texi: Document that volatile asms can now throw.
>
> gcc/testsuite/
> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>
>         PR inline-asm/93981
>         * g++.target/i386/pr93981.C: New test.
>         * g++.dg/eh/pr93981.C: New test.
> ---
>  gcc/doc/extend.texi                     |  5 +++
>  gcc/testsuite/g++.dg/eh/pr93981.C       | 18 ++++++++
>  gcc/testsuite/g++.target/i386/pr93981.C | 55 +++++++++++++++++++++++++
>  gcc/tree-cfg.c                          |  2 +
>  gcc/tree-eh.c                           | 32 ++++++++++++++
>  5 files changed, 112 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/eh/pr93981.C
>  create mode 100644 gcc/testsuite/g++.target/i386/pr93981.C
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index e0e7f540c21..b51e34c617a 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -9577,6 +9577,11 @@ errors during compilation if your @code{asm} code defines symbols or labels.
>  Using @samp{%=}
>  (@pxref{AssemblerTemplate}) may help resolve this problem.
>
> +When non-call exceptions (@option{-fnon-call-exceptions}) are enabled, a
> +@code{volatile asm} statement is also allowed to throw exceptions.  If it does,
> +then its register output operands are assumed to be clobbered and will not be
> +used.  Memory operands, however, are always considered valid.
> +
>  @anchor{AssemblerTemplate}
>  @subsubsection Assembler Template
>  @cindex @code{asm} assembler template
> diff --git a/gcc/testsuite/g++.dg/eh/pr93981.C b/gcc/testsuite/g++.dg/eh/pr93981.C
> new file mode 100644
> index 00000000000..a9adb5c069e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/eh/pr93981.C
> @@ -0,0 +1,18 @@
> +// PR inline-asm/93981
> +// { dg-do compile }
> +// { dg-options "-fnon-call-exceptions" }
> +
> +void
> +f ()
> +{
> +  try
> +    {
> +      asm ("#try");
> +    }
> +  catch (...)
> +    {
> +      asm ("#catch");
> +    }
> +}
> +
> +// { dg-final { scan-assembler "#catch" } }
> diff --git a/gcc/testsuite/g++.target/i386/pr93981.C b/gcc/testsuite/g++.target/i386/pr93981.C
> new file mode 100644
> index 00000000000..7a3117901f9
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/i386/pr93981.C
> @@ -0,0 +1,55 @@
> +// PR inline-asm/93981
> +// { dg-do run }
> +// { dg-options "-fnon-call-exceptions -O3" }
> +// { dg-xfail-if "" { ! *-linux-gnu } }
> +// { dg-xfail-run-if "" { ! *-linux-gnu } }
> +
> +#include <signal.h>
> +
> +struct illegal_opcode { };
> +
> +extern "C" void
> +sigill (int)
> +{
> +  throw illegal_opcode ( );
> +}
> +
> +int
> +test_mem ()
> +{
> +  int i = 2;
> +  try
> +    {
> +      asm volatile ("mov%z0 $1, %0; ud2" : "=m" (i));
> +    }
> +  catch (const illegal_opcode&)
> +    {
> +      if (i == 1) return 0;
> +    }
> +  return i;
> +}
> +
> +int
> +test_reg ()
> +{
> +  int i = 8;
> +  try
> +    {
> +      asm volatile ("mov%z0 $4, %0; ud2" : "=r" (i));
> +    }
> +  catch (const illegal_opcode&)
> +    {
> +      if (i == 8) return 0;
> +    }
> +  return i;
> +}
> +
> +int
> +main ()
> +{
> +  struct sigaction sa = { };
> +  sa.sa_handler = sigill;
> +  sa.sa_flags = SA_NODEFER;
> +  sigaction (SIGILL, &sa, 0);
> +  return test_mem () | test_reg ();
> +}
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index f7b817d94e6..c21a7978493 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -913,6 +913,8 @@ make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index)
>        break;
>
>      case GIMPLE_ASM:
> +      if (stmt_can_throw_internal (cfun, last))
> +       make_eh_edges (last);
>        make_gimple_asm_edges (bb);
>        fallthru = true;
>        break;
> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
> index 2a409dcaffe..58b16aa763a 100644
> --- a/gcc/tree-eh.c
> +++ b/gcc/tree-eh.c
> @@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>             DECL_GIMPLE_REG_P (tmp) = 1;
>           gsi_insert_after (gsi, s, GSI_SAME_STMT);
>         }
> +
> +record_throwing_stmt:
>        /* Look for things that can throw exceptions, and record them.  */
>        if (state->cur_region && stmt_could_throw_p (cfun, stmt))
>         {
> @@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>         }
>        break;
>
> +    case GIMPLE_ASM:
> +      {
> +       /* As above with GIMPLE_ASSIGN.  Change each register output operand
> +          to a temporary and insert a new stmt to assign this to the original
> +          operand.  */
> +       gasm *asm_stmt = as_a <gasm *> (stmt);
> +       if (stmt_could_throw_p (cfun, stmt)
> +           && gimple_asm_noutputs (asm_stmt) > 0
> +           && gimple_stmt_may_fallthru (stmt))
> +         {
> +           for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
> +             {
> +               tree op = gimple_asm_output_op (asm_stmt, i);
> +               tree opval = TREE_VALUE (op);
> +               if (tree_could_throw_p (opval)
> +                   || !is_gimple_reg_type (TREE_TYPE (opval))
> +                   || !is_gimple_reg (get_base_address (opval)))
> +                 continue;
> +
> +               tree tmp = create_tmp_reg (TREE_TYPE (opval));
> +               gimple *s = gimple_build_assign (opval, tmp);
> +               gimple_set_location (s, gimple_location (stmt));
> +               gimple_set_block (s, gimple_block (stmt));
> +               TREE_VALUE (op) = tmp;
> +               gsi_insert_after (gsi, s, GSI_SAME_STMT);
> +             }
> +         }
> +      }
> +      goto record_throwing_stmt;

Can you avoid the ugly goto by simply duplicating the common code please?

Otherwise OK.

As you say volatile asms are already considered throwing in some pieces of
code so this is a step towards fulfilling that promise.

Thanks,
Richard.

> +
>      case GIMPLE_COND:
>      case GIMPLE_GOTO:
>      case GIMPLE_RETURN:
> --
> 2.25.1
>

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Richard Biener]

On Thu, Nov 12, 2020 at 4:53 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> On 3/11/20 6:38 PM, J.W. Jagersma via Gcc-patches wrote:
> > The following patch extends the generation of exception handling
> > information, so that it is possible to catch exceptions thrown from
> > volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
> > of the gcc code already suggested this should be possible, but it was
> > never fully implemented.
> >
> > Two new test cases are added.  The target-dependent test should pass on
> > platforms where throwing from a signal handler is allowed.  The only
> > platform I am aware of where that is the case is *-linux-gnu, so it is
> > set to XFAIL on all others.
> >
> > gcc/
> > 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
> >
> >       PR inline-asm/93981
> >       * tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
> >       * tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
> >       Assign register output operands to temporaries.
> >       * doc/extend.texi: Document that volatile asms can now throw.
> >
> > gcc/testsuite/
> > 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
> >
> >       PR inline-asm/93981
> >       * g++.target/i386/pr93981.C: New test.
> >       * g++.dg/eh/pr93981.C: New test.
>
> Is this the final version of the patch?  Do we have agreement on the
> sematics for output operands, particularly memory operands?  The last
> few messages in the March thread lead me to believe that's still not
> settled.

I think it's up to the asm itself to put the correct contents.  For the
cases where GCC needs to emit copies from outputs (that is,
if it ever reloads them) the only sensible thing is that those are
not emitted on the EH edge but only on the fallthru one.

On GIMPLE this cannot be represented but it means that
SSA uses of asm defs may not appear on the EH edge
(I do have some checking patch for this somewhere which
I think catches one or two existing problems).  On RTL if
the outputs are registers we cannot do any such checking
of course (no SSA form) and whether the old or the "new"
value lives is an implementation detail of the asm itself.

Richard.

>
> Jeff
>
>

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-12 16:51, Jeff Law wrote:
> 
> On 3/11/20 6:38 PM, J.W. Jagersma via Gcc-patches wrote:
>> The following patch extends the generation of exception handling
>> information, so that it is possible to catch exceptions thrown from
>> volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
>> of the gcc code already suggested this should be possible, but it was
>> never fully implemented.
>>
>> Two new test cases are added.  The target-dependent test should pass on
>> platforms where throwing from a signal handler is allowed.  The only
>> platform I am aware of where that is the case is *-linux-gnu, so it is
>> set to XFAIL on all others.
>>
>> gcc/
>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>
>> 	PR inline-asm/93981
>> 	* tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
>> 	* tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
>> 	Assign register output operands to temporaries.
>> 	* doc/extend.texi: Document that volatile asms can now throw.
>>
>> gcc/testsuite/
>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>
>> 	PR inline-asm/93981
>> 	* g++.target/i386/pr93981.C: New test.
>> 	* g++.dg/eh/pr93981.C: New test.
> 
> Is this the final version of the patch?  Do we have agreement on the
> sematics for output operands, particularly memory operands?  The last
> few messages in the March thread lead me to believe that's still not
> settled.
> 
> 
> Jeff

Hi Jeff,

From what I remember, no consensus was reached.  The discussion didn't seem
to be going anywhere, and I had found a suitable workaround, so the issue
dropped off my radar.  However this workaround now turned out to be somewhat
fragile so I do hope to see this implemented in gcc someday.

I'll have to check but I do think this is the "best" version I have of this
patch.  In my most recent branch here I changed all memory operands to inout,
but I recall there being some problem with this.  Anyway, as I think Richard
said (most of this goes over my head), I think this is best left up to the
user.  If one expects their asm to throw they would also be smart enough to
use the '+' modifier so that previous assignments are not optimized out.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-13 09:41, Richard Biener wrote:
> On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
>> index 2a409dcaffe..58b16aa763a 100644
>> --- a/gcc/tree-eh.c
>> +++ b/gcc/tree-eh.c
>> @@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>             DECL_GIMPLE_REG_P (tmp) = 1;
>>           gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>         }
>> +
>> +record_throwing_stmt:
>>        /* Look for things that can throw exceptions, and record them.  */
>>        if (state->cur_region && stmt_could_throw_p (cfun, stmt))
>>         {
>> @@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>         }
>>        break;
>>
>> +    case GIMPLE_ASM:
>> +      {
>> +       /* As above with GIMPLE_ASSIGN.  Change each register output operand
>> +          to a temporary and insert a new stmt to assign this to the original
>> +          operand.  */
>> +       gasm *asm_stmt = as_a <gasm *> (stmt);
>> +       if (stmt_could_throw_p (cfun, stmt)
>> +           && gimple_asm_noutputs (asm_stmt) > 0
>> +           && gimple_stmt_may_fallthru (stmt))
>> +         {
>> +           for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
>> +             {
>> +               tree op = gimple_asm_output_op (asm_stmt, i);
>> +               tree opval = TREE_VALUE (op);
>> +               if (tree_could_throw_p (opval)
>> +                   || !is_gimple_reg_type (TREE_TYPE (opval))
>> +                   || !is_gimple_reg (get_base_address (opval)))
>> +                 continue;
>> +
>> +               tree tmp = create_tmp_reg (TREE_TYPE (opval));
>> +               gimple *s = gimple_build_assign (opval, tmp);
>> +               gimple_set_location (s, gimple_location (stmt));
>> +               gimple_set_block (s, gimple_block (stmt));
>> +               TREE_VALUE (op) = tmp;
>> +               gsi_insert_after (gsi, s, GSI_SAME_STMT);
>> +             }
>> +         }
>> +      }
>> +      goto record_throwing_stmt;
> 
> Can you avoid the ugly goto by simply duplicating the common code please?
> 
> Otherwise OK.
> 
> As you say volatile asms are already considered throwing in some pieces of
> code so this is a step towards fulfilling that promise.
> 
> Thanks,
> Richard.
> 

Hi Richard,

Thanks for your feedback.  I'll have to check again if I made any other
changes since this.  If not, and if there are no further objections, I will
resubmit this patch soon with this goto removed.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Jeff Law]

On 11/13/20 1:45 AM, Richard Biener wrote:
> On Thu, Nov 12, 2020 at 4:53 PM Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> On 3/11/20 6:38 PM, J.W. Jagersma via Gcc-patches wrote:
>>> The following patch extends the generation of exception handling
>>> information, so that it is possible to catch exceptions thrown from
>>> volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
>>> of the gcc code already suggested this should be possible, but it was
>>> never fully implemented.
>>>
>>> Two new test cases are added.  The target-dependent test should pass on
>>> platforms where throwing from a signal handler is allowed.  The only
>>> platform I am aware of where that is the case is *-linux-gnu, so it is
>>> set to XFAIL on all others.
>>>
>>> gcc/
>>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>>
>>>       PR inline-asm/93981
>>>       * tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
>>>       * tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
>>>       Assign register output operands to temporaries.
>>>       * doc/extend.texi: Document that volatile asms can now throw.
>>>
>>> gcc/testsuite/
>>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>>
>>>       PR inline-asm/93981
>>>       * g++.target/i386/pr93981.C: New test.
>>>       * g++.dg/eh/pr93981.C: New test.
>> Is this the final version of the patch?  Do we have agreement on the
>> sematics for output operands, particularly memory operands?  The last
>> few messages in the March thread lead me to believe that's still not
>> settled.
> I think it's up to the asm itself to put the correct contents.  For the
> cases where GCC needs to emit copies from outputs (that is,
> if it ever reloads them) the only sensible thing is that those are
> not emitted on the EH edge but only on the fallthru one.
On the non-EH edge everything should work as expected.

We can't know where in the ASM where the throw occurred and we don't
model what happens inside the ASM.  I think that combination inherently
means we have no way to reliably know the state any output operand on
the EH edge. 

>
> On GIMPLE this cannot be represented but it means that
> SSA uses of asm defs may not appear on the EH edge
> (I do have some checking patch for this somewhere which
> I think catches one or two existing problems).
Right.  I do wonder if we should have a clobber of the output operands
that are gimple registers on the EH edge though.  I think that most
closely matches the actual semantics.  A checker could see if there's
any SSA_NAME uses that are reached by one of those clobbers.  I think
all you and I aren't in agreement on is whether to issue the clobber on
the EH edge or not and the implications for how we'd check for this case.



>   On RTL if
> the outputs are registers we cannot do any such checking
> of course (no SSA form) and whether the old or the "new"
> value lives is an implementation detail of the asm itself.
Agreed.  I don't see a way to check this in RTL.   I'd like to hope that
if we catch it in gimple that it wouldn't ever be an issue in RTL.   But
I'm sure there'd be some path where it could sneak through ;(

jeff

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Jeff Law]

On 11/15/20 6:00 AM, J.W. Jagersma wrote:
> On 2020-11-12 16:51, Jeff Law wrote:
>> On 3/11/20 6:38 PM, J.W. Jagersma via Gcc-patches wrote:
>>> The following patch extends the generation of exception handling
>>> information, so that it is possible to catch exceptions thrown from
>>> volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
>>> of the gcc code already suggested this should be possible, but it was
>>> never fully implemented.
>>>
>>> Two new test cases are added.  The target-dependent test should pass on
>>> platforms where throwing from a signal handler is allowed.  The only
>>> platform I am aware of where that is the case is *-linux-gnu, so it is
>>> set to XFAIL on all others.
>>>
>>> gcc/
>>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>>
>>> 	PR inline-asm/93981
>>> 	* tree-cfg.c (make_edges_bb): Make EH edges for GIMPLE_ASM.
>>> 	* tree-eh.c (lower_eh_constructs_2): Add case for GIMPLE_ASM.
>>> 	Assign register output operands to temporaries.
>>> 	* doc/extend.texi: Document that volatile asms can now throw.
>>>
>>> gcc/testsuite/
>>> 2020-03-11  Jan W. Jagersma  <jwjagersma@gmail.com>
>>>
>>> 	PR inline-asm/93981
>>> 	* g++.target/i386/pr93981.C: New test.
>>> 	* g++.dg/eh/pr93981.C: New test.
>> Is this the final version of the patch?  Do we have agreement on the
>> sematics for output operands, particularly memory operands?  The last
>> few messages in the March thread lead me to believe that's still not
>> settled.
>>
>>
>> Jeff
> Hi Jeff,
>
> From what I remember, no consensus was reached.  The discussion didn't seem
> to be going anywhere, and I had found a suitable workaround, so the issue
> dropped off my radar.  However this workaround now turned out to be somewhat
> fragile so I do hope to see this implemented in gcc someday.
>
> I'll have to check but I do think this is the "best" version I have of this
> patch.  In my most recent branch here I changed all memory operands to inout,
> but I recall there being some problem with this.  Anyway, as I think Richard
> said (most of this goes over my head), I think this is best left up to the
> user.  If one expects their asm to throw they would also be smart enough to
> use the '+' modifier so that previous assignments are not optimized out.
I wouldn't expect that changing the memory operands to inout would
consistently work.

jeff

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Jeff Law]

On 11/15/20 6:04 AM, J.W. Jagersma via Gcc-patches wrote:
> On 2020-11-13 09:41, Richard Biener wrote:
>> On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>>> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
>>> index 2a409dcaffe..58b16aa763a 100644
>>> --- a/gcc/tree-eh.c
>>> +++ b/gcc/tree-eh.c
>>> @@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>             DECL_GIMPLE_REG_P (tmp) = 1;
>>>           gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>>         }
>>> +
>>> +record_throwing_stmt:
>>>        /* Look for things that can throw exceptions, and record them.  */
>>>        if (state->cur_region && stmt_could_throw_p (cfun, stmt))
>>>         {
>>> @@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>         }
>>>        break;
>>>
>>> +    case GIMPLE_ASM:
>>> +      {
>>> +       /* As above with GIMPLE_ASSIGN.  Change each register output operand
>>> +          to a temporary and insert a new stmt to assign this to the original
>>> +          operand.  */
>>> +       gasm *asm_stmt = as_a <gasm *> (stmt);
>>> +       if (stmt_could_throw_p (cfun, stmt)
>>> +           && gimple_asm_noutputs (asm_stmt) > 0
>>> +           && gimple_stmt_may_fallthru (stmt))
>>> +         {
>>> +           for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
>>> +             {
>>> +               tree op = gimple_asm_output_op (asm_stmt, i);
>>> +               tree opval = TREE_VALUE (op);
>>> +               if (tree_could_throw_p (opval)
>>> +                   || !is_gimple_reg_type (TREE_TYPE (opval))
>>> +                   || !is_gimple_reg (get_base_address (opval)))
>>> +                 continue;
>>> +
>>> +               tree tmp = create_tmp_reg (TREE_TYPE (opval));
>>> +               gimple *s = gimple_build_assign (opval, tmp);
>>> +               gimple_set_location (s, gimple_location (stmt));
>>> +               gimple_set_block (s, gimple_block (stmt));
>>> +               TREE_VALUE (op) = tmp;
>>> +               gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>> +             }
>>> +         }
>>> +      }
>>> +      goto record_throwing_stmt;
>> Can you avoid the ugly goto by simply duplicating the common code please?
>>
>> Otherwise OK.
>>
>> As you say volatile asms are already considered throwing in some pieces of
>> code so this is a step towards fulfilling that promise.
>>
>> Thanks,
>> Richard.
>>
> Hi Richard,
>
> Thanks for your feedback.  I'll have to check again if I made any other
> changes since this.  If not, and if there are no further objections, I will
> resubmit this patch soon with this goto removed.
Sounds good.  I'll keep an eye out for it.  I think we'll want to look
at the doc text one more time too to make sure it matches the semantics
we can actually guarantee.

Jeff

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-19 06:55, Jeff Law wrote:
> 
> 
> On 11/15/20 6:04 AM, J.W. Jagersma via Gcc-patches wrote:
>> On 2020-11-13 09:41, Richard Biener wrote:
>>> On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>>> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
>>>> index 2a409dcaffe..58b16aa763a 100644
>>>> --- a/gcc/tree-eh.c
>>>> +++ b/gcc/tree-eh.c
>>>> @@ -2077,6 +2077,8 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>>             DECL_GIMPLE_REG_P (tmp) = 1;
>>>>           gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>>>         }
>>>> +
>>>> +record_throwing_stmt:
>>>>        /* Look for things that can throw exceptions, and record them.  */
>>>>        if (state->cur_region && stmt_could_throw_p (cfun, stmt))
>>>>         {
>>>> @@ -2085,6 +2087,36 @@ lower_eh_constructs_2 (struct leh_state *state, gimple_stmt_iterator *gsi)
>>>>         }
>>>>        break;
>>>>
>>>> +    case GIMPLE_ASM:
>>>> +      {
>>>> +       /* As above with GIMPLE_ASSIGN.  Change each register output operand
>>>> +          to a temporary and insert a new stmt to assign this to the original
>>>> +          operand.  */
>>>> +       gasm *asm_stmt = as_a <gasm *> (stmt);
>>>> +       if (stmt_could_throw_p (cfun, stmt)
>>>> +           && gimple_asm_noutputs (asm_stmt) > 0
>>>> +           && gimple_stmt_may_fallthru (stmt))
>>>> +         {
>>>> +           for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); ++i)
>>>> +             {
>>>> +               tree op = gimple_asm_output_op (asm_stmt, i);
>>>> +               tree opval = TREE_VALUE (op);
>>>> +               if (tree_could_throw_p (opval)
>>>> +                   || !is_gimple_reg_type (TREE_TYPE (opval))
>>>> +                   || !is_gimple_reg (get_base_address (opval)))
>>>> +                 continue;
>>>> +
>>>> +               tree tmp = create_tmp_reg (TREE_TYPE (opval));
>>>> +               gimple *s = gimple_build_assign (opval, tmp);
>>>> +               gimple_set_location (s, gimple_location (stmt));
>>>> +               gimple_set_block (s, gimple_block (stmt));
>>>> +               TREE_VALUE (op) = tmp;
>>>> +               gsi_insert_after (gsi, s, GSI_SAME_STMT);
>>>> +             }
>>>> +         }
>>>> +      }
>>>> +      goto record_throwing_stmt;
>>> Can you avoid the ugly goto by simply duplicating the common code please?
>>>
>>> Otherwise OK.
>>>
>>> As you say volatile asms are already considered throwing in some pieces of
>>> code so this is a step towards fulfilling that promise.
>>>
>>> Thanks,
>>> Richard.
>>>
>> Hi Richard,
>>
>> Thanks for your feedback.  I'll have to check again if I made any other
>> changes since this.  If not, and if there are no further objections, I will
>> resubmit this patch soon with this goto removed.
> Sounds good.  I'll keep an eye out for it.  I think we'll want to look
> at the doc text one more time too to make sure it matches the semantics
> we can actually guarantee.
> 
> Jeff

The only hard guarantees we can make with this patch as-is, is for "=r" and
"+r" operands (clobber), and "+m" (keep).  I suppose the test case for memory
operands should be altered to use '+' and the inverse should be tested too
(what happens when it doesn't throw).

For "=rm" and "=m" we could say that the output value is undefined on
exception, as the first depends on whether the operand is a GIMPLE register
type or not, and the latter may have earlier assignments optimized out.  And
that is valid too, as there may be cases (possibly most cases) where you
wouldn't care what the value is either before or after the asm, if an
exception is thrown.

Another idea I had is to introduce a new operand modifier, eg. '-', which
would signify that the output *must* be considered clobbered on exception,
and it would be an error if a copy is not possible.  Then the meaning of '+'
can be extended to say that the output must be valid regardless of whether an
exception occured or not.  Modifier '=' would mean the same as '+' with the
additional implication that it is okay to eliminate earlier assignments, so
that its value on the EH edge would be undefined.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-21 12:27, J.W. Jagersma wrote:
> ...
> Another idea I had is to introduce a new operand modifier, eg. '-', which
> would signify that the output *must* be considered clobbered on exception,
> and it would be an error if a copy is not possible.  Then the meaning of '+'
> can be extended to say that the output must be valid regardless of whether an
> exception occured or not.  Modifier '=' would mean the same as '+' with the
> additional implication that it is okay to eliminate earlier assignments, so
> that its value on the EH edge would be undefined.

I've been working on implementing this, but I ran into an issue:

First of all, in the first version of this patch I had to make sure that
debug stmts were inserted across the fallthrough edge, so that they don't
appear at the end a basic block.  I was surprised to find that this is no
longer necessary.

Next I discovered that the following test case fails (returns -1):

    int f ()
    {
      int i = 0;
      try { asm ("mov %0, 1; ud2" : "+r" (i)); }
      catch (...) { }
      return i - 1;
    }

And this does succeed with a memory operand.

It seems that something changed in the past few months, and now asm register
outputs are already being assigned via a temporary variable, somewhere.  Does
anyone know where that might be?

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Richard Biener]

On Sun, Nov 22, 2020 at 5:38 PM J.W. Jagersma <jwjagersma@gmail.com> wrote:
>
> On 2020-11-21 12:27, J.W. Jagersma wrote:
> > ...
> > Another idea I had is to introduce a new operand modifier, eg. '-', which
> > would signify that the output *must* be considered clobbered on exception,
> > and it would be an error if a copy is not possible.  Then the meaning of '+'
> > can be extended to say that the output must be valid regardless of whether an
> > exception occured or not.  Modifier '=' would mean the same as '+' with the
> > additional implication that it is okay to eliminate earlier assignments, so
> > that its value on the EH edge would be undefined.
>
> I've been working on implementing this, but I ran into an issue:
>
> First of all, in the first version of this patch I had to make sure that
> debug stmts were inserted across the fallthrough edge, so that they don't
> appear at the end a basic block.  I was surprised to find that this is no
> longer necessary.
>
> Next I discovered that the following test case fails (returns -1):
>
>     int f ()
>     {
>       int i = 0;
>       try { asm ("mov %0, 1; ud2" : "+r" (i)); }
>       catch (...) { }
>       return i - 1;
>     }
>
> And this does succeed with a memory operand.
>
> It seems that something changed in the past few months, and now asm register
> outputs are already being assigned via a temporary variable, somewhere.  Does
> anyone know where that might be?

It's likely out-of SSA / RTL expansion inserting a bogus copy or doing
coalescing on the EH edge.  Semantics of throwing asm stmts need to be
honored there.

Richard.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-23 09:20, Richard Biener wrote:
> On Sun, Nov 22, 2020 at 5:38 PM J.W. Jagersma <jwjagersma@gmail.com> wrote:
>>
>> On 2020-11-21 12:27, J.W. Jagersma wrote:
>>> ...
>>> Another idea I had is to introduce a new operand modifier, eg. '-', which
>>> would signify that the output *must* be considered clobbered on exception,
>>> and it would be an error if a copy is not possible.  Then the meaning of '+'
>>> can be extended to say that the output must be valid regardless of whether an
>>> exception occured or not.  Modifier '=' would mean the same as '+' with the
>>> additional implication that it is okay to eliminate earlier assignments, so
>>> that its value on the EH edge would be undefined.
>>
>> I've been working on implementing this, but I ran into an issue:
>>
>> First of all, in the first version of this patch I had to make sure that
>> debug stmts were inserted across the fallthrough edge, so that they don't
>> appear at the end a basic block.  I was surprised to find that this is no
>> longer necessary.
>>
>> Next I discovered that the following test case fails (returns -1):
>>
>>     int f ()
>>     {
>>       int i = 0;
>>       try { asm ("mov %0, 1; ud2" : "+r" (i)); }
>>       catch (...) { }
>>       return i - 1;
>>     }
>>
>> And this does succeed with a memory operand.
>>
>> It seems that something changed in the past few months, and now asm register
>> outputs are already being assigned via a temporary variable, somewhere.  Does
>> anyone know where that might be?
> 
> It's likely out-of SSA / RTL expansion inserting a bogus copy or doing
> coalescing on the EH edge.  Semantics of throwing asm stmts need to be
> honored there.
> 
> Richard.

I believe the issue is caused by commit e3b3b59 which adds support for outputs
from asm goto (CC-ing the author of that patch).  I don't yet see how to
reconcile this with the idea to add a new constraint modifier.

Also, that patch has another problem:
    asm goto ("# %0 %1 %2" : "+r" (i) ::: jmp);
Prints two registers and a label offset, which is somewhat unexpected.

And then:
    asm goto ("# %l[jmp]" : "+r" (i) ::: jmp);
Produces an error: '%l' operand isn't a label.

So label operands are numbered after the in-out operand is split, but %l is
evaluated before the split.  The examples given in extend.texi can never work.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[Segher Boessenkool]

On Fri, Nov 13, 2020 at 09:41:28AM +0100, Richard Biener via Gcc-patches wrote:
> On Thu, Mar 12, 2020 at 1:41 AM J.W. Jagersma via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> > The following patch extends the generation of exception handling
> > information, so that it is possible to catch exceptions thrown from
> > volatile asm statements, when -fnon-call-exceptions is enabled.  Parts
> > of the gcc code already suggested this should be possible, but it was
> > never fully implemented.

> As you say volatile asms are already considered throwing in some pieces of
> code so this is a step towards fulfilling that promise.

But that is just wrong.  Volatile is an orthogonal concept.  There is
nothing wrong with having a non-volatile throwing asm, either, and it
can optimise better.

Can you just add some markup for throwing asm?

LLVM implements "asm goto" with outputs as a throwing insn, instead of
as a jump insn (as it should be, as it is documented!)  I suggested
doing an "asm break" (or whatever name, this is just a vaguely related
already existing keyword) for this, instead.  This has exactly the
semantics you want: all outputs are written on the normal path, and
they are either or not written when it throws.

The difference with your situation is in that case you specify all
labels the code could jump to, and in your case you don't.  Maybe
just one less colon could make the distinction?  (asm goto has four
colons, other asm has at most three).


Segher

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-23 09:20, Richard Biener wrote:
> On Sun, Nov 22, 2020 at 5:38 PM J.W. Jagersma <jwjagersma@gmail.com> wrote:
>>
>> On 2020-11-21 12:27, J.W. Jagersma wrote:
>>> ...
>>> Another idea I had is to introduce a new operand modifier, eg. '-', which
>>> would signify that the output *must* be considered clobbered on exception,
>>> and it would be an error if a copy is not possible.  Then the meaning of '+'
>>> can be extended to say that the output must be valid regardless of whether an
>>> exception occured or not.  Modifier '=' would mean the same as '+' with the
>>> additional implication that it is okay to eliminate earlier assignments, so
>>> that its value on the EH edge would be undefined.
>>
>> I've been working on implementing this, but I ran into an issue:
>>
>> First of all, in the first version of this patch I had to make sure that
>> debug stmts were inserted across the fallthrough edge, so that they don't
>> appear at the end a basic block.  I was surprised to find that this is no
>> longer necessary.
>>
>> Next I discovered that the following test case fails (returns -1):
>>
>>     int f ()
>>     {
>>       int i = 0;
>>       try { asm ("mov %0, 1; ud2" : "+r" (i)); }
>>       catch (...) { }
>>       return i - 1;
>>     }
>>
>> And this does succeed with a memory operand.
>>
>> It seems that something changed in the past few months, and now asm register
>> outputs are already being assigned via a temporary variable, somewhere.  Does
>> anyone know where that might be?
> 
> It's likely out-of SSA / RTL expansion inserting a bogus copy or doing
> coalescing on the EH edge.  Semantics of throwing asm stmts need to be
> honored there.
> 
> Richard.

Quick update.  I have a mostly working implementation now, the only case where
it fails is when an asm clobbers all callee-saved registers.  In this one case
the output is not available on the exception edge.  However this same case does
work correctly with an 'asm goto' that throws.

The problem I think is in lra_process_new_insns (lra.c:1878) where the condition
to insert new insns across edges is "JUMP_P (insn)", but this should happen for
throwing asms as well.  I figured "insn == BB_END (BLOCK_FOR_INSN (insn))" would
make sense but that causes all sorts of trouble (segfaults during bootstrap).

Would appreciate any advice on what to do here.

Re: [PATCH v3 1/2] generate EH info for volatile asm statements (PR93981)

[J.W. Jagersma]

On 2020-11-30 17:47, J.W. Jagersma wrote:
> On 2020-11-23 09:20, Richard Biener wrote:
>> On Sun, Nov 22, 2020 at 5:38 PM J.W. Jagersma <jwjagersma@gmail.com> wrote:
>>>
>>> On 2020-11-21 12:27, J.W. Jagersma wrote:
>>>> ...
>>>> Another idea I had is to introduce a new operand modifier, eg. '-', which
>>>> would signify that the output *must* be considered clobbered on exception,
>>>> and it would be an error if a copy is not possible.  Then the meaning of '+'
>>>> can be extended to say that the output must be valid regardless of whether an
>>>> exception occured or not.  Modifier '=' would mean the same as '+' with the
>>>> additional implication that it is okay to eliminate earlier assignments, so
>>>> that its value on the EH edge would be undefined.
>>>
>>> I've been working on implementing this, but I ran into an issue:
>>>
>>> First of all, in the first version of this patch I had to make sure that
>>> debug stmts were inserted across the fallthrough edge, so that they don't
>>> appear at the end a basic block.  I was surprised to find that this is no
>>> longer necessary.
>>>
>>> Next I discovered that the following test case fails (returns -1):
>>>
>>>     int f ()
>>>     {
>>>       int i = 0;
>>>       try { asm ("mov %0, 1; ud2" : "+r" (i)); }
>>>       catch (...) { }
>>>       return i - 1;
>>>     }
>>>
>>> And this does succeed with a memory operand.
>>>
>>> It seems that something changed in the past few months, and now asm register
>>> outputs are already being assigned via a temporary variable, somewhere.  Does
>>> anyone know where that might be?
>>
>> It's likely out-of SSA / RTL expansion inserting a bogus copy or doing
>> coalescing on the EH edge.  Semantics of throwing asm stmts need to be
>> honored there.
>>
>> Richard.
> 
> Quick update.  I have a mostly working implementation now, the only case where
> it fails is when an asm clobbers all callee-saved registers.  In this one case
> the output is not available on the exception edge.  However this same case does
> work correctly with an 'asm goto' that throws.
> 
> The problem I think is in lra_process_new_insns (lra.c:1878) where the condition
> to insert new insns across edges is "JUMP_P (insn)", but this should happen for
> throwing asms as well.  I figured "insn == BB_END (BLOCK_FOR_INSN (insn))" would
> make sense but that causes all sorts of trouble (segfaults during bootstrap).
> 
> Would appreciate any advice on what to do here.

Just realized I can make it work by simply expanding throwing asms as jump
insns with no label.  I'm not sure about the specific semantic differences
between regular and jump insns, but this doesn't seem to have any negative
effects so far.

I will submit v4 after I finish running regression tests and writing docs and
changelogs.  Or if this is not an acceptable workaround, please let me know.