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