Re: [PATCH] PR gold/17704

[Sriraman Tallam <tmsriram at google dot com>]

On Thu, Jul 21, 2016 at 11:11 AM, Sriraman Tallam <tmsriram@google.com> wrote:
> On Sun, Jul 17, 2016 at 10:50 PM, Alan Modra <amodra@gmail.com> wrote:
>> On Thu, Jul 14, 2016 at 11:48:39AM -0700, Sriraman Tallam wrote:
>>> @@ -671,7 +667,31 @@ match_sections(unsigned int iteration_num,
>>>                           this_secn_contents.c_str(),
>>>                           this_secn_contents.length()) != 0)
>>>                    continue;
>>> -              (*kept_section_id)[i] = kept_section;
>>> +
>>> +           // Check section alignment here.
>>> +              // The section with the larger alignment requirement
>>> +              // should be kept. Do not fold if the larger is not
>>> +              // divisible by the smaller, but that case will never
>>> +              // happen if the alignments are powers of 2.
>>> +              uint64_t align_i = section_addraligns[i];
>>> +              uint64_t align_kept = section_addraligns[kept_section];
>>> +              if (align_i <= align_kept)
>>> +                {
>>> +                  if (align_i != align_kept && align_i != 0
>>> +                   && (align_kept % align_i) != 0)
>>> +                      continue;
>>> +                  (*kept_section_id)[i] = kept_section;
>>> +                }
>>> +              else
>>> +                {
>>> +                  if (align_kept != 0 && (align_i % align_kept) != 0)
>>> +                      continue;
>>> +                  (*kept_section_id)[kept_section] = i;
>>> +                  it->second = i;
>>> +                  full_section_contents[kept_section].swap(
>>> +                      full_section_contents[i]);
>>> +                }
>>> +
>>>                converged = false;
>>>                break;
>>>              }
>>
>> The gabi says of sh_addralign "Currently, only 0 and positive integral
>> powers of two are allowed", so I think you can discount the
>> possibility of alignment not a power of two.  It's not worth
>> supporting here when other parts of gold do not, eg. see layout.cc
>> calls to align_address.
>>
>> Please also fix indentation, only some of the above lines use tabs.
>
> New patch attached with the changes.
>
> PR gold/17704
> * icf.cc (match_sections): Add new parameter section_addraligns.
> Check section alignment and keep the section with the strictest
> alignment.
> (find_identical_sections): New local variable section_addraligns.
> Store each section's alignment.
> * testsuite/pr17704a_test.s: New file.
> * testsuite/Makefile.am (pr17704a_test): New test.
> * testsuite/Makefile.in: Regenerate.


Hi Cary,

     Ping, in case you didnt take a look at this.

Thanks
Sri

>
> Thanks
> Sri
>
>>
>>> diff --git a/gold/testsuite/Makefile.am b/gold/testsuite/Makefile.am
>>> index c8d3093..7b9b267 100644
>>> --- a/gold/testsuite/Makefile.am
>>> +++ b/gold/testsuite/Makefile.am
>>> @@ -355,6 +355,12 @@ icf_sht_rel_addend_test: icf_sht_rel_addend_test_1.o icf_sht_rel_addend_test_2.o
>>>  icf_sht_rel_addend_test.stdout: icf_sht_rel_addend_test
>>>       $(TEST_NM) icf_sht_rel_addend_test > icf_sht_rel_addend_test.stdout
>>>
>>> +check_PROGRAMS += pr17704a_test
>>> +pr17704a_test.o: pr17704a_test.s
>>> +     $(TEST_AS) --64  -o $@ $<
>>> +pr17704a_test: pr17704a_test.o gcctestdir/ld
>>> +     gcctestdir/ld --icf=all -o $@ $<
>>> +
>>>  check_PROGRAMS += large_symbol_alignment
>>>  large_symbol_alignment_SOURCES = large_symbol_alignment.cc
>>>  large_symbol_alignment_DEPENDENCIES = gcctestdir/ld
>>
>> The new test contains x86 assembly, but is enabled for all native
>> targets using gcc.  Please move to a more appropriate part of the
>> makefile.
>>
>> --
>> Alan Modra
>> Australia Development Lab, IBM

Attachment: icf_section_alignment.txt
Description: Text document