ICF on PowerPC Bug

ICF on PowerPC Bug

[Sriraman Tallam]

Hi,


This program fails with ICF on powerpc:

__attribute__((noinline))
int foo () { return 1; }

__attribute__((noinline))
int bar () { return 0; }

__attribute__((noinline))
int fold1() {
  return foo ();
}

__attribute__((noinline))
int fold2() {
  return bar();
}

int main()
{
  assert (fold1() != fold2());
}

because fold2 is folded onto fold1.    The only way to differentiate
fold1 and fold2 which have the same object code is via the relocation
type and the Info value is different.  However, with powerpc this is
rewritten here in icf.cc:

// Look through function descriptors
parameters->target().function_location(&loc);
 if (loc.shndx != it_v->second)
{
 it_v->second = loc.shndx;
 // Modify symvalue/addend to the code entry.
 it_a->first = loc.offset;
 it_a->second = 0;
}

I am not sure how to fix it.

Thanks
Sri

Re: ICF on PowerPC Bug

[Alan Modra]

On Tue, Apr 01, 2014 at 03:06:37PM -0700, Sriraman Tallam wrote:
> because fold2 is folded onto fold1.    The only way to differentiate
> fold1 and fold2 which have the same object code is via the relocation
> type and the Info value is different.  However, with powerpc this is
> rewritten here in icf.cc:
> 
> // Look through function descriptors
> parameters->target().function_location(&loc);
>  if (loc.shndx != it_v->second)
> {
>  it_v->second = loc.shndx;
>  // Modify symvalue/addend to the code entry.
>  it_a->first = loc.offset;
>  it_a->second = 0;
> }
> 
> I am not sure how to fix it.

The following patch fixes the problem, but did you really mean to copy
all the vectors here?

      Icf::Sections_reachable_info v =
        (it_reloc_info_list->second).section_info;
      // Stores the information of the symbol pointed to by the reloc.
      Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
      // Stores the addend and the symbol value.
      Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
      // Stores the offset of the reloc.
      Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
      Icf::Reloc_addend_size_info reloc_addend_size_info =
        (it_reloc_info_list->second).reloc_addend_size_info;


