Fix PR ipa/64693

Hello.

Following patch is a fix for PR ipa/64693, where ICF merges a pair of functions
that somehow take a references to a different functions. Unfortunately, such function
pointer can be used for function comparison that can break an executable.

Tested on x86_64-linux-pc and no regression is added.
Apart from that, I'm able to run tests on LTO bootstraped compiler.

Ready for trunk?
Thanks,
Martin

> Hello.
> 
> Following patch is a fix for PR ipa/64693, where ICF merges a pair of functions
> that somehow take a references to a different functions. Unfortunately, such function
> pointer can be used for function comparison that can break an executable.
> 
> Tested on x86_64-linux-pc and no regression is added.
> Apart from that, I'm able to run tests on LTO bootstraped compiler.
> 
> Ready for trunk?
> Thanks,
> Martin

> >From ff65c1bb4ba1593ea42aec5d6cbedf2330d8a866 Mon Sep 17 00:00:00 2001
> From: mliska <mliska@suse.cz>
> Date: Fri, 23 Jan 2015 14:58:36 +0100
> Subject: [PATCH] Fix PR ipa/64693.
> 
> gcc/ChangeLog:
> 
> 2015-01-23  Martin Liska  <mliska@suse.cz>
> 
> 	PR ipa/64693
> 	* ipa-icf.c (sem_function::equals_private): Handle references
> 	for a pair of compared functions.

I think you can copy a logic whether given function needs thunk - in particular
you can still merge if functions are DECL_VIRTUAL.
I also think you have same problem with variable initializers - if you merge two
variables pointing to equivalent functions that however need different addresses, you
still need to prevent merging.

Honza
> 
> gcc/testsuite/ChangeLog:
> 
> 2015-01-23  Martin Liska  <mliska@suse.cz>
> 
> 	* gcc.dg/ipa/ipa-icf-26.c: Fix expected results.
> 	* gcc.dg/ipa/ipa-icf-33.c: Remove reduced line.
> 	* gcc.dg/ipa/ipa-icf-34.c: New test.
> ---
>  gcc/ipa-icf.c                         | 27 +++++++++++++++++++++-
>  gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c |  3 +--
>  gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c |  1 -
>  gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c | 43 +++++++++++++++++++++++++++++++++++
>  4 files changed, 70 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
> 
> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
> index afb5be5..4bea08d 100644
> --- a/gcc/ipa-icf.c
> +++ b/gcc/ipa-icf.c
> @@ -525,13 +525,38 @@ sem_function::equals_private (sem_item *item,
>    if (decl1 != decl2)
>      return return_false();
>  
> -
>    for (arg1 = DECL_ARGUMENTS (decl),
>         arg2 = DECL_ARGUMENTS (m_compared_func->decl);
>         arg1; arg1 = DECL_CHAIN (arg1), arg2 = DECL_CHAIN (arg2))
>      if (!m_checker->compare_decl (arg1, arg2))
>        return return_false ();
>  
> +  /* Compare if this function contains any function references
> +     and compare them.  */
> +  cgraph_node *source_node = get_node ();
> +  cgraph_node *target_node = m_compared_func->get_node ();
> +  if (source_node->num_references () != target_node->num_references ())
> +    return return_false_with_msg ("different number of references");
> +
> +  ipa_ref *r1 = NULL, *r2 = NULL;
> +
> +  for (unsigned i = 0; i < source_node->num_references (); i++)
> +    {
> +      symtab_node *ref1 = source_node->iterate_reference (i, r1)->referred;
> +      symtab_node *ref2 = target_node->iterate_reference (i, r2)->referred;
> +
> +      bool is_cgraph1 = is_a<cgraph_node *> (ref1);
> +      bool is_cgraph2 = is_a<cgraph_node *> (ref2);
> +
> +      if (!is_cgraph1 && !is_cgraph2)
> +	continue;
> +
> +      if ((is_cgraph1 && is_cgraph2 && ref1 != ref2)
> +	  || (!is_cgraph1 && is_cgraph2)
> +	  || (is_cgraph1 && !is_cgraph2))
> +	return return_false_with_msg ("different target of a reference");
> +    }
> +
>    /* Fill-up label dictionary.  */
>    for (unsigned i = 0; i < bb_sorted.length (); ++i)
>      {
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> index 0c5a5a6..f48c040 100644
> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
> @@ -38,7 +38,6 @@ int main()
>    return 0;
>  }
>  
> -/* { dg-final { scan-ipa-dump "Semantic equality hit:bar->foo" "icf"  } } */
>  /* { dg-final { scan-ipa-dump "Semantic equality hit:remove->destroy" "icf"  } } */
> -/* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf"  } } */
> +/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
>  /* { dg-final { cleanup-ipa-dump "icf" } } */
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
> index d7e814d..7b6a8ae 100644
> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
> @@ -23,5 +23,4 @@ int main()
>  }
>  
>  /* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
> -/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
>  /* { dg-final { cleanup-ipa-dump "icf" } } */
> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
> new file mode 100644
> index 0000000..c8507c7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
> @@ -0,0 +1,43 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O0 -fipa-icf -fdump-ipa-icf-details"  } */
> +
> +#include <stdlib.h>
> +#include <assert.h>
> +
> +int callback1(int a)
> +{
> +  return a * a;
> +}
> +
> +int callback2(int a)
> +{
> +  return a * a;
> +}
> +
> +static int test(int (*callback) (int))
> +{
> +  if (callback == callback1)
> +    return 1;
> +
> +  return 0;
> +}
> +
> +int foo()
> +{
> +  return test(&callback1);
> +}
> +
> +int bar()
> +{
> +  return test(&callback2);
> +}
> +
> +int main()
> +{
> +  assert (foo() != bar());
> +
> +  return 0;
> +}
> +
> +/* { dg-final { scan-ipa-dump "different target of a reference" "icf"  } } */
> +/* { dg-final { cleanup-ipa-dump "icf" } } */
> -- 
> 2.1.2
>

