Re: [patch] Fix ICE during inlining with SJLJ

[Richard Guenther <richard dot guenther at gmail dot com>]

On Wed, Mar 28, 2012 at 4:42 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
> Hi,
>
> this is a regression present on the mainline and 4.7 branch for platforms using
> SJLJ exceptions, e.g. the ARM. ?The scenario is as follows: the main procedure
> calls My_Iterators.Current which has a pragma Inline on it. ?The procedure
> also has exceptions handlers so there is an abnormal edge from the BB
> containing the call to My_Iterators.Current (which is therefore the last
> statement in the BB) to the setjmp dispatcher.
>
> The local-pure-const pass computes that My_Iterators.Current is pure and may
> not terminate (DECL_PURE_P && DECL_LOOPING_CONST_OR_PURE_P) because it is pure
> and contains a call to gnat_check, which is no-return, and
>
> ? ? ?state_from_flags (&call_state, &call_looping, flags,
> ? ? ? ? ? ? ? ? ? ? ? ?((flags & (ECF_NORETURN | ECF_NOTHROW))
> ? ? ? ? ? ? ? ? ? ? ? ? == (ECF_NORETURN | ECF_NOTHROW))
> ? ? ? ? ? ? ? ? ? ? ? ?|| (!flag_exceptions && (flags & ECF_NORETURN)));
>
> considers that, with !flag_exceptions, a no-return function call can really
> never return, including by exceptional means.
>
> The early SRA pass then inserts a statement in the procedure right after the
> call to My_Iterators.Current, so at the end of the BB, because stmt_ends_bb_p
> has returned false on the call, in turn because the statement is ECF_PURE and
> is_ctrl_altering_stmt has
>
> ? ? ? ?/* A non-pure/const call alters flow control if the current
> ? ? ? ? ? function has nonlocal labels. ?*/
> ? ? ? ?if (!(flags & (ECF_CONST | ECF_PURE | ECF_LEAF))
> ? ? ? ? ? ?&& cfun->has_nonlocal_label)
> ? ? ? ? ?return true;
>
> i.e. doesn't return true if ECF_PURE. ?As a consequence, the abnormal edge from
> the BB to the setjmp receiver is deleted.
>
> When My_Iterators.Current gets inlined into P, the call to gnat_check is copied
> into it and, since stmt_can_make_abnormal_goto returns true on it, a new
> abnormal edge to the setjmp dispatcher is created. ?The compiler aborts in
> update_ssa_across_abnormal_edges because it cannot find the original abnormal
> edge that it needs to use in order to complete the new one.
>
>
> The cause is the discrepancy between local-pure-const, is_ctrl_altering_stmt
> and stmt_can_make_abnormal_goto (the latter two themselves disagreeing) as to
> when a call can return exceptionally/make an abnormal goto. ?It's clear that
>
> ?(!flag_exceptions && (flags & ECF_NORETURN))
>
> overlooks the __builtin_setjmp/__builtin_longjmp constructs so is optimistic at
> best. ?But we cannot really do better in local-pure-const, short of removing
> the condition entirely.
>
> The interesting thing is that stmt_can_make_abnormal_goto, unlike the related
> is_ctrl_altering_stmt, doesn't consider that a mere ECF_PURE can change the
> property of a call wrt control flow:
>
> bool
> stmt_can_make_abnormal_goto (gimple t)
> {
> ?if (computed_goto_p (t))
> ? ?return true;
> ?if (is_gimple_call (t))
> ? ?return (gimple_has_side_effects (t) && cfun->has_nonlocal_label
> ? ? ? ? ? ?&& !(gimple_call_flags (t) & ECF_LEAF));
> ?return false;
> }
>
> because it tests gimple_has_side_effects, which is still true if the call may
> not return:
>
> ?if (is_gimple_call (s))
> ? ?{
> ? ? ?int flags = gimple_call_flags (s);
>
> ? ? ?/* An infinite loop is considered a side effect. ?*/
> ? ? ?if (!(flags & (ECF_CONST | ECF_PURE))
> ? ? ? ? ?|| (flags & ECF_LOOPING_CONST_OR_PURE))
> ? ? ? ?return true;
>
> ? ? ?return false;
> ? ?}
>
>
> So, in the end, a reasonable fix might be to unify the condition used by
> is_ctrl_altering_stmt and stmt_can_make_abnormal_goto, by using the most
> conservative one (the latter), which happens to also cover the optimistic
> semantics used by local-pure-const.
>
> Tested on x86_64-suse-linux, OK for mainline and 4.7 branch?

Thanks for the detailed analysis.  The patch indeed looks ok and makes
things easier to understand even.

Thus, ok.

Thanks,
Richard.

>
> 2012-03-28 ?Eric Botcazou ?<ebotcazou@adacore.com>
>
> ? ? ? ?* tree-cfg.c (call_can_make_abnormal_goto): New predicate.
> ? ? ? ?(stmt_can_make_abnormal_goto): Use it.
> ? ? ? ?(is_ctrl_altering_stmt): Likewise.
>
>
> 2012-03-28 ?Eric Botcazou ?<ebotcazou@adacore.com>
>
> ? ? ? ?* gnat.dg/controlled6.adb: New test.
> ? ? ? ?* gnat.dg/controlled6_pkg.ads: New helper.
> ? ? ? ?* gnat.dg/controlled6_pkg-iterators.ad[sb]: Likewise.
>
>
> --
> Eric Botcazou