diff mbox series

(RFA tree-tailcall) PR c++/82081 - tail call optimization breaks noexcept

Message ID 20190412212426.3411-1-jason@redhat.com
State New
Headers show
Series (RFA tree-tailcall) PR c++/82081 - tail call optimization breaks noexcept | expand

Commit Message

Jason Merrill April 12, 2019, 9:24 p.m. UTC 
If a noexcept function calls a function that might throw, doing the tail
call optimization means that an exception thrown in the called function
will propagate out, breaking the noexcept specification.  So we need to
prevent the optimization in that case.

Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
regression, but it is a straightforward fix for a wrong-code bug.

	* tree-tailcall.c (find_tail_calls): Don't turn a call from a
	nothrow function to a might-throw function into a tail call.
---
 gcc/tree-tailcall.c                         |  7 +++++++
 gcc/testsuite/g++.dg/tree-ssa/tail-call-1.C | 11 +++++++++++
 gcc/ChangeLog                               |  6 ++++++
 3 files changed, 24 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/tree-ssa/tail-call-1.C


base-commit: b7a39acf193fd19e41502d5e2cf6b3fa8b44dbac

Comments

Jeff Law April 12, 2019, 10:34 p.m. UTC | #1 
On 4/12/19 3:24 PM, Jason Merrill wrote:
> If a noexcept function calls a function that might throw, doing the tail
> call optimization means that an exception thrown in the called function
> will propagate out, breaking the noexcept specification.  So we need to
> prevent the optimization in that case.
> 
> Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> regression, but it is a straightforward fix for a wrong-code bug.
> 
> 	* tree-tailcall.c (find_tail_calls): Don't turn a call from a
> 	nothrow function to a might-throw function into a tail call.
I'd go on the trunk.  It's a wrong-code issue, what we're doing is just
plain wrong.  One could even make a case for backporting to the branches.

jeff

ps.  I'm a bit surprised it hasn't been reported until now.
Richard Biener April 15, 2019, 6:50 a.m. UTC | #2 
On Sat, Apr 13, 2019 at 12:34 AM Jeff Law <law@redhat.com> wrote:
>
> On 4/12/19 3:24 PM, Jason Merrill wrote:
> > If a noexcept function calls a function that might throw, doing the tail
> > call optimization means that an exception thrown in the called function
> > will propagate out, breaking the noexcept specification.  So we need to
> > prevent the optimization in that case.
> >
> > Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> > regression, but it is a straightforward fix for a wrong-code bug.
> >
> >       * tree-tailcall.c (find_tail_calls): Don't turn a call from a
> >       nothrow function to a might-throw function into a tail call.
> I'd go on the trunk.  It's a wrong-code issue, what we're doing is just
> plain wrong.  One could even make a case for backporting to the branches.

Hmm, how's this different from adding another indirection?  That is,
I don't understand why the tailcall is the issue here, shouldn't unwind
still stop at the noexcept caller?  Thus, isn't this wrong CFI instead?

Of course I know to little about this.

Btw, doesn't your check also prevent tail/sibling calls when
the caller wraps it into a try { } catch (...) {}?  Or does unwind
not work in that case either?

Btw, I'd like to see a runtime testcase that fails.

Richard.

> jeff
>
> ps.  I'm a bit surprised it hasn't been reported until now.
Andrew Pinski April 15, 2019, 5:09 p.m. UTC | #3 
On Sun, Apr 14, 2019 at 11:50 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Sat, Apr 13, 2019 at 12:34 AM Jeff Law <law@redhat.com> wrote:
> >
> > On 4/12/19 3:24 PM, Jason Merrill wrote:
> > > If a noexcept function calls a function that might throw, doing the tail
> > > call optimization means that an exception thrown in the called function
> > > will propagate out, breaking the noexcept specification.  So we need to
> > > prevent the optimization in that case.
> > >
> > > Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> > > regression, but it is a straightforward fix for a wrong-code bug.
> > >
> > >       * tree-tailcall.c (find_tail_calls): Don't turn a call from a
> > >       nothrow function to a might-throw function into a tail call.
> > I'd go on the trunk.  It's a wrong-code issue, what we're doing is just
> > plain wrong.  One could even make a case for backporting to the branches.
>
> Hmm, how's this different from adding another indirection?  That is,
> I don't understand why the tailcall is the issue here, shouldn't unwind
> still stop at the noexcept caller?  Thus, isn't this wrong CFI instead?