diff --git a/gold/icf.cc b/gold/icf.cc
index f30eb41..920514c 100644
--- a/gold/icf.cc
+++ b/gold/icf.cc
@@ -288,8 +288,7 @@ get_section_contents(bool first_iteration,
 
       for (; it_v != v.end(); ++it_v, ++it_s, ++it_a, ++it_o, ++it_addend_size)
         {
-	  if (first_iteration
-	      && it_v->first != NULL)
+	  if (it_v->first != NULL)
 	    {
 	      Symbol_location loc;
 	      loc.object = it_v->first;

-- 
Alan Modra
Australia Development Lab, IBM

Re: ICF on PowerPC Bug

[Sriraman Tallam]

On Tue, Apr 1, 2014 at 11:54 PM, Alan Modra <amodra@gmail.com> wrote:
> On Tue, Apr 01, 2014 at 03:06:37PM -0700, Sriraman Tallam wrote:
>> because fold2 is folded onto fold1.    The only way to differentiate
>> fold1 and fold2 which have the same object code is via the relocation
>> type and the Info value is different.  However, with powerpc this is
>> rewritten here in icf.cc:
>>
>> // Look through function descriptors
>> parameters->target().function_location(&loc);
>>  if (loc.shndx != it_v->second)
>> {
>>  it_v->second = loc.shndx;
>>  // Modify symvalue/addend to the code entry.
>>  it_a->first = loc.offset;
>>  it_a->second = 0;
>> }
>>
>> I am not sure how to fix it.
>
> The following patch fixes the problem, but did you really mean to copy
> all the vectors here?

Yikes!, no did not intend that. I will make these references and send a patch.

Thanks
Sri

>
>       Icf::Sections_reachable_info v =
>         (it_reloc_info_list->second).section_info;
>       // Stores the information of the symbol pointed to by the reloc.
>       Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
>       // Stores the addend and the symbol value.
>       Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
>       // Stores the offset of the reloc.
>       Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
>       Icf::Reloc_addend_size_info reloc_addend_size_info =
>         (it_reloc_info_list->second).reloc_addend_size_info;
>
>
> diff --git a/gold/icf.cc b/gold/icf.cc
> index f30eb41..920514c 100644
> --- a/gold/icf.cc
> +++ b/gold/icf.cc
> @@ -288,8 +288,7 @@ get_section_contents(bool first_iteration,
>
>        for (; it_v != v.end(); ++it_v, ++it_s, ++it_a, ++it_o, ++it_addend_size)
>          {
> -         if (first_iteration
> -             && it_v->first != NULL)
> +         if (it_v->first != NULL)
>             {
>               Symbol_location loc;
>               loc.object = it_v->first;
>
> --
> Alan Modra
> Australia Development Lab, IBM

Re: ICF on PowerPC Bug

[Sriraman Tallam]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

Attached patch to fix the PPC ICF problem by using references to the
reloc info vectors. Ok to commit?

Thanks
Sri

On Wed, Apr 2, 2014 at 9:49 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Tue, Apr 1, 2014 at 11:54 PM, Alan Modra <amodra@gmail.com> wrote:
>> On Tue, Apr 01, 2014 at 03:06:37PM -0700, Sriraman Tallam wrote:
>>> because fold2 is folded onto fold1.    The only way to differentiate
>>> fold1 and fold2 which have the same object code is via the relocation
>>> type and the Info value is different.  However, with powerpc this is
>>> rewritten here in icf.cc:
>>>
>>> // Look through function descriptors
>>> parameters->target().function_location(&loc);
>>>  if (loc.shndx != it_v->second)
>>> {
>>>  it_v->second = loc.shndx;
>>>  // Modify symvalue/addend to the code entry.
>>>  it_a->first = loc.offset;
>>>  it_a->second = 0;
>>> }
>>>
>>> I am not sure how to fix it.
>>
>> The following patch fixes the problem, but did you really mean to copy
>> all the vectors here?
>
> Yikes!, no did not intend that. I will make these references and send a patch.
>
> Thanks
> Sri
>
>>
>>       Icf::Sections_reachable_info v =
>>         (it_reloc_info_list->second).section_info;
>>       // Stores the information of the symbol pointed to by the reloc.
>>       Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
>>       // Stores the addend and the symbol value.
>>       Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
>>       // Stores the offset of the reloc.
>>       Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
>>       Icf::Reloc_addend_size_info reloc_addend_size_info =
>>         (it_reloc_info_list->second).reloc_addend_size_info;
>>
>>
>> diff --git a/gold/icf.cc b/gold/icf.cc
>> index f30eb41..920514c 100644
>> --- a/gold/icf.cc
>> +++ b/gold/icf.cc
>> @@ -288,8 +288,7 @@ get_section_contents(bool first_iteration,
>>
>>        for (; it_v != v.end(); ++it_v, ++it_s, ++it_a, ++it_o, ++it_addend_size)
>>          {
>> -         if (first_iteration
>> -             && it_v->first != NULL)
>> +         if (it_v->first != NULL)
>>             {
>>               Symbol_location loc;
>>               loc.object = it_v->first;
>>
>> --
>> Alan Modra
>> Australia Development Lab, IBM

[-- Attachment #2: icf_fix.txt --]
[-- Type: text/plain, Size: 1482 bytes --]

	* icf.cc (get_section_contents): Use references to reloc_info
	vectors to avoid copies.

Index: icf.cc
===================================================================
RCS file: /cvs/src/src/gold/icf.cc,v
retrieving revision 1.21
diff -u -p -r1.21 icf.cc
--- icf.cc	15 Mar 2013 07:51:32 -0000	1.21
+++ icf.cc	2 Apr 2014 19:08:28 -0000
@@ -269,15 +269,15 @@ get_section_contents(bool first_iteratio
 
   if (it_reloc_info_list != reloc_info_list.end())
     {
-      Icf::Sections_reachable_info v =
+      Icf::Sections_reachable_info &v =
         (it_reloc_info_list->second).section_info;
       // Stores the information of the symbol pointed to by the reloc.
-      Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
+      Icf::Symbol_info &s = (it_reloc_info_list->second).symbol_info;
       // Stores the addend and the symbol value.
-      Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
+      Icf::Addend_info &a = (it_reloc_info_list->second).addend_info;
       // Stores the offset of the reloc.
-      Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
-      Icf::Reloc_addend_size_info reloc_addend_size_info =
+      Icf::Offset_info &o = (it_reloc_info_list->second).offset_info;
+      Icf::Reloc_addend_size_info &reloc_addend_size_info =
         (it_reloc_info_list->second).reloc_addend_size_info;
       Icf::Sections_reachable_info::iterator it_v = v.begin();
       Icf::Symbol_info::iterator it_s = s.begin();

Re: ICF on PowerPC Bug

[Cary Coutant]

> Attached patch to fix the PPC ICF problem by using references to the
> reloc info vectors. Ok to commit?

-      Icf::Sections_reachable_info v =
+      Icf::Sections_reachable_info &v =
         (it_reloc_info_list->second).section_info;
       // Stores the information of the symbol pointed to by the reloc.
-      Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
+      Icf::Symbol_info &s = (it_reloc_info_list->second).symbol_info;
       // Stores the addend and the symbol value.
-      Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
+      Icf::Addend_info &a = (it_reloc_info_list->second).addend_info;
       // Stores the offset of the reloc.
-      Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
-      Icf::Reloc_addend_size_info reloc_addend_size_info =
+      Icf::Offset_info &o = (it_reloc_info_list->second).offset_info;
+      Icf::Reloc_addend_size_info &reloc_addend_size_info =
         (it_reloc_info_list->second).reloc_addend_size_info;
       Icf::Sections_reachable_info::iterator it_v = v.begin();
       Icf::Symbol_info::iterator it_s = s.begin();

Except for v and a, I think these can all be const, and their
iterators can be const_iterators. OK with those changes.

Thanks!

-cary

Re: ICF on PowerPC Bug

[Sriraman Tallam]

Committed with those changes.

Thanks
Sri

On Wed, Apr 2, 2014 at 2:12 PM, Cary Coutant <ccoutant@google.com> wrote:
>> Attached patch to fix the PPC ICF problem by using references to the
>> reloc info vectors. Ok to commit?
>
> -      Icf::Sections_reachable_info v =
> +      Icf::Sections_reachable_info &v =
>          (it_reloc_info_list->second).section_info;
>        // Stores the information of the symbol pointed to by the reloc.
> -      Icf::Symbol_info s = (it_reloc_info_list->second).symbol_info;
> +      Icf::Symbol_info &s = (it_reloc_info_list->second).symbol_info;
>        // Stores the addend and the symbol value.
> -      Icf::Addend_info a = (it_reloc_info_list->second).addend_info;
> +      Icf::Addend_info &a = (it_reloc_info_list->second).addend_info;
>        // Stores the offset of the reloc.
> -      Icf::Offset_info o = (it_reloc_info_list->second).offset_info;
> -      Icf::Reloc_addend_size_info reloc_addend_size_info =
> +      Icf::Offset_info &o = (it_reloc_info_list->second).offset_info;
> +      Icf::Reloc_addend_size_info &reloc_addend_size_info =
>          (it_reloc_info_list->second).reloc_addend_size_info;
>        Icf::Sections_reachable_info::iterator it_v = v.begin();
>        Icf::Symbol_info::iterator it_s = s.begin();
>
> Except for v and a, I think these can all be const, and their
> iterators can be const_iterators. OK with those changes.
>
> Thanks!
>
> -cary