This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: allow EH to escape from GIMPLE_EH_ELSE ELSE block


On Fri, Jun 28, 2019 at 11:43 AM Alexandre Oliva <oliva@adacore.com> wrote:
>
> On Jun 27, 2019, Richard Biener <richard.guenther@gmail.com> wrote:
>
> > On Thu, Jun 27, 2019 at 10:18 AM Alexandre Oliva <oliva@adacore.com> wrote:
> >>
> >> The only preexisting use of GIMPLE_EH_ELSE, for transactional memory
> >> commits, did not allow exceptions to escape from the ELSE path.  The
> >> trick it uses to allow the ELSE path to see the propagating exception
> >> does not work very well if the exception cleanup raises further
> >> exceptions: the ELSE block is configured to handle exceptions in
> >> itself.  This confuses the heck out of CFG and EH cleanups.
> >>
> >> Basing the lowering context for the ELSE block on outer_state, rather
> >> than this_state, gets us the expected enclosing handler.
> >>
> >> Regstrapped on x86_64-linux-gnu.  Ok to install?
>
> > Testcase?
>
> None possible with the current codebase, I'm afraid.  Nothing creates
> EH_ELSE yet, and nothing creates GIMPLE_EH_ELSE with
> exception-generating cleanups.

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 ... ;)).  Can you share
a .gimple dump that has the issue?  So the testcase "survives"
CFG construction but then crashes during the first CFG cleanup or
only after some EH optimization?

Thanks,
Richard.

> The following patch, an extract of a larger patch that is still under
> internal review (and pending approval of the EH_ELSE-related changes
> ;-), is minimized into pointlessness so as to just exercise the
> GIMPLE_EH_ELSE lowering issue.
>
> IIRC the problem is NOT immediately triggered in a bootstrap, it
> requires more elaborate EH scenarios to trigger it: without the
> GIMPLE_EH_ELSE lowering patch, a few libadalang units failed to compile
> within delete_unreachable_blocks, using a compiler built with the patch
> below and the patch that introduced EH_ELSE that I posted yesterday.
>
> At first, I suspected GIMPLE_EH_ELSE had bit-rotted, as it doesn't seem
> to get much use, but the problem turned out to be caused by the
> nonsensical CFG resulting from the GIMPLE_EH_ELSE lowering, that breaks
> EH CFG optimizations and end up corrupting the CFG.  IIRC it was
> unsplit_eh that mishandled self edges that arise after a bunch of other
> valid transformations.  I cannot observe the crash with trunk any more,
> but the EH tree is visibly wrong in tree dumps with eh and blocks,
> compiling such a simple testcase as this (with a patched compiler as
> described above):
>
> -- compile with -Ofast -g -c
> with GNAT.Semaphores; use GNAT.Semaphores;
> package T is
>    subtype Mutual_Exclusion is Binary_Semaphore
>      (Initially_Available => True,
>       Ceiling             => Default_Ceiling);
>    Lock : aliased Mutual_Exclusion;
> end T;
>
>
> Self edges end up arising because of the way GIMPLE_EH_ELSE was lowered:
> the exceptional cleanup was lowered as if within the TRY_FINALLY_EXPR
> TRY block, whose exceptions get handled by the exceptional cleanup, but
> both cleanup paths should be lowered as if after the TRY_FINALLY_EXPR,
> so that an enclosing EH region is used should they throw exceptions.
>
> The current lowering made sense for cleanups that couldn't throw: no EH
> edge would come out of the lowered exceptional cleanup block.  However,
> EH_ELSE-using code below breaks that assumption.  Fortunately, the
> assumption was not essential for GIMPLE_EH_ELSE: the lowering code just
> needed this small adjustment to make room for relaxing the requirement
> that the cleanups couldn't throw and restoring CFG EH edges that match
> what we get without the patch below.
>
>
> --- gcc/ada/gcc-interface/trans.c
> +++ gcc/ada/gcc-interface/trans.c
> @@ -6249,7 +6249,7 @@ Exception_Handler_to_gnu_gcc (Node_Id gnat_node)
>    if (stmt_list_cannot_alter_control_flow_p (Statements (gnat_node)))
>      add_stmt_with_node (stmt, gnat_node);
>    else
> -    add_cleanup (stmt, gnat_node);
> +    add_cleanup (build2 (EH_ELSE, void_type_node, stmt, stmt), gnat_node);
>
>    gnat_poplevel ();
>
> @@ -9066,7 +9081,29 @@ add_cleanup (tree gnu_cleanup, Node_Id g
>  {
>    if (Present (gnat_node))
>      set_expr_location_from_node (gnu_cleanup, gnat_node, true);
> -  append_to_statement_list (gnu_cleanup, &current_stmt_group->cleanups);
> +  /* An EH_ELSE must be by itself, and that's all we need when we use
> +     it.  The assert below makes sure that is so.  Should we ever need
> +     more than that, we could combine EH_ELSEs, and copy non-EH_ELSE
> +     stmts into both cleanup paths of an EH_ELSE, being careful to
> +     make sure the exceptional path doesn't throw.  */
> +  if (TREE_CODE (gnu_cleanup) == EH_ELSE)
> +    {
> +      gcc_assert (!current_stmt_group->cleanups);
> +      if (Present (gnat_node))
> +       {
> +         set_expr_location_from_node (TREE_OPERAND (gnu_cleanup, 0),
> +                                      gnat_node, true);
> +         set_expr_location_from_node (TREE_OPERAND (gnu_cleanup, 1),
> +                                      gnat_node, true);
> +       }
> +      current_stmt_group->cleanups = gnu_cleanup;
> +    }
> +  else
> +    {
> +      gcc_assert (!current_stmt_group->cleanups
> +                 || TREE_CODE (current_stmt_group->cleanups) != EH_ELSE);
> +      append_to_statement_list (gnu_cleanup, &current_stmt_group->cleanups);
> +    }
>  }
>
>  /* Set the BLOCK node corresponding to the current code group to GNU_BLOCK.  */
>
>
> --
> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]