can_throw_internal affected by inlining?

can_throw_internal affected by inlining?

[Richard Henderson]

Re: http://gcc.gnu.org/ml/gcc-patches/2009-03/msg01404.html

Do you have test cases for this?

Changing can_throw_internal/external to depend on whether or not future 
inlining is possible looks *very* wrong to me.  Surely the only thing 
that matters for new code that might appear "below" this position in the 
tree is whether or not it might throw, and the only thing that changes 
with inlining is increased knowledge of whether and how it throws.

The only thing I can imagine is that somehow an inline function was 
incorrectly marked as nothrow, and then it was inlined exposing the 
throw (i.e. resx) which then led the problem you report.

On the trans-mem branch this is causing me problems.  I'm replacing a 
direct function call with an indirect function call.  Neither can be 
inlined, but inlinable_call_p thinks that it's a possibility for the 
indirect function call.  With your logic, this magically changes a 
statement from !can_throw_internal to can_throw_internal.  Which then of 
course results in a verify_cfg abort.


r~

Re: can_throw_internal affected by inlining?

[Jan Hubicka]

> Re: http://gcc.gnu.org/ml/gcc-patches/2009-03/msg01404.html
> 
> Do you have test cases for this?
> 
> Changing can_throw_internal/external to depend on whether or not future 
> inlining is possible looks *very* wrong to me.  Surely the only thing 
> that matters for new code that might appear "below" this position in the 
> tree is whether or not it might throw, and the only thing that changes 
> with inlining is increased knowledge of whether and how it throws.

The problem here is fact that MUST_NOT_THROW region reachable only via
runtime is handled completely via runtime, however MUST_NOT_THROW region
reachable via RESX is eventually going to be handled by direct
std::terminate call, since RESX will eventually get translated as direct
goto to the MIST_NOT_THROW reciever.

When compiling, C++ frontend first produce MUST_NOT_THROW receivers
doing direct call and those are left in code till they are renered
unreachable and removed by cleanup_cfg.

This decision do depend on inlining - when you call function with
externally throwing RESX in MUST_NOT_THROW region, you don't need
std::terminate() call since runtime will handle it. After you inline it,
you must have the std::terminate() call.  We got this wrong on old tree
removing the receivers and sometimes incorrectly optimizing out
MUST_NOT_THROW wrappers.  One can construct testcase with inlined
destructor in cleanup that have externally throwing resx that won't
result in terminate call afterwards, but will silently continue
unwinding form the cleanup.

So I am delaying removal of those receiver after inlining decisions are
fixed, but because these regions tends to be very numberous and tends to
consume a lot of memory, I also added the logic to remove them early
when it is known that they are not needed.
> 
> The only thing I can imagine is that somehow an inline function was 
> incorrectly marked as nothrow, and then it was inlined exposing the 
> throw (i.e. resx) which then led the problem you report.
> 
> On the trans-mem branch this is causing me problems.  I'm replacing a 
> direct function call with an indirect function call.  Neither can be 
> inlined, but inlinable_call_p thinks that it's a possibility for the 
> indirect function call.  With your logic, this magically changes a 
> statement from !can_throw_internal to can_throw_internal.  Which then of 
> course results in a verify_cfg abort.

Well, we can either teach inlinable_call_p to handle your new indirect
calls as "for sure uninlinable", make it conservative and consider all
calls inlinable or we can stop doing the early removal of MUST_NOT_THROW
receivers.  I also added logic to inliner heuristics to prevent those
from increasing estimated function bodies, so it should affect mostly
memory usage and compile time, I can re-test that.

Honza
> 
> 
> r~

Re: can_throw_internal affected by inlining?

[Richard Henderson]

On 07/11/2009 05:59 AM, Jan Hubicka wrote:
> Well, we can either teach inlinable_call_p to handle your new indirect
> calls as "for sure uninlinable", make it conservative and consider all
> calls inlinable or we can stop doing the early removal of MUST_NOT_THROW
> receivers.

I think this last option would probably be best.  Something to work on 
next week...


r~

Re: can_throw_internal affected by inlining?

[Jan Hubicka]

> On 07/11/2009 05:59 AM, Jan Hubicka wrote:
> >Well, we can either teach inlinable_call_p to handle your new indirect
> >calls as "for sure uninlinable", make it conservative and consider all
> >calls inlinable or we can stop doing the early removal of MUST_NOT_THROW
> >receivers.
> 
> I think this last option would probably be best.  Something to work on 
> next week...

It is easilly doable by making inlinable_call_p to return always true
pre-inlining (even for non-calls).  I can drop it to C++ testers so we
can see how much memory/compile time overhad does this imply.

I would like to bring more of EH lowering to tree level (i.e. instead of
relying on RTL to lower RESX instructions into gotos/calls/jumptables do
this at gimple and keep to RTL world only job of constructing landing
pads).  Then it might be nice to consider option to generate these in
middle-end instead of preserving whatever frontend builds.  The
receivers are different for C++ and Java EH (calling std::terminate or
abort) but otherwise there is nothing backend can't do.  Currently we
tend to end up with many std::terminate recerivers in single function
that we should unify anyway.

Honza
> 
> 
> r~

Re: can_throw_internal affected by inlining?

[Richard Henderson]

On 07/11/2009 10:59 AM, Jan Hubicka wrote:
> I would like to bring more of EH lowering to tree level (i.e. instead of
> relying on RTL to lower RESX instructions into gotos/calls/jumptables do
> this at gimple and keep to RTL world only job of constructing landing
> pads).

