On Fri, Jun 28, 2019 at 11:43 AM Alexandre Oliva <ol...@adacore.com> wrote:
>
> On Jun 27, 2019, Richard Biener <richard.guent...@gmail.com> wrote:
>
> > On Thu, Jun 27, 2019 at 10:18 AM Alexandre Oliva <ol...@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, ¤t_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, ¤t_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