On 01/23/2015 10:26 PM, Jan Hubicka wrote:
>> Hello.
>>
>> Following patch is a fix for PR ipa/64693, where ICF merges a pair of functions
>> that somehow take a references to a different functions. Unfortunately, such function
>> pointer can be used for function comparison that can break an executable.
>>
>> Tested on x86_64-linux-pc and no regression is added.
>> Apart from that, I'm able to run tests on LTO bootstraped compiler.
>>
>> Ready for trunk?
>> Thanks,
>> Martin
>
>> >From ff65c1bb4ba1593ea42aec5d6cbedf2330d8a866 Mon Sep 17 00:00:00 2001
>> From: mliska <mliska@suse.cz>
>> Date: Fri, 23 Jan 2015 14:58:36 +0100
>> Subject: [PATCH] Fix PR ipa/64693.
>>
>> gcc/ChangeLog:
>>
>> 2015-01-23  Martin Liska  <mliska@suse.cz>
>>
>> 	PR ipa/64693
>> 	* ipa-icf.c (sem_function::equals_private): Handle references
>> 	for a pair of compared functions.
>
> I think you can copy a logic whether given function needs thunk - in particular
> you can still merge if functions are DECL_VIRTUAL.
> I also think you have same problem with variable initializers - if you merge two
> variables pointing to equivalent functions that however need different addresses, you
> still need to prevent merging.
>
> Honza

Hello.

If I understand correctly, the right way would be to identify all function merge candidates
that have address taken and create a thunk. With that restriction all function comparison will
be safe.

Thanks,
Martin

