Re: Fix a tcb crash and a potential bug on mainline

[Jeffrey A Law <law@redhat.com>]

On Mon, 2004-10-18 at 15:03, Steven Bosscher wrote:

> Later on cleanup_control_flow() tries to do its thing on basic block 4.
> It tries to fold "(c_13 != 0B)", but c_13 is on the free list, so it
> does not have a TREE_TYPE, so fold crashes.
Hmm.  I wonder if this is the same bug Andrew Pinski was trying to
fix by not clearing TREE_TYPE.  Your description matches a lot of
the tidbits Andrew mentioned.


> 
> I think the root cause of this bug is that c_13 is put on the free list,
> and I think its SSA_NAME_DEF_STMT should be set to the empty statement,
> because remove_phi_arg_num() cannot know if there are any immediate
> uses of it still left. 
???  Putting c_13 on the free list is the right thing to do.  I'm not
sure why you think setting SSA_NAME_DEF_STMT to NULL is particularly
interesting though.  Whether or not the name has any uses left in the
IL isn't something we use to determine whether or not to set
SSA_NAME_DEF_STMT to any particular value IIRC. 


>  I don't know if making that change could have
> some unforseen effects, so I decided to re-hide the the bug by just not
> looking at unreachable blocks in cleanup_control_flow().
I'm not sure this is sufficient to hide the bug well enough though.


>   I've tried to
> convince myself that this can not happen in other situations, but I'm
> not sure.
Well, any code that deletes edges from the CFG, then later looks at
statements without first calling delete_unreachable_blocks is
suspect.  But I'm not sure if we've even solved this problem
within the context of cleanup_tree_cfg.

Let's say we have a PHI in block A.  Assume the result is used in 
block B.  Assume block A will become unreachable.

While block B must also be unreachable (block B is dominated by
block A), we won't necessarily have removed all its incoming edges.

Thus when we process block B we can still end up touching a statement
which references a released SSA_NAME.  Ugh...

>   This may be something Andrew may want to look into as part
> of his immediate uses work??
It's something we've pondered and talked about a little.  Basically
with the immediate use work we would know where all the uses are
and we could probably arrange to delete those uses.

The trick is whether or not we can do that without introducing the
kinds of problems we had with delete_insn in the past which tried
to do similar stuff.

> 
> This fix is completely untested at the moment, but it's quite obvious
> and it fixes the ICE I was seeing.  Still, I am a bit surprised that we
> have not been in trouble over this bug before - perhaps we should still
> fix the remove_phi_arg_num() issue instead of papering it over this way?
I'm not sure what you mean by fix remove_phi_arg_num except to compute
the immediate uses of the result and delete those statements, which
in turn can cause other statements to get deleted.  Basically it turns
into immediate use based DCE.  Roughly the same thing we'd end up
doing if we tried to do integrated dead code removal using
immediate uses.

Alternately we could split up cleanup_control_expr_graph into two
phases.  The first identifies all the COND_EXPRs and SWITCH_EXPRs
which need updating, but does not do any edge removal.   Once we
have done our full IL scan, we go back to the nodes where we know
which outgoing edge will be taken and proceed to remove the 
COND_EXPR/SWITCH_EXPR and the untaken edges.

jeff