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.
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
