Re: [PATCH] Fix create_dispatcher_calls ICE (PR ipa/89684)

[Richard Biener <rguenther at suse dot de>]

On Wed, 13 Mar 2019, Jakub Jelinek wrote:

> Hi!
> 
> create_dispatcher_calls iterates over ipa_ref *s referring the current node
> and wants to remove them all and create new ones.  The problem is
> that if there are any ipa_ref objects where two or more of them are from the
> same cgraph node to the current node (in the testcases there are both cases
> where there are two or more foo -> foo self-references, or two or more
> foo.avx -> foo references), when we ref->remove_reference () one of them,
> that method will actually move some other ipa_ref that was last in
> references vector over to that and thus invalidate what some other ipa_ref
> pointers point to (the pointers still point to some elements of some
> references vector, but could either point past the last one, or could point
> to an element that was overwritten with something else).
> 
> The following patch fixes it by remove_reference all references immediately,
> but push into the vector their content first; besides that
> ref->remove_reference we are just accessing a couple of fields from that,
> not calling any other methods.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM.

Richard.

> 2019-03-13  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR ipa/89684
> 	* multiple_target.c (create_dispatcher_calls): Change
> 	references_to_redirect from vector of ipa_ref * to vector of ipa_ref.
> 	In the node->iterate_referring loop, push *ref rather than ref, call
> 	ref->remove_reference () and always pass 0 to iterate_referring.
> 
> --- gcc/multiple_target.c.jj	2019-03-13 09:23:35.345567298 +0100
> +++ gcc/multiple_target.c	2019-03-13 10:12:49.071371927 +0100
> @@ -103,10 +103,16 @@ create_dispatcher_calls (struct cgraph_n
>      inode->resolve_alias (cgraph_node::get (resolver_decl));
>  
>    auto_vec<cgraph_edge *> edges_to_redirect;
> -  auto_vec<ipa_ref *> references_to_redirect;
> +  /* We need to capture the references by value rather than just pointers to them
> +     and remove them right away, as removing them later would invalidate what
> +     some other reference pointers point to.  */
> +  auto_vec<ipa_ref> references_to_redirect;
>  
> -  for (unsigned i = 0; node->iterate_referring (i, ref); i++)
> -    references_to_redirect.safe_push (ref);
> +  while (node->iterate_referring (0, ref))
> +    {
> +      references_to_redirect.safe_push (*ref);
> +      ref->remove_reference ();
> +    }
>  
>    /* We need to remember NEXT_CALLER as it could be modified in the loop.  */
>    for (cgraph_edge *e = node->callers; e ; e = e->next_caller)
> @@ -146,13 +152,11 @@ create_dispatcher_calls (struct cgraph_n
>  		}
>  
>  	      symtab_node *source = ref->referring;
> -	      ref->remove_reference ();
>  	      source->create_reference (inode, IPA_REF_ADDR);
>  	    }
>  	  else if (ref->use == IPA_REF_ALIAS)
>  	    {
>  	      symtab_node *source = ref->referring;
> -	      ref->remove_reference ();
>  	      source->create_reference (inode, IPA_REF_ALIAS);
>  	      source->add_to_same_comdat_group (inode);
>  	    }
> --- gcc/testsuite/gcc.target/i386/pr89684.c.jj	2019-03-13 10:12:05.677080120 +0100
> +++ gcc/testsuite/gcc.target/i386/pr89684.c	2019-03-13 10:11:56.390231682 +0100
> @@ -0,0 +1,23 @@
> +/* PR ipa/89684 */
> +/* { dg-do compile } */
> +/* { dg-require-ifunc "" } */
> +
> +void bar (int, void (*) (void));
> +
> +__attribute__((target_clones ("default", "avx")))
> +void foo (void)
> +{
> +  bar (0, foo);
> +  bar (0, foo);
> +}
> +
> +__attribute__((target_clones ("default", "avx", "avx2")))
> +void baz (void)
> +{
> +  bar (0, foo);
> +  bar (0, foo);
> +  bar (0, foo);
> +  bar (0, foo);
> +  bar (0, foo);
> +  bar (0, foo);
> +}
> 
> 	Jakub
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)