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

[Cary Coutant <ccoutant at gmail dot com>]

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

Attachment: pr23016.patch
Description: Binary data