Re: [gold patch] proposed fix for PR/23016

["Than McIntosh via binutils" <binutils at sourceware dot org>]

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.
>