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

[Jeff Law <law@redhat.com>]

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!

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.

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.


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