Sure.  Should probably turn EXC_PTR_EXPR and FILTER_EXPR into proper 
decls at the same time.


r~

Re: can_throw_internal affected by inlining?

[Jan Hubicka]

> On 07/11/2009 10:59 AM, Jan Hubicka wrote:
> >I would like to bring more of EH lowering to tree level (i.e. instead of
> >relying on RTL to lower RESX instructions into gotos/calls/jumptables do
> >this at gimple and keep to RTL world only job of constructing landing
> >pads).
> 
> Sure.  Should probably turn EXC_PTR_EXPR and FILTER_EXPR into proper 
> decls at the same time.

I was thinking of this, but I have problem with the fact that
EXC_PTR_EXPR/FILTER_EXPR do have value changed by runtime during EH
delievery.  It seems difficult to build SSA form for those since the
sets are implicit on the EH edges, not on statements.
If those two remains special for most of passes, I wonder if making them
DECLs is bringing any benefits...

Honza
> 
> 
> r~

Re: can_throw_internal affected by inlining?

[Richard Henderson]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

On 07/11/2009 05:59 AM, Jan Hubicka wrote:
>> Re: http://gcc.gnu.org/ml/gcc-patches/2009-03/msg01404.html
>>
>> Do you have test cases for this?
>>
>> Changing can_throw_internal/external to depend on whether or not future
>> inlining is possible looks *very* wrong to me.  Surely the only thing
>> that matters for new code that might appear "below" this position in the
>> tree is whether or not it might throw, and the only thing that changes
>> with inlining is increased knowledge of whether and how it throws.
>
> The problem here is fact that MUST_NOT_THROW region reachable only via
> runtime is handled completely via runtime, however MUST_NOT_THROW region
> reachable via RESX is eventually going to be handled by direct
> std::terminate call, since RESX will eventually get translated as direct
> goto to the MIST_NOT_THROW reciever.

I'm committing the following test case that displays the bug.  It does 
in fact pass with mainline, and does in fact fail with gcc 4.4.0.

I spent two days trying to come up with some cleaner way to fix this bug 
than the inlinable flag you pass around, but to no avail.  The only 
thing better I could think of is some global flag (or state variable) 
that indicates whether or not inlining is complete.  At least then we 
would not have to pass around that flag.  But I wouldn't want to 
introduce yet another boolean state variable; I'd much prefer all of the 
existing state variables we have be consolidated, and I can't justify 
spending the time on that just now.

> Well, we can either teach inlinable_call_p to handle your new indirect
> calls as "for sure uninlinable"

This is the approach I'll take.  I've already hacked on an extra bit in 
the gimple call subcode to indicate whether an indirect call is nothrow; 
I might as well add another bit to say an indirect call is noinline.


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 1092 bytes --]

--- testsuite/g++.dg/opt/eh4.C	(revision 149703)
+++ testsuite/g++.dg/opt/eh4.C	(local)
@@ -0,0 +1,59 @@
+// { dg-do run }
+// { dg-options "-O3" }
+
+// Make sure that the call to terminate within F2 is not eliminated
+// by incorrect MUST_NOT_THROW optimization.  Note that we expect F1
+// to be inlined into F2 in order to expose this case.
+
+#include <cstdlib>
+#include <exception>
+
+static volatile int zero = 0;
+
+// Note that we need F0 to not be marked nothrow, though we don't actually
+// want a throw to happen at runtime here.  The noinline tag is merely to
+// make sure the assembly in F0 is not unnecessarily complex.
+static void __attribute__((noinline)) f0()
+{
+  if (zero != 0)
+    throw 0;
+}
+
+struct S1
+{
+  S1() { }
+  ~S1() { f0(); }
+};
+
+static void f1()
+{
+  S1 s1;
+  throw 1;
+}
+
+struct S2
+{
+  S2() { }
+  ~S2() { f1(); }
+};
+
+static void __attribute__((noinline)) f2()
+{
+  S2 s2;
+  throw 2;
+}
+
+static void pass()
+{
+  exit (0);
+}
+
+int main()
+{
+  std::set_terminate (pass);
+  try {
+    f2();
+  } catch (...) {
+  }
+  abort ();
+}

Re: can_throw_internal affected by inlining?

[Jan Hubicka]

> I'm committing the following test case that displays the bug.  It does 
> in fact pass with mainline, and does in fact fail with gcc 4.4.0.
> 
> I spent two days trying to come up with some cleaner way to fix this bug 
> than the inlinable flag you pass around, but to no avail.  The only 
> thing better I could think of is some global flag (or state variable) 
> that indicates whether or not inlining is complete.  At least then we 
> would not have to pass around that flag.  But I wouldn't want to 
> introduce yet another boolean state variable; I'd much prefer all of the 
> existing state variables we have be consolidated, and I can't justify 
> spending the time on that just now.

There is cfun->after_inlinng that says you this info on per-function
basis.
I will run the tests on C++ benchmarks this weekend whether simply
assuming all calls to be possibly inlined throws before this flag is set
is producing noticeable degradation in memuse/compile time.

Honza
> 
> >Well, we can either teach inlinable_call_p to handle your new indirect
> >calls as "for sure uninlinable"
> 
> This is the approach I'll take.  I've already hacked on an extra bit in 
> the gimple call subcode to indicate whether an indirect call is nothrow; 
> I might as well add another bit to say an indirect call is noinline.

Yep, it seems sort of resonable too.

Honza