tree: Fix up try_catch_may_fallthru [PR112619]

Hi!

The following testcase ICEs with -std=c++98 since r14-5086 because
block_may_fallthru is called on a TRY_CATCH_EXPR whose second operand
is a MODIFY_EXPR rather than STATEMENT_LIST, which try_catch_may_fallthru
apparently expects.
I've been wondering whether that isn't some kind of FE bug and whether
there isn't some unwritten rule that second operand of TRY_CATCH_EXPR
must be a STATEMENT_LIST, but then I tried

	Jakub

On Wed, 22 Nov 2023, Jakub Jelinek wrote:

> Hi!
> 
> The following testcase ICEs with -std=c++98 since r14-5086 because
> block_may_fallthru is called on a TRY_CATCH_EXPR whose second operand
> is a MODIFY_EXPR rather than STATEMENT_LIST, which try_catch_may_fallthru
> apparently expects.
> I've been wondering whether that isn't some kind of FE bug and whether
> there isn't some unwritten rule that second operand of TRY_CATCH_EXPR
> must be a STATEMENT_LIST, but then I tried
> --- gcc/gimplify.cc	2023-07-19 14:23:42.409875238 +0200
> +++ gcc/gimplify.cc	2023-11-22 11:07:50.511000206 +0100
> @@ -16730,6 +16730,10 @@ gimplify_expr (tree *expr_p, gimple_seq
>  	       Note that this only affects the destructor calls in FINALLY/CATCH
>  	       block, and will automatically reset to its original value by the
>  	       end of gimplify_expr.  */
> +            if (TREE_CODE (*expr_p) == TRY_CATCH_EXPR
> +		&& TREE_OPERAND (*expr_p, 1)
> +		&& TREE_CODE (TREE_OPERAND (*expr_p, 1)) != STATEMENT_LIST)
> +	      gcc_unreachable ();
>  	    input_location = UNKNOWN_LOCATION;
>  	    eval = cleanup = NULL;
>  	    gimplify_and_add (TREE_OPERAND (*expr_p, 0), &eval);
> hack in gcc 13 and triggered on hundreds of tests there within just 5
> seconds of running make check-g++ -j32 (and in cases I looked at had nothing
> to do with the r14-5086 backports), so I believe this is just bad
> assumption on the try_catch_may_fallthru side, gimplify.cc certainly doesn't
> care, it just calls gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);
> on it.  So, IMHO non-STATEMENT_LIST in the second operand is equivalent to
> a STATEMENT_LIST containing a single statement.

Did you check if there's ever a CATCH_EXPR or EH_FILTER_EXPR not wrapped
inside a STATEMENT_LIST?  That is, does

 if (TREE_CODE (TREE_OPERAND (stmt, 1)) != STATEMENT_LIST)
   {
     gcc_checking_assert (code != CATCH_EXPR && code != EH_FILTER_EXPR);
     return false;
   }

work?