>>
>> gcc/testsuite/ChangeLog:
>>
>> 2015-01-23  Martin Liska  <mliska@suse.cz>
>>
>> 	* gcc.dg/ipa/ipa-icf-26.c: Fix expected results.
>> 	* gcc.dg/ipa/ipa-icf-33.c: Remove reduced line.
>> 	* gcc.dg/ipa/ipa-icf-34.c: New test.
>> ---
>>   gcc/ipa-icf.c                         | 27 +++++++++++++++++++++-
>>   gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c |  3 +--
>>   gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c |  1 -
>>   gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c | 43 +++++++++++++++++++++++++++++++++++
>>   4 files changed, 70 insertions(+), 4 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>>
>> diff --git a/gcc/ipa-icf.c b/gcc/ipa-icf.c
>> index afb5be5..4bea08d 100644
>> --- a/gcc/ipa-icf.c
>> +++ b/gcc/ipa-icf.c
>> @@ -525,13 +525,38 @@ sem_function::equals_private (sem_item *item,
>>     if (decl1 != decl2)
>>       return return_false();
>>
>> -
>>     for (arg1 = DECL_ARGUMENTS (decl),
>>          arg2 = DECL_ARGUMENTS (m_compared_func->decl);
>>          arg1; arg1 = DECL_CHAIN (arg1), arg2 = DECL_CHAIN (arg2))
>>       if (!m_checker->compare_decl (arg1, arg2))
>>         return return_false ();
>>
>> +  /* Compare if this function contains any function references
>> +     and compare them.  */
>> +  cgraph_node *source_node = get_node ();
>> +  cgraph_node *target_node = m_compared_func->get_node ();
>> +  if (source_node->num_references () != target_node->num_references ())
>> +    return return_false_with_msg ("different number of references");
>> +
>> +  ipa_ref *r1 = NULL, *r2 = NULL;
>> +
>> +  for (unsigned i = 0; i < source_node->num_references (); i++)
>> +    {
>> +      symtab_node *ref1 = source_node->iterate_reference (i, r1)->referred;
>> +      symtab_node *ref2 = target_node->iterate_reference (i, r2)->referred;
>> +
>> +      bool is_cgraph1 = is_a<cgraph_node *> (ref1);
>> +      bool is_cgraph2 = is_a<cgraph_node *> (ref2);
>> +
>> +      if (!is_cgraph1 && !is_cgraph2)
>> +	continue;
>> +
>> +      if ((is_cgraph1 && is_cgraph2 && ref1 != ref2)
>> +	  || (!is_cgraph1 && is_cgraph2)
>> +	  || (is_cgraph1 && !is_cgraph2))
>> +	return return_false_with_msg ("different target of a reference");
>> +    }
>> +
>>     /* Fill-up label dictionary.  */
>>     for (unsigned i = 0; i < bb_sorted.length (); ++i)
>>       {
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> index 0c5a5a6..f48c040 100644
>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-26.c
>> @@ -38,7 +38,6 @@ int main()
>>     return 0;
>>   }
>>
>> -/* { dg-final { scan-ipa-dump "Semantic equality hit:bar->foo" "icf"  } } */
>>   /* { dg-final { scan-ipa-dump "Semantic equality hit:remove->destroy" "icf"  } } */
>> -/* { dg-final { scan-ipa-dump "Equal symbols: 2" "icf"  } } */
>> +/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
>>   /* { dg-final { cleanup-ipa-dump "icf" } } */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>> index d7e814d..7b6a8ae 100644
>> --- a/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-33.c
>> @@ -23,5 +23,4 @@ int main()
>>   }
>>
>>   /* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
>> -/* { dg-final { scan-ipa-dump "Equal symbols: 1" "icf"  } } */
>>   /* { dg-final { cleanup-ipa-dump "icf" } } */
>> diff --git a/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>> new file mode 100644
>> index 0000000..c8507c7
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.dg/ipa/ipa-icf-34.c
>> @@ -0,0 +1,43 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O0 -fipa-icf -fdump-ipa-icf-details"  } */
>> +
>> +#include <stdlib.h>
>> +#include <assert.h>
>> +
>> +int callback1(int a)
>> +{
>> +  return a * a;
>> +}
>> +
>> +int callback2(int a)
>> +{
>> +  return a * a;
>> +}
>> +
>> +static int test(int (*callback) (int))
>> +{
>> +  if (callback == callback1)
>> +    return 1;
>> +
>> +  return 0;
>> +}
>> +
>> +int foo()
>> +{
>> +  return test(&callback1);
>> +}
>> +
>> +int bar()
>> +{
>> +  return test(&callback2);
>> +}
>> +
>> +int main()
>> +{
>> +  assert (foo() != bar());
>> +
>> +  return 0;
>> +}
>> +
>> +/* { dg-final { scan-ipa-dump "different target of a reference" "icf"  } } */
>> +/* { dg-final { cleanup-ipa-dump "icf" } } */
>> --
>> 2.1.2
>>
>