[PR,89693] Reorganize cgraph_node::clone_of_p

Hi,

this reorganization of cgraph_node:clone_of_p() prevents verifier
falsely ICEing because it cannot recognize that a former thunk (expanded
even before reaching pass pipeline) was cloned by IPA-CP.

It basically traverses the clone_of chain at each step of thunk chain
traversal, rather than just after it.  This is only done when checking
is on, so the extra little overhead should be of little concern.

Bootstrapped, LTO-bootstrapped and tested on x86_64, OK for trunk?  If
so, I will check if we need it in GCC 8 and if so, backport it there
too.

Thanks,

Martin


2019-04-11  Martin Jambor  <mjambor@suse.cz>

	PR ipa/89693
	* cgraph.c (clone_of_p): Loop over clone chain for each step in
	the thunk chain.

	testsuite/
	* g++.dg/ipa/pr89693.C: New test.
---
 gcc/cgraph.c                       | 30 ++++++++++-------
 gcc/testsuite/g++.dg/ipa/pr89693.C | 52 ++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+), 12 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/ipa/pr89693.C

Dne 2019-04-11 17:14, Martin Jambor napsal:
> Hi,
> 
> this reorganization of cgraph_node:clone_of_p() prevents verifier
> falsely ICEing because it cannot recognize that a former thunk 
> (expanded
> even before reaching pass pipeline) was cloned by IPA-CP.
> 
> It basically traverses the clone_of chain at each step of thunk chain
> traversal, rather than just after it.  This is only done when checking
> is on, so the extra little overhead should be of little concern.
> 
> Bootstrapped, LTO-bootstrapped and tested on x86_64, OK for trunk?  If
> so, I will check if we need it in GCC 8 and if so, backport it there
> too.
> 
> Thanks,
> 
> Martin
> 
> 
> 2019-04-11  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR ipa/89693
> 	* cgraph.c (clone_of_p): Loop over clone chain for each step in
> 	the thunk chain.
> 
> 	testsuite/
> 	* g++.dg/ipa/pr89693.C: New test.

Thanks,
the patch is OK.  If this verification turns out to be too much trouble, 
i would be for removing it
(it really need to second guess all sane transformations do by all 
passes we have)

Honza
> ---
>  gcc/cgraph.c                       | 30 ++++++++++-------
>  gcc/testsuite/g++.dg/ipa/pr89693.C | 52 ++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+), 12 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/ipa/pr89693.C
> 
> diff --git a/gcc/cgraph.c b/gcc/cgraph.c
> index 49d80ad1e28..b1b0b4c42d5 100644
> --- a/gcc/cgraph.c
> +++ b/gcc/cgraph.c
> @@ -2977,17 +2977,25 @@ cgraph_node::collect_callers (void)
>  static bool
>  clone_of_p (cgraph_node *node, cgraph_node *node2)
>  {
> -  bool skipped_thunk = false;
>    node = node->ultimate_alias_target ();
>    node2 = node2->ultimate_alias_target ();
> 
> +  if (node2->clone_of == node
> +      || node2->former_clone_of == node->decl)
> +    return true;
> +
> +  if (!node->thunk.thunk_p && !node->former_thunk_p ())
> +    {
> +      while (node2 && node->decl != node2->decl)
> +	node2 = node2->clone_of;
> +      return node2 != NULL;
> +    }
> +
>    /* There are no virtual clones of thunks so check former_clone_of or 
> if we
>       might have skipped thunks because this adjustments are no longer
>       necessary.  */
>    while (node->thunk.thunk_p || node->former_thunk_p ())
>      {
> -      if (node2->former_clone_of == node->decl)
> -	return true;
>        if (!node->thunk.this_adjusting)
>  	return false;
>        /* In case of instrumented expanded thunks, which can have 
> multiple calls
> @@ -2996,23 +3004,21 @@ clone_of_p (cgraph_node *node, cgraph_node 
> *node2)
>        if (node->callees->next_callee)
>  	return true;
>        node = node->callees->callee->ultimate_alias_target ();
> -      skipped_thunk = true;
> -    }
> 
> -  if (skipped_thunk)
> -    {
>        if (!node2->clone.args_to_skip
>  	  || !bitmap_bit_p (node2->clone.args_to_skip, 0))
>  	return false;
>        if (node2->former_clone_of == node->decl)
>  	return true;
> -      else if (!node2->clone_of)
> -	return false;
> +
> +      cgraph_node *n2 = node2;
> +      while (n2 && node->decl != n2->decl)
> +	n2 = n2->clone_of;
> +      if (n2)
> +	return true;
>      }
> 
> -  while (node2 && node->decl != node2->decl)
> -    node2 = node2->clone_of;
> -  return node2 != NULL;
> +  return false;
>  }
> 
>  /* Verify edge count and frequency.  */
> diff --git a/gcc/testsuite/g++.dg/ipa/pr89693.C
> b/gcc/testsuite/g++.dg/ipa/pr89693.C
> new file mode 100644
> index 00000000000..4ac83eeeb3a
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/ipa/pr89693.C
> @@ -0,0 +1,52 @@
> +// Copyright (C) 2005 Free Software Foundation, Inc.
> +// Contributed by Nathan Sidwell 4 Apr 2005 <nathan@codesourcery.com>
> +// Re-purposed to check for re-rurgesnce of PR 89693 in 2019.
> +
> +// { dg-do compile }
> +// { dg-options "-O3 -fno-ipa-icf-functions" }
> +
> +// Origin: yanliu@ca.ibm.com
> +//         nathan@codesourcery.com
> +
> +struct A {
> +  virtual void One ();
> +};
> +struct B  {
> +  virtual B *Two ();
> +  virtual B &Three ();
> +};
> +
> +struct C : A, B
> +{
> +  virtual C *Two ();
> +  virtual C &Three ();
> +};
> +void A::One () {}
> +B *B::Two()    {return this;}
> +B &B::Three()    {return *this;}
> +C *C::Two ()   {return 0;}
> +C &C::Three ()   {return *(C *)0;}
> +
> +B *Foo (B *b)
> +{
> +  return b->Two ();
> +}
> +
> +B &Bar (B *b)
> +{
> +  return b->Three ();
> +}
> +
> +int main ()
> +{
> +  C c;
> +
> +  /* We should not adjust a null pointer.  */
> +  if (Foo (&c))
> +    return 1;
> +  /* But we should adjust a (bogus) null reference.  */
> +  if (!&Bar (&c))
> +    return 2;
> +
> +  return 0;
> +}