noexcept caller is no longer on the stack so the unwinder does not see it.
It is not the tail call from a normal function to a noexcept that is
an issue but rather inside a noexcept caller to a normal function.

>
> Of course I know to little about this.
>
> Btw, doesn't your check also prevent tail/sibling calls when
> the caller wraps it into a try { } catch (...) {}?  Or does unwind
> not work in that case either?
>
> Btw, I'd like to see a runtime testcase that fails.

There is one in the bug report.  Though it would not work for the
testsuite.  It should not be hard to change it to be one that works
for the testsuite.

Thanks,
Andrew Pinski

>
> Richard.
>
> > jeff
> >
> > ps.  I'm a bit surprised it hasn't been reported until now.
Richard Biener April 16, 2019, 8:23 a.m. UTC | #4 
On Mon, Apr 15, 2019 at 7:09 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Sun, Apr 14, 2019 at 11:50 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Sat, Apr 13, 2019 at 12:34 AM Jeff Law <law@redhat.com> wrote:
> > >
> > > On 4/12/19 3:24 PM, Jason Merrill wrote:
> > > > If a noexcept function calls a function that might throw, doing the tail
> > > > call optimization means that an exception thrown in the called function
> > > > will propagate out, breaking the noexcept specification.  So we need to
> > > > prevent the optimization in that case.
> > > >
> > > > Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> > > > regression, but it is a straightforward fix for a wrong-code bug.
> > > >
> > > >       * tree-tailcall.c (find_tail_calls): Don't turn a call from a
> > > >       nothrow function to a might-throw function into a tail call.
> > > I'd go on the trunk.  It's a wrong-code issue, what we're doing is just
> > > plain wrong.  One could even make a case for backporting to the branches.
> >
> > Hmm, how's this different from adding another indirection?  That is,
> > I don't understand why the tailcall is the issue here, shouldn't unwind
> > still stop at the noexcept caller?  Thus, isn't this wrong CFI instead?
>
> noexcept caller is no longer on the stack so the unwinder does not see it.
> It is not the tail call from a normal function to a noexcept that is
> an issue but rather inside a noexcept caller to a normal function.

Hmm, OK, so essentially a tail-call cannot be represented in the CFI
program.

> >
> > Of course I know to little about this.
> >
> > Btw, doesn't your check also prevent tail/sibling calls when
> > the caller wraps it into a try { } catch (...) {}?  Or does unwind
> > not work in that case either?
> >
> > Btw, I'd like to see a runtime testcase that fails.
>
> There is one in the bug report.  Though it would not work for the
> testsuite.  It should not be hard to change it to be one that works
> for the testsuite.

With dg-additional-sources and registering a custom std::terminate
it should work I guess (or by catching SIGABRT).

