On Thu, Nov 14, 2013 at 9:58 PM, Jeff Law <l...@redhat.com> wrote:
>
> This patch fixes two issues, the most important issue is the related to the
> Ada build failures on the trunk.
>
> When non-call-exceptions is on, most memory references potentially throw.
> As a result those statements end basic blocks. This causes
> checking failures when the __builtin_trap is placed immediately after the
> memory reference because we find the memory reference in the middle of a
> basic block.
>
> While I think we could support this with some more work, it just doesn't
> seem worth the effort. It certainly isn't something that's occurring with
> any regularity AFAICT when buliding the Ada compiler/runtime system.
>
> It's easiest to just disallow optimization when the statement that triggers
> undefined behaviour ends a block -- with the exception of GIMPLE_RETURN.
Hmm, don't you simply want to look for abnormal or eh outgoing edges?
That catches all stmt-ends-bb and doesn't catch GIMPLE_RETURN.
Richard.
> That captures the key issue, namely that the code is not currently prepared
> to have the trap in a separate block from the statement triggering undefined
> behaviour.
>
> I didn't make a testcase for this failure because it triggers during
> bootstrapping Ada and my Ada-fu has severely eroded since the 80s when I was
> forced to learn Ada.
>
> The second issue is when a block has outgoing abnormal edges, out of an
> abundance of caution, we should simply not apply the optimization. That may
> be a bit too cautious, but it's clearly the safe thing to do. I don't have
> a testcase for this.
>
> In addition to the usual bootstrap & regression test on
> x86_64-unknown-linux-gnu, this patch fixes the Ada bootstrap on
> x86_64-unknown-linux-gnu and shows no regressions in the Ada test suite when
> compared to a compiler with the optimization totally disabled. ie, a poor
> mans regression test for Ada given that Ada wasn't bootstrapping w/o this
> patch.
>
> Applied to the trunk.
>
> Jeff
>
> ps. Obviously the next thing to verify is that Ada is bootstrapping on
> Itanic, but there's other potential issues on Itanic that might get in the
> way.
>
>
> * basic-block.h (has_abnormal_outgoing_edge_p): Moved here from...
> * tree-inline.c (has_abnormal_outgoing_edge_p): Remove.
> * gimple-ssa-isolate-paths.c: Include tree-cfg.h.
> (find_implicit_erroneous_behaviour): If a block has abnormal
> outgoing
> edges, then ignore it. If the statement exhibiting erroneous
> behaviour ends basic blocks, with the exception of GIMPLE_RETURNs,
> then we can not optimize.
> (find_explicit_erroneous_behaviour): Likewise.
>
>
> diff --git a/gcc/basic-block.h b/gcc/basic-block.h
> index 9c28f14..b7e3b50 100644
> --- a/gcc/basic-block.h
> +++ b/gcc/basic-block.h
> @@ -1008,4 +1008,19 @@ inverse_probability (int prob1)
> check_probability (prob1);
> return REG_BR_PROB_BASE - prob1;
> }
> +
> +/* Return true if BB has at least one abnormal outgoing edge. */
> +
> +static inline bool
> +has_abnormal_outgoing_edge_p (basic_block bb)
> +{
> + edge e;
> + edge_iterator ei;
> +
> + FOR_EACH_EDGE (e, ei, bb->succs)
> + if (e->flags & EDGE_ABNORMAL)
> + return true;
> +
> + return false;
> +}
> #endif /* GCC_BASIC_BLOCK_H */
> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
> index 108b98e..66c13f4 100644
> --- a/gcc/gimple-ssa-isolate-paths.c
> +++ b/gcc/gimple-ssa-isolate-paths.c
> @@ -37,6 +37,7 @@ along with GCC; see the file COPYING3. If not see
> #include "ssa-iterators.h"
> #include "cfgloop.h"
> #include "tree-pass.h"
> +#include "tree-cfg.h"
>
>
> static bool cfg_altered;
> @@ -215,6 +216,17 @@ find_implicit_erroneous_behaviour (void)
> {
> gimple_stmt_iterator si;
>
> + /* Out of an abundance of caution, do not isolate paths to a
> + block where the block has any abnormal outgoing edges.
> +
> + We might be able to relax this in the future. We have to detect
> + when we have to split the block with the NULL dereference and
> + the trap we insert. We have to preserve abnormal edges out
> + of the isolated block which in turn means updating PHIs at
> + the targets of those abnormal outgoing edges. */
> + if (has_abnormal_outgoing_edge_p (bb))
> + continue;
> +
> /* First look for a PHI which sets a pointer to NULL and which
> is then dereferenced within BB. This is somewhat overly
> conservative, but probably catches most of the interesting
> @@ -256,8 +268,15 @@ find_implicit_erroneous_behaviour (void)
> {
> /* We only care about uses in BB. Catching cases in
> in other blocks would require more complex path
> - isolation code. */
> - if (gimple_bb (use_stmt) != bb)
> + isolation code.
> +
> + If the statement must end a block and is not a
> + GIMPLE_RETURN, then additional work would be
> + necessary to isolate the path. Just punt it for
> + now. */
> + if (gimple_bb (use_stmt) != bb
> + || (stmt_ends_bb_p (use_stmt)
> + && gimple_code (use_stmt) != GIMPLE_RETURN))
> continue;
>
> if (infer_nonnull_range (use_stmt, lhs))
> @@ -289,6 +308,17 @@ find_explicit_erroneous_behaviour (void)
> {
> gimple_stmt_iterator si;
>
> + /* Out of an abundance of caution, do not isolate paths to a
> + block where the block has any abnormal outgoing edges.
> +
> + We might be able to relax this in the future. We have to detect
> + when we have to split the block with the NULL dereference and
> + the trap we insert. We have to preserve abnormal edges out
> + of the isolated block which in turn means updating PHIs at
> + the targets of those abnormal outgoing edges. */
> + if (has_abnormal_outgoing_edge_p (bb))
> + continue;
> +
> /* Now look at the statements in the block and see if any of
> them explicitly dereference a NULL pointer. This happens
> because of jump threading and constant propagation. */
> @@ -299,7 +329,8 @@ find_explicit_erroneous_behaviour (void)
> /* By passing null_pointer_node, we can use infer_nonnull_range
> to detect explicit NULL pointer dereferences and other uses
> where a non-NULL value is required. */
> - if (infer_nonnull_range (stmt, null_pointer_node))
> + if ((!stmt_ends_bb_p (stmt) || gimple_code (stmt) ==
> GIMPLE_RETURN)
> + && infer_nonnull_range (stmt, null_pointer_node))
> {
> insert_trap_and_remove_trailing_statements (&si,
>
> null_pointer_node);
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index fe5c0cb..08c4541 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4504,21 +4504,6 @@ fold_marked_statements (int first, struct
> pointer_set_t *statements)
> }
> }
>
> -/* Return true if BB has at least one abnormal outgoing edge. */
> -
> -static inline bool
> -has_abnormal_outgoing_edge_p (basic_block bb)
> -{
> - edge e;
> - edge_iterator ei;
> -
> - FOR_EACH_EDGE (e, ei, bb->succs)
> - if (e->flags & EDGE_ABNORMAL)
> - return true;
> -
> - return false;
> -}
> -
> /* Expand calls to inline functions in the body of FN. */
>
> unsigned int
>
