| Message ID | 20200312003851.2366-2-jwjagersma@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | generate EH info for asm statements (PR93981) | expand |
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
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 >
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 > >
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.
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.
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
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
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
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.
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?
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.
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.
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
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.
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.
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: