Re: [PATCH] Fix middle-end/67133, part 1

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

On Mon, Aug 17, 2015 at 7:31 PM, Jeff Law <law@redhat.com> wrote:
> On 08/14/2015 02:46 PM, Marek Polacek wrote:
>>>>
>>>> Then in isolate-paths in find_explicit_erroneous_behaviour we're walking
>>>> the
>>>> stmts in bb 2 and we find a null dereference, so
>>>> insert_trap_and_remove_trailing_statements
>>>> comes in play and turns bb 2 into:
>>>>
>>>>    <bb 2>:
>>>>    ...
>>>>    SR.5_10 = MEM[(const struct A &)0B];
>>>>    __builtin_trap ();
>>>>
>>>> i.e. it removs the defining statement for c_13.  Then
>>>> find_explicit_erroneous_behaviour
>>>> walks over bb 3, hits the fn1 (&D.2434.OutBufCur, &b, c_13); statement,
>>>> and
>>>> ICEs on the c_13 argument: it's a released SSA name with NULL TREE_TYPE.
>>>>
>>>> The question now is what to do with that.  Skip SSA_NAME_IN_FREE_LIST?
>>>> That
>>>> sounds weird.  Note that we're going to remove bb 3 and bb 4 anyway...
>>>
>>> Jeez, looking at the code N years later, I feel like a complete idiot. Of
>>> course that's not going to work.
>>>
>>> We certainly don't want to peek at SSA_NAME_IN_FREE_LIST.
>>
>>
>> Yeh, I thought as much.
>>
>>> I wonder if we should be walking in backwards dominator order to avoid
>>> these
>>> effects.  Or maybe just ignoring any BB with no preds.  I'll ponder those
>>> over the weekend.
>>
>>
>> I suppose both ought to work.  Or at least theoretically we could run e.g.
>> cleanup_cfg to prune the IR after we've inserted trap and removed trailing
>> stmts so that it gets rid of unreachable bbs.  Would that make sense?
>>
>> Anyway, if you think of how would you like to solve this I can take a
>> crack
>> at it next week.
>
> The funny thing here is we remove the statements after the trap to avoid
> this exact situation!

Not sure, when I came along that code in the past I wondered why we
don't just do like other passes - split the block, insert the unreachable()
at the end of the first and leave the actual cleanup to cfg-cleanup.

> I think the problem with schemes that either change the order of block
> processing, or which ignore some blocks are going to run into issues. By
> walking blocks and statements in a backwards order, we address 99% of the
> problems, including uses in PHIs in a direct successor block.
>
> What's not handled is a use in a PHI at the frontier of a subgraph that
> becomes unreachable.  We'd have to do the usual unreachable block analysis
> to catch and handle those properly.

So you are after second-level effects that require earlier unreachable paths to
be pruned?

> I don't particularly like that idea....
>
> But in walking through all that, I think I've stumbled on a simpler
> solution.  Specifically do as a little as possible and let the standard
> mechanisms clean things up :-)
>
> 1. Delete the code that removes instructions after the trap.
>
> 2. Split the block immediately after the trap and remove the edge
>    from the original block (with the trap) to the new block.

cfg-cleanup will do that for you if you have a not returning stmt ending
the previous block.

Richard.

>
> THen let the standard mechanisms handle things when that pass is complete.
>
> By setting cfg_altered, we'll  get unreachable code removal which will
> capture most of the intended effect.  DCE fires a couple more passes down in
> the pipeline to pick up the remaining tidbits.
>
> Do you want to try and tackle this?
>
> jeff
>
>