This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] PR gold/17704
- From: Alan Modra <amodra at gmail dot com>
- To: Sriraman Tallam <tmsriram at google dot com>
- Cc: binutils <binutils at sourceware dot org>, ngg at tesorit dot com, Cary Coutant <ccoutant at gmail dot com>, David Li <davidxl at google dot com>, Roland McGrath <mcgrathr at google dot com>
- Date: Mon, 18 Jul 2016 15:20:57 +0930
- Subject: Re: [PATCH] PR gold/17704
- Authentication-results: sourceware.org; auth=none
- References: <CAAs8HmwBLxMgozRZ2pQkUTxxBrweWc7d8eTv9mKpZDX7ucEahQ@mail.gmail.com>
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.
> 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