[PATCH, PR 59176] Mark "zombie" call graph nodes to remove verifier false positive

[Jan Hubicka]

> Hi,
> 
> On Fri, Mar 21, 2014 at 09:40:39PM +0100, Jan Hubicka wrote:
> > > On Thu, 20 Mar 2014, Martin Jambor wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Thu, Mar 20, 2014 at 07:40:56PM +0100, Jakub Jelinek wrote:
> > > > > On Thu, Mar 20, 2014 at 05:07:32PM +0100, Martin Jambor wrote:
> > > > > > in the PR, verifier claims an edge is pointing to a wrong declaration
> > > > > > even though it has successfully verified the edge multiple times
> > > > > > before.  The reason is that symtab_remove_unreachable_nodes decides to
> > > > > > "remove the body" of a node and also clear any information that it is
> > > > > > an alias of another in the process (more detailed analysis in comment
> > > > > > #9 of the bug).
> > > > > > 
> > > > > > In bugzilla Honza wrote that "silencing the verifier" is the way to
> > > > > > go.  Either we can dedicate a new flag in each cgraph_node or
> > > > > > symtab_node just for the purpose of verification or do something more
> > > > > > hackish like the patch below which re-uses the former_clone_of field
> > > > > > for this purpose.  Since clones are always private nodes, they should
> > > > > > always either survive removal of unreachable nodes or be completely
> > > > > > killed by it and should never enter the in_border zombie state.
> > > > > > Therefore their former_clone_of must always be NULL.  So I added a new
> > > > > > special value, error_mark_node, to mark this zombie state and taught
> > > > > > the verifier to be happy with such nodes.
> > > > > > 
> > > > > > Bootstrapped and tested on x86_64-linux.  What do you think?
> > > > > 
> > > > > Don't we have like 22 spare bits in cgraph_node and 20 spare bits in
> > > > > symtab_node?  I'd find it clearer if you just used a new flag to mark the
> > > > > zombie nodes.  Though, I'll let Richard or Honza to decide, don't feel
> > > > > strongly about it.
> > > > > 
> > > > 
> > > > I guess you are right, here is the proper version which is currently
> > > > undergoing bootstrap and testing.
> > > 
> > > I agree with Jakub, the following variant is ok.
> > 
> > With the extra bit, you probably will need to LTO pickle it, too.
> 
> Oops, that omission is a mistake.  I'd like to blame it on quilt but
> it is more probable I simply forgot.  I will commit the patch below as
> obvious after it passes a bootstrap.
> 
> > I would go with just clerning the thunk flag: this makes thunk to behave like
> > external function that is safe to do.
> 
> No, no thunks are involved here, it is an alias and the alias flag is
> cleared and was even before I fixed PR 60419.  The problem is exactly
> that after symtab_remove_unreachable_nodes clears the flag (and the
> associated reference), the verifier sees just an ordinary symbol, now
> seemingly unrelated to the edge destination (or the node it is cloned
> from).
> 
> > (I am back in civilization from Alaska camping, will catch up with email
> > early next week)
> 
> Well, if camping in Alaska was not punishable by sever email backlog,
> life would not be fair at all.
> 
> Thanks,
> 
> Martin
> 
> 
> 2014-03-24  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/59176
> 	* lto-cgraph.c (lto_output_node): Stream body_removed flag.
> 	(lto_output_varpool_node): Likewise.
> 	(input_overwrite_node): Likewise.
> 	(input_varpool_node): Likewise.

Hmm, that looks OK then. I guess eventually we will need to retire that cgraph verfier check,
since it is harder and harder to keep track of all types of redirections we do...
But it is useful one, so lets keep it alive for a bit longer.

Honza