Re: allow EH to escape from GIMPLE_EH_ELSE ELSE block

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Jul 2, 2019 at 10:51 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jul  1, 2019, Richard Biener <richard.guenther@gmail.com> wrote:
>
> > Oh, I see.  The GIMPLE frontend is also missing parsing support
> > for the EH stmt kinds, it might have been possible to build a testcase
> > with that I guess (yeah, only a very slight hint ... ;))
>
> Ooh, beautiful!  Is that in the trunk already?  I couldn't immediately
> find it, but...  it's almost early again ;-) and I haven't tried really
> hard.  Pointers to files to change and testcases to modify are welcome
> either way.

Yeah, it's on trunk.  The parser is ontop of the C frontend and resides
in gcc/c/gimple-parser.c while testcases are in gcc.dg/gimplefe-*.c
There are also quite a few unit tests for individual passes now,
for example the just contributed gcc.dg/tree-ssa/ssa-dom-cse-8.c

The parsing is incomplete, there's no support for parsing try/catch/finally
but the FE should be able to handle both high and low GIMPLE as well
as GIMPLE with a CFG and SSA.  Not sure if there are any/many
high-gimple testcases.  Also populating on-the-side info (like EH tables
or associating stmts with it) isn't implemented but if you do high
GIMPLE that doesn't exist besides stuff created by the frontends
(so specific catches would need quite some support I guess).

>
>
> There's a bit of a catch (no pun intended) that TRY_FINALLY (without
> GIMPLE_EH_ELSE) lowering involves lang_hooks.eh_protect_cleanup_actions;
> propagating exceptions out of cleanups, in either EH_ELSE branch, should
> probably pay attention to that hook as well, but I haven't touched it
> because the current code suits us.  I mention it because I don't know
> how the gimple frontend sets up this hook, even though it probably
> wouldn't matter since you'd be taking the EH_ELSE branch rather than the
> eh_protect_cleanup_actions one in honor_protect_cleanup_actions.
>
>
> > Can you share a .gimple dump that has the issue?
>
> Attached.  Look for t.finalize_spec () and t (), both have
> <<<else_eh_exit>>>s.
>
> (ugh, else_eh* vs *EH_ELSE; could we make it ehlse? ;-) / 2

I guess dumping it as

  finally
    {
         if (normal exit)
           ...
         else if (eh exit)
           ...
    }

would work as well?   Or even

  try
    {
    }
  eh_finally
    {
    }
  finally
    {
    }

which I would find more natural (even in the trees...).

>
>
> > So the testcase "survives" CFG construction but then crashes during
> > the first CFG cleanup or only after some EH optimization?
>
> I dug up and found out what enabled the crash was a patch that Eric
> Botcazou tells me he will contribute soon.  The bit that brought the
> crash about was the following patchlet extract.  Without this patchlet,
> the CFG is wrong, but I haven't observed any crash.  The reason it's
> wrong is that the block has an abnormal edge to itself, which can be
> observed in the optimized dump.  Exceptions in the cleanup portion of
> the TRY_FINALLY should propagate to enclosing handlers, and they do when
> EH_ELSE is not used.  With EH_ELSE, however, we get the unintended EH
> loop as soon as EH lowering.

Ah, that's of course bad.

Thanks,
Richard.

>
> t.finalize_spec ()
> Eh tree:
>    2 cleanup land:{2,<L9>}
>    1 try land:{1,<L4>} catch:{&OTHERS}
> [...]
> ;;   basic block 10, loop depth 1
> ;;    pred:       8
> ;;                10
> <L9>: [LP 2]
>   [LP 2] .gnat_end_handler (EXPTR_9);
> ;;    succ:       10
> ;;                11
>
>
>
> diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
> index 5a93830..597a488 100644
> --- a/gcc/gimple-ssa-store-merging.c
> +++ b/gcc/gimple-ssa-store-merging.c
> @@ -159,6 +159,7 @@
>  #include "gimple-fold.h"
>  #include "stor-layout.h"
>  #include "timevar.h"
> +#include "cfgcleanup.h"
>  #include "tree-cfg.h"
>  #include "tree-eh.h"
>  #include "target.h"
> @@ -4671,6 +4672,14 @@ pass_store_merging::execute (function *fun)
>    basic_block bb;
>    hash_set<gimple *> orig_stmts;
>
> +  if (cfun->can_throw_non_call_exceptions && cfun->eh)
> +    {
> +      maybe_remove_unreachable_handlers ();
> +      bool changed = unsplit_all_eh ();
> +      if (changed)
> +       delete_unreachable_blocks ();
> +    }
> +
>    calculate_dominance_info (CDI_DOMINATORS);
>
>    FOR_EACH_BB_FN (bb, fun)
> diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
> index 23c56b5..f547d98 100644
> --- a/gcc/tree-eh.c
> +++ b/gcc/tree-eh.c
> @@ -4185,7 +4185,7 @@ unsplit_eh (eh_landing_pad lp)
>
>  /* Examine each landing pad block and see if it matches unsplit_eh.  */
>
> -static bool
> +bool
>  unsplit_all_eh (void)
>  {
>    bool changed = false;
> diff --git a/gcc/tree-eh.h b/gcc/tree-eh.h
> index a588c10..3998a97 100644
> --- a/gcc/tree-eh.h
> +++ b/gcc/tree-eh.h
> @@ -52,5 +52,6 @@ extern bool maybe_duplicate_eh_stmt (gimple *, gimple *);
>  extern void maybe_remove_unreachable_handlers (void);
>  extern bool verify_eh_edges (gimple *);
>  extern bool verify_eh_dispatch_edge (geh_dispatch *);
> +extern bool unsplit_all_eh (void);
>
>  #endif /* GCC_TREE_EH_H */
>
>
>
>
>
>
> --
> Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> Be the change, be Free!                 FSF Latin America board member
> GNU Toolchain Engineer                        Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara