Re: [RFC] Isolate & simplify paths with undefined behaviour

[Jeff Law <law at redhat dot com>]

On 10/23/13 07:05, Richard Biener wrote:
> Btw, -ftree-isolate-paths sounds a bit generic for isolating paths leading
> to undefined behavior, maybe -fisolate-errorneous-paths?  (please drop
> 'tree-' from new options, 'tree' isn't meaningful to GCC users)
Seems reasonable, particularly since I think this can and should be 
easily extended for things like out-of-bounds array accesses and perhaps 
other stuff.  Oh yea, it'll be -fisolate-erroneous-paths.  Done.

> The file should be named gimple-ssa-isolate-paths.c, too.
Done.

> +   for (si = gsi_start_nondebug_bb (bb), si2 = gsi_start_nondebug_bb
> (duplicate);
> +        !gsi_end_p (si) && !gsi_end_p (si2);
> +        gsi_next_nondebug (&si), gsi_next_nondebug (&si2))
> +     {
> +       while (gimple_code (gsi_stmt (si)) == GIMPLE_LABEL)
> +       gsi_next_nondebug (&si);
>
> gsi_after_labels (bb);
> if (is_gimple_debug (gsi_stmt (gsi)))
>    gsi_next_nondebug (&gsi);
>
> warrants a new gsi_nondebug_after_labels ()
I assume you meant gsi_start_nondebug_after_labels_bb.  Once we hit the
first real statement in a BB  we shouldn't see any more labels in that 
block, so we just need to use gsi_next_nondebug after that.





> +       /* SI2 is the iterator in the duplicate block and it now points
> +        to our victim statement.  */
> +       gimple_seq seq = NULL;
> +       tree t = build_call_expr_loc (0,
> +                                   builtin_decl_explicit (BUILT_IN_TRAP), 0);
> +       gimplify_and_add (t, &seq);
> +       gsi_insert_before (&si2, seq, GSI_SAME_STMT);
>
> please build a gimple call stmt directly instead of going through the
> gimplifier.
I can't recall where I got that gem (wherever it was it should be fixed 
too), but yea, going through the gimplifier is dumb.  Fixed.


> +             if (operand_equal_p (op, integer_zero_node, 0))
>
> integer_zerop ()
Fixed.  Note this instance goes away, but there was another in the 
stanza which detected explicit NULL pointer dereferences in the IL.

> +                 /* We've got a NULL PHI argument.  Now see if the
> +                    PHI's result is dereferenced within BB.  */
> +                 FOR_EACH_IMM_USE_STMT (use_stmt, iter, lhs)
> +                   {
> +                     unsigned int j;
> +                     bool removed_edge = false;
> +
> +                     /* We only care about uses in BB which are assignements
> +                        with memory operands.
> +
> +                        We could see if any uses are as function arguments
> +                        when the callee has marked the argument as being
> +                        non-null.  */
> +                     if (gimple_bb (use_stmt) != bb
> +                         || !gimple_has_mem_ops (use_stmt)
> +                         || gimple_code (use_stmt) != GIMPLE_ASSIGN)
>
> !is_gimple_assign (), you can drop the has_mem_ops check, but ...
>
> +                       continue;
> +
> +                     /* Look at each operand for a memory reference using
> +                        LHS.  */
>
> ... you want to use walk_stmt_load_store_ops () which handles all kinds
> of stmts fine.
Actually for this code Marc Glisse recommended infer_nonnull_range, and 
I'm in agreement.



> +                     for (j = 0; j < gimple_num_ops (use_stmt); j++)
> +                       {
> +                         tree op = gimple_op (use_stmt, j);
> +
> +                         if (op == NULL)
> +                           continue;
> +
> +                         while (handled_component_p (op))
> +                           op = TREE_OPERAND (op, 0);
> +
> +                         if ((TREE_CODE (op) == MEM_REF
> +                              || TREE_CODE (op) == INDIRECT_REF)
>
> no more INDIRECT_REFs are in the IL.
Oh.  I learn something every day.  I vaguely recall discussing MEM_REF 
at a summit years ago, but I was a bit pre-occupied with personal 
matters.  I guess it totally subsumed INDIRECT_REF at this stage in the 
compiler.  Can't complain about that :-)

I'll need to rework the second stanza which detects NULL dereferences 
explicitly appearing in the IL a bit, but it shouldn't be too difficult.

Thanks for the feedback,
Jeff