The patch and the bug also suggests that an internally
throwing function cannot be tail-called either (can't find a testcase
we'd mark as tail-call here)

Richard.

> Thanks,
> Andrew Pinski
>
> >
> > Richard.
> >
> > > jeff
> > >
> > > ps.  I'm a bit surprised it hasn't been reported until now.
Jason Merrill April 17, 2019, 10:02 a.m. UTC | #5 
On Tue, Apr 16, 2019 at 1:24 AM Richard Biener
<richard.guenther@gmail.com> wrote:
> On Mon, Apr 15, 2019 at 7:09 PM Andrew Pinski <pinskia@gmail.com> wrote:
> > On Sun, Apr 14, 2019 at 11:50 PM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Sat, Apr 13, 2019 at 12:34 AM Jeff Law <law@redhat.com> wrote:
> > > >
> > > > On 4/12/19 3:24 PM, Jason Merrill wrote:
> > > > > If a noexcept function calls a function that might throw, doing the tail
> > > > > call optimization means that an exception thrown in the called function
> > > > > will propagate out, breaking the noexcept specification.  So we need to
> > > > > prevent the optimization in that case.
> > > > >
> > > > > Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> > > > > regression, but it is a straightforward fix for a wrong-code bug.
> > > > >
> > > > >       * tree-tailcall.c (find_tail_calls): Don't turn a call from a
> > > > >       nothrow function to a might-throw function into a tail call.
> > > > I'd go on the trunk.  It's a wrong-code issue, what we're doing is just
> > > > plain wrong.  One could even make a case for backporting to the branches.
> > >
> > > Hmm, how's this different from adding another indirection?  That is,
> > > I don't understand why the tailcall is the issue here, shouldn't unwind
> > > still stop at the noexcept caller?  Thus, isn't this wrong CFI instead?
> >
> > noexcept caller is no longer on the stack so the unwinder does not see it.
> > It is not the tail call from a normal function to a noexcept that is
> > an issue but rather inside a noexcept caller to a normal function.
>
> Hmm, OK, so essentially a tail-call cannot be represented in the CFI
> program.

Right.  Because the "caller" frame no longer exists.

> > > Of course I know to little about this.
> > >
> > > Btw, doesn't your check also prevent tail/sibling calls when
> > > the caller wraps it into a try { } catch (...) {}?  Or does unwind
> > > not work in that case either?
> > >
> > > Btw, I'd like to see a runtime testcase that fails.
> >
> > There is one in the bug report.  Though it would not work for the
> > testsuite.  It should not be hard to change it to be one that works
> > for the testsuite.
>
> With dg-additional-sources and registering a custom std::terminate
> it should work I guess (or by catching SIGABRT).
>
> The patch and the bug also suggests that an internally
> throwing function cannot be tail-called either (can't find a testcase
> we'd mark as tail-call here)

If you mean a call wrapped in try/catch, that is correct.  The
tail-call optimization breaks all exception handlers, so the patch
prevents it if the call can throw and is in an active exception
region.

Jason
Jakub Jelinek April 26, 2019, 3:14 p.m. UTC | #6 
On Fri, Apr 12, 2019 at 05:24:26PM -0400, Jason Merrill wrote:
> If a noexcept function calls a function that might throw, doing the tail
> call optimization means that an exception thrown in the called function
> will propagate out, breaking the noexcept specification.  So we need to
> prevent the optimization in that case.
> 
> Tested x86_64-pc-linux-gnu.  OK for trunk or hold for GCC 10?  This isn't a
> regression, but it is a straightforward fix for a wrong-code bug.
> 
> 	* tree-tailcall.c (find_tail_calls): Don't turn a call from a
> 	nothrow function to a might-throw function into a tail call.

Ok for trunk (i.e. GCC 10) and we can backport later to 9.2 if it works fine
on the trunk.

	Jakub
diff mbox series

Patch

diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index afe8931b5f0..e0265b22dd5 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -37,6 +37,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-into-ssa.h"
 #include "tree-dfa.h"
 #include "except.h"
+#include "tree-eh.h"
 #include "dbgcnt.h"
 #include "cfgloop.h"
 #include "common/common-target.h"
@@ -472,6 +473,12 @@  find_tail_calls (basic_block bb, struct tailcall **ret)
       && !auto_var_in_fn_p (ass_var, cfun->decl))
     return;
 
+  /* If the call might throw an exception that wouldn't propagate out of
+     cfun, we can't transform to a tail or sibling call (82081).  */
+  if (stmt_could_throw_p (cfun, stmt)
+      && !stmt_can_throw_external (cfun, stmt))
+    return;
+
   /* We found the call, check whether it is suitable.  */
   tail_recursion = false;
   func = gimple_call_fndecl (call);
diff --git a/gcc/testsuite/g++.dg/tree-ssa/tail-call-1.C b/gcc/testsuite/g++.dg/tree-ssa/tail-call-1.C
new file mode 100644
index 00000000000..c67af6e41c7
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tree-ssa/tail-call-1.C
@@ -0,0 +1,11 @@ 
+// PR c++/82081
+// { dg-do compile { target c++11 } }
+// { dg-additional-options "-O2 -fdump-tree-optimized" }
+// { dg-final { scan-tree-dump-not "tail call" "optimized" } }
+
+int g(int) ;
+
+int f() noexcept {
+    int i = 42, j = 43;
+    return g(i+j);
+}
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 02d3d07d0e4..77ab20929ff 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,9 @@ 
+2019-04-11  Jason Merrill  <jason@redhat.com>
+
+	PR c++/82081 - tail call optimization breaks noexcept
+	* tree-tailcall.c (find_tail_calls): Don't turn a call from a
+	nothrow function to a might-throw function into a tail call.
+
 2019-04-12  Jakub Jelinek  <jakub@redhat.com>
 	
 	PR rtl-optimization/89965