> Unfortunately, I don't see an easy way to create an artificial tree iterator
> from just a single tree statement, so the patch duplicates what the loops
> later do (after all, it is very simple, just didn't want to duplicate
> also the large comments explaning it, so the 3 See below. comments).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> Shall it go to branches as well given that r14-5086 has been backported
> to those branches as well?
> 
> 2023-11-22  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/112619
> 	* tree.cc (try_catch_may_fallthru): If second operand of
> 	TRY_CATCH_EXPR is not a STATEMENT_LIST, handle it as if it was a
> 	STATEMENT_LIST containing a single statement.
> 
> 	* g++.dg/eh/pr112619.C: New test.
> 
> --- gcc/tree.cc.jj	2023-11-14 09:24:28.436530018 +0100
> +++ gcc/tree.cc	2023-11-21 19:19:19.384347469 +0100
> @@ -12573,6 +12573,24 @@ try_catch_may_fallthru (const_tree stmt)
>    if (block_may_fallthru (TREE_OPERAND (stmt, 0)))
>      return true;
>  
> +  switch (TREE_CODE (TREE_OPERAND (stmt, 1)))
> +    {
> +    case CATCH_EXPR:
> +      /* See below.  */
> +      return block_may_fallthru (CATCH_BODY (TREE_OPERAND (stmt, 1)));
> +
> +    case EH_FILTER_EXPR:
> +      /* See below.  */
> +      return block_may_fallthru (EH_FILTER_FAILURE (TREE_OPERAND (stmt, 1)));
> +
> +    case STATEMENT_LIST:
> +      break;
> +
> +    default:
> +      /* See below.  */
> +      return false;
> +    }
> +
>    i = tsi_start (TREE_OPERAND (stmt, 1));
>    switch (TREE_CODE (tsi_stmt (i)))
>      {
> --- gcc/testsuite/g++.dg/eh/pr112619.C.jj	2023-11-21 19:22:47.437439283 +0100
> +++ gcc/testsuite/g++.dg/eh/pr112619.C	2023-11-21 19:22:24.887754376 +0100
> @@ -0,0 +1,15 @@
> +// PR c++/112619
> +// { dg-do compile }
> +
> +struct S { S (); ~S (); };
> +
> +S
> +foo (int a, int b)
> +{
> +  if (a || b)
> +    {
> +      S s;
> +      return s;
> +    }
> +  return S ();
> +}
> 
> 	Jakub
> 
>

On Wed, Nov 22, 2023 at 11:32:10AM +0000, Richard Biener wrote:
> > hack in gcc 13 and triggered on hundreds of tests there within just 5
> > seconds of running make check-g++ -j32 (and in cases I looked at had nothing
> > to do with the r14-5086 backports), so I believe this is just bad
> > assumption on the try_catch_may_fallthru side, gimplify.cc certainly doesn't
> > care, it just calls gimplify_and_add (TREE_OPERAND (*expr_p, 1), &cleanup);
> > on it.  So, IMHO non-STATEMENT_LIST in the second operand is equivalent to
> > a STATEMENT_LIST containing a single statement.
> 
> Did you check if there's ever a CATCH_EXPR or EH_FILTER_EXPR not wrapped
> inside a STATEMENT_LIST?  That is, does
> 
>  if (TREE_CODE (TREE_OPERAND (stmt, 1)) != STATEMENT_LIST)
>    {
>      gcc_checking_assert (code != CATCH_EXPR && code != EH_FILTER_EXPR);
>      return false;
>    }
> 
> work?

Looking at a trivial example
void bar ();
void
foo (void)
{
  try { bar (); } catch (int) {}
}
it seems it is even more complicated, because what e.g. the gimplification
sees is not TRY_CATCH_EXPR with CATCH_EXPR second operand, but
TRY_BLOCK with HANDLER second operand (note, certainly not wrapped in a
STATEMENT_LIST, one would need another catch (long) {} for it after it),
C++ FE specific trees.
And cp_gimplify_expr then on the fly turns the TRY_BLOCK into TRY_CATCH_EXPR
(in genericize_try_block) and HANDLER into CATCH_EXPR
(genericize_catch_block).
When gimplifying EH_SPEC_BLOCK in genericize_eh_spec_block it even
creates TRY_CATCH_EXPR with genericize_eh_spec_block -> build_gimple_eh_filter_tree
if even creates TRY_CATCH_EXPR with EH_FILTER_EXPR as its second operand
(without intervening STATEMENT_LIST).

So, I believe the patch is correct but for C++ it might be hard to see it
actually trigger because most often one will see the C++ FE specific trees
of TRY_BLOCK (with HANDLER) and EH_SPEC_BLOCK instead.
So, I wonder why cxx_block_may_fallthru doesn't handle TRY_BLOCK and
EH_SPEC_BLOCK as well.  Given the genericization, I think
TRY_BLOCK should be handled similarly to TRY_CATCH_EXPR in tree.cc,
if second operand is HANDLER or STATEMENT_LIST starting with HANDLER,
check if any of the handler bodies can fall thru, dunno if TRY_BLOCK without
HANDLERs is possible, and for EH_SPEC_BLOCK see if the failure can fall
through.

	Jakub

On Wed, Nov 22, 2023 at 01:06:28PM +0100, Jakub Jelinek wrote:
> Looking at a trivial example
> void bar ();
> void
> foo (void)
> {
>   try { bar (); } catch (int) {}
> }
> it seems it is even more complicated, because what e.g. the gimplification
> sees is not TRY_CATCH_EXPR with CATCH_EXPR second operand, but
> TRY_BLOCK with HANDLER second operand (note, certainly not wrapped in a
> STATEMENT_LIST, one would need another catch (long) {} for it after it),
> C++ FE specific trees.
> And cp_gimplify_expr then on the fly turns the TRY_BLOCK into TRY_CATCH_EXPR
> (in genericize_try_block) and HANDLER into CATCH_EXPR
> (genericize_catch_block).
> When gimplifying EH_SPEC_BLOCK in genericize_eh_spec_block it even
> creates TRY_CATCH_EXPR with genericize_eh_spec_block -> build_gimple_eh_filter_tree
> if even creates TRY_CATCH_EXPR with EH_FILTER_EXPR as its second operand
> (without intervening STATEMENT_LIST).

Ah, and the difference between the above where TRY_BLOCK is turned into
TRY_CATCH_EXPR and HANDLER into CATCH_EXPR vs. the ICE on the testcase from
the PR is that in that case it isn't TRY_BLOCK, but CLEANUP_STMT which is
not changed during gimplification but already during cp generication.
So, pedantically perhaps just assuming TRY_CATCH_EXPR where second argument
is not STATEMENT_LIST to be the CATCH_EXPR/EH_FILTER_EXPR case could work
for C++, but there are other FEs and it would be fragile (and weird, given
that STATEMENT_LIST with single stmt in it vs. that stmt ought to be
generally interchangeable).

Plus of course question whether we want to handle TRY_BLOCK/EH_SPEC_BLOCK in
cxx_block_may_fallthru in addition to that remains (it apparently already
handles CLEANUP_STMT, but strangely just the try/finally special case of it
- I'd assume the CLEANUP_EH_ONLY case would be
(block_may_fallthru (CLEANUP_BODY (stmt))
 || block_may_fallthru (CLEANUP_EXPR (stmt)))
because if the body can fallthru, everything can, and if there is an
exception and cleanup can fallthru, then it could fallthru as well).

	Jakub

On Wed, Nov 22, 2023 at 01:21:12PM +0100, Jakub Jelinek wrote:
> So, pedantically perhaps just assuming TRY_CATCH_EXPR where second argument
> is not STATEMENT_LIST to be the CATCH_EXPR/EH_FILTER_EXPR case could work
> for C++, but there are other FEs and it would be fragile (and weird, given
> that STATEMENT_LIST with single stmt in it vs. that stmt ought to be
> generally interchangeable).

Looking at other FE, e.g. go/go-gcc.cc clearly has:
    stat_tree = build2_loc(location.gcc_location(), TRY_CATCH_EXPR,
                           void_type_node, stat_tree,
                           build2_loc(location.gcc_location(), CATCH_EXPR,
                                      void_type_node, NULL, except_tree));
so CATCH_EXPR is immediately the second operand of TRY_CATCH_EXPR.
d/toir.cc has:
    /* Back-end expects all catches in a TRY_CATCH_EXPR to be enclosed in a
       statement list, however pop_stmt_list may optimize away the list
       if there is only a single catch to push.  */
    if (TREE_CODE (catches) != STATEMENT_LIST)
      {
        tree stmt_list = alloc_stmt_list ();
        append_to_statement_list_force (catches, &stmt_list);
        catches = stmt_list;
      }

    add_stmt (build2 (TRY_CATCH_EXPR, void_type_node, trybody, catches));
so I assume it run into the try_catch_may_fallthru issue (because gimplifier
clearly doesn't require that).
rust/rust-gcc.cc copies go-gcc.cc and also creates CATCH_EXPR directly in
TRY_CATCH_EXPR's operand.

Note, the only time one runs into the ICE is when the first operand (i.e.
try body) doesn't fall thru, otherwise the function returns true early.

	Jakub

On Wed, 22 Nov 2023, Jakub Jelinek wrote:

> On Wed, Nov 22, 2023 at 01:21:12PM +0100, Jakub Jelinek wrote:
> > So, pedantically perhaps just assuming TRY_CATCH_EXPR where second argument
> > is not STATEMENT_LIST to be the CATCH_EXPR/EH_FILTER_EXPR case could work
> > for C++, but there are other FEs and it would be fragile (and weird, given
> > that STATEMENT_LIST with single stmt in it vs. that stmt ought to be
> > generally interchangeable).
> 
> Looking at other FE, e.g. go/go-gcc.cc clearly has:
>     stat_tree = build2_loc(location.gcc_location(), TRY_CATCH_EXPR,
>                            void_type_node, stat_tree,
>                            build2_loc(location.gcc_location(), CATCH_EXPR,
>                                       void_type_node, NULL, except_tree));
> so CATCH_EXPR is immediately the second operand of TRY_CATCH_EXPR.
> d/toir.cc has:
>     /* Back-end expects all catches in a TRY_CATCH_EXPR to be enclosed in a
>        statement list, however pop_stmt_list may optimize away the list
>        if there is only a single catch to push.  */
>     if (TREE_CODE (catches) != STATEMENT_LIST)
>       {
>         tree stmt_list = alloc_stmt_list ();
>         append_to_statement_list_force (catches, &stmt_list);
>         catches = stmt_list;
>       }
> 
>     add_stmt (build2 (TRY_CATCH_EXPR, void_type_node, trybody, catches));
> so I assume it run into the try_catch_may_fallthru issue (because gimplifier
> clearly doesn't require that).
> rust/rust-gcc.cc copies go-gcc.cc and also creates CATCH_EXPR directly in
> TRY_CATCH_EXPR's operand.
> 
> Note, the only time one runs into the ICE is when the first operand (i.e.
> try body) doesn't fall thru, otherwise the function returns true early.

OK, I think this suggests we should be more forgiving here, meaning
your patch is OK.  Unless Jason has any additional comments today.

Thanks,
Richard.