[gold patch] proposed fix for PR/23016

Tue Apr 3 11:31:00 GMT 2018

Thanks Cary for the fix -- your patch looks much nicer and more
comprehensive!
Cheers, Than

On Tue, Apr 3, 2018 at 7:30 AM Than McIntosh <thanm@google.com> wrote:

> Thanks Cary for the fix -- your patch looks much nicer and more
comprehensive!
> Cheers, Than

> On Mon, Apr 2, 2018 at 9:57 PM Cary Coutant <ccoutant@gmail.com> wrote:

>> > The patch below is a first draft of a fix for PR binutils/23016; hoping
>> > to solicit comments.

>> Thanks, Than, for the analysis and proposed patch.

>> We really want to do both of the things you were considering: (1)
>> canonicalize the .eh_frame section types and combine them even when
>> mixed, and (2) maintain separate relocation sections where the data
>> sections are separate. In the attached patch, I've done both of these.

>> > For relocatable links, this add some logic to avoid merging two input
>> > relocation sections if the sections targeted by said relocations have
>> > different section types. The motivating example for this change is a
>> > relocatable link on X86_64 that includes objects compiled by
>> > Clang/LLVM (which uses a section type of X86_64_UNWIND for the
>> > .eh_frame section) and GCC (which uses a section type of PROGBITS),
>> > resulting in an assert in output.h.

>> This isn't quite what we want to do. I've simplified the logic by
>> keeping track of the associated reloc section for each output data
>> section. When we layout a reloc section, we look to see if the output
>> data section already has an output reloc section, and use that if it
>> has one. Otherwise, we simply create a new output reloc section.

>> For .eh_frame sections, I've added logic in
>> Sized_relobj_file::do_layout to force the section type to
>> SHT_X86_64_UNWIND if it's not already, so we'll always get that
>> section type in the output file. Unfortunately, making the decision
>> there necessitated passing the overridden section type down through a
>> couple of layers, but the scope of the change wasn't too bad. (I could
>> have made the decision later in Layout::layout, avoiding some
>> interface changes, but I felt that was the wrong place to make that
>> decision.)

>> For non-dash-r links, where we send the .eh_frame sections through
>> Layout::layout_eh_frame, I've also changed
>> Layout::make_eh_frame_section to use the unwind section type for the
>> new output sections.

>> It's wrong to use a processor-specific section type in the common
>> parts of gold, so I added a target interface to select what section
>> type to use for .eh_frame, and fixed the few places where we were
>> referencing SHT_X86_64_UNWIND improperly.

>> > I would welcome suggestions on how to write a test for this fix: in
>> > order to trigger the problematic scenario there needs to be a
>> > relocatable link with two input files each containing an input section
>> > (with relocations) with the same name/flags but different section
>> > types-- not sure how to accomplish that short of writing in assembler
>> > (maybe there is some C compiler option I could use that would
>> > perturb section types?).

>> I've added a couple of simple test cases that manufacture this
>> scenario in the regular case (where we want to keep the sections
>> separate), and in the .eh_frame case (where we want to combine them).
>> I just wrote some short assembly code.

>> I'm committing the attached patch.

>> -cary

>> Fix problem where mixed section types can cause internal error during a
-r link.

>> During a -r (or --emit-relocs) link, if two sections had the same name
but
>> different section types, gold would put relocations for both sections
into
>> the same relocation section even though the data sections remained
separate.

>> For .eh_frame sections, when one section is PROGBITS and another is
>> X86_64_UNWIND, we really should be using the UNWIND section type and
>> combining the sections anyway.  For other sections, we should be
>> creating one relocation section for each output data section.

>> 2018-04-02  Cary Coutant  <ccoutant@gmail.com>

>> gold/
>>          PR gold/23016
>>          * incremental.cc (can_incremental_update): Check for unwind
section
>>          type.
>>          * layout.h (Layout::layout): Add sh_type parameter.
>>          * layout.cc (Layout::layout): Likewise.
>>          (Layout::layout_reloc): Create new output reloc section if data
>>          section does not already have one.
>>          (Layout::layout_eh_frame): Check for unwind section type.
>>          (Layout::make_eh_frame_section): Use unwind section type for
.eh_frame
>>          and .eh_frame_hdr.
>>          * object.h (Sized_relobj_file::Shdr_write): New typedef.
>>          (Sized_relobj_file::layout_section): Add sh_type parameter.
>>          (Sized_relobj_file::Deferred_layout::Deferred_layout): Add
sh_type
>>          parameter.
>>          * object.cc (Sized_relobj_file::check_eh_frame_flags): Check for
>>          unwind section type.
>>          (Sized_relobj_file::layout_section): Add sh_type parameter; pass
it
>>          to Layout::layout.
>>          (Sized_relobj_file::do_layout): Make local copy of sh_type.
>>          Force .eh_frame sections to unwind section type.
>>          Pass sh_type to layout_section.
>>          (Sized_relobj_file<size,
big_endian>::do_layout_deferred_sections):
>>          Pass sh_type to layout_section.
>>          * output.cc (Output_section::Output_section): Initialize
reloc_section_.
>>          * output.h (Output_section::reloc_section): New method.
>>          (Output_section::set_reloc_section): New method.
>>          (Output_section::reloc_section_): New data member.
>>          * target.h (Target::unwind_section_type): New method.
>>          (Target::Target_info::unwind_section_type): New data member.

>>          * aarch64.cc (aarch64_info): Add unwind_section_type.
>>          * arm.cc (arm_info, arm_nacl_info): Likewise.
>>          * i386.cc (i386_info, i386_nacl_info, iamcu_info): Likewise.
>>          * mips.cc (mips_info, mips_nacl_info): Likewise.
>>          * powerpc.cc (powerpc_info): Likewise.
>>          * s390.cc (s390_info): Likewise.
>>          * sparc.cc (sparc_info): Likewise.
>>          * tilegx.cc (tilegx_info): Likewise.
>>          * x86_64.cc (x86_64_info, x86_64_nacl_info): Likewise.

>>          * testsuite/Makefile.am (pr23016_1, pr23016_2): New test cases.
>>          * testsuite/Makefile.in: Regenerate.
>>          * testsuite/testfile.cc: Add unwind_section_type.
>>          * testsuite/pr23016_1.sh: New test script.
>>          * testsuite/pr23016_1a.s: New source file.
>>          * testsuite/pr23016_1b.s: New source file.
>>          * testsuite/pr23016_2.sh: New test script.
>>          * testsuite/pr23016_2a.s: New source file.
>>          * testsuite/pr23016_2b.s: New source file.