Re: [PATCH] Generalize GC support for SHF_LINK_ORDER.

["Evgenii Stepanov via binutils" <binutils at sourceware dot org>]

Friendly ping. Any opinions?


On Fri, Mar 31, 2017 at 5:16 PM, Evgenii Stepanov <eugenis@google.com> wrote:
> This change implements new GC semantics of SHF_LINK_ORDER discussed
> in the following thread on generic-abi ML:
> https://groups.google.com/d/msg/generic-abi/_CbBM6T6WeM/LZnqx1KZAQAJ
>
> It is a generalization of the logic that prevents garbage collection
> of .ARM.exidx sections by adding a GC reference from the
> corresponding text section (found by the sh_link field of the
> .ARM.exidx section).
>
> The new logic is exactly the same, and the code is basically a copy
> of the .ARM.exidx code, but now it is keyed off the SHF_LINK_ORDER
> flag instead of SHT_ARM_EXIDX section type. The existing ARM behavior
> is not changed because Gold already demands that SHT_ARM_EXIDX
> sections have SHF_LINK_ORDER flag set.
>
> Another change is not to consider __start_$section and
> __stop_$section symbols as GC references when $section has
> SHF_LINK_ORDER flag. The idea behind this is that SHF_LINK_ORDER
> section can contain metadata for the linked section, and section
> start/stop symbols can be used to enumarate that metadata at runtime.
> In this case the liveness of the metadata section should be
> determined only by the liveness of the linked section, and not be
> affected by the presence of start/stop symbols.
> ---
>  ChangeLog      |  6 ++++++
>  gold/arm.cc    | 47 -----------------------------------------------
>  gold/object.cc |  4 +++-
>  gold/reloc.cc  | 26 ++++++++++++++++++++++++++
>  4 files changed, 35 insertions(+), 48 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index 3904dc134d..082cb0670f 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2016-03-31 Evgenii Stepanov <eugenis@google.com>
> +
> +       * gold/arm.cc: Remove do_gc_process_relocs
> +       * gold/reloc.cc: Handle SHF_LINK_ORDER in do_gc_process_relocs
> +       * gold/object.cc: Do not add SHF_LINK_ORDER sections to cident_sections.
> +
>  2016-03-17  Cary Coutant  <ccoutant@gmail.com>
>
>         * configure.ac: Add mips and s390 to the gold target check.
> diff --git a/gold/arm.cc b/gold/arm.cc
> index c47b00224c..f83c3bd737 100644
> --- a/gold/arm.cc
> +++ b/gold/arm.cc
> @@ -1686,10 +1686,6 @@ class Arm_relobj : public Sized_relobj_file<32, big_endian>
>    void
>    do_read_symbols(Read_symbols_data* sd);
>
> -  // Process relocs for garbage collection.
> -  void
> -  do_gc_process_relocs(Symbol_table*, Layout*, Read_relocs_data*);
> -
>   private:
>
>    // Whether a section needs to be scanned for relocation stubs.
> @@ -6939,49 +6935,6 @@ Arm_relobj<big_endian>::do_read_symbols(Read_symbols_data* sd)
>      }
>  }
>
> -// Process relocations for garbage collection.  The ARM target uses .ARM.exidx
> -// sections for unwinding.  These sections are referenced implicitly by
> -// text sections linked in the section headers.  If we ignore these implicit
> -// references, the .ARM.exidx sections and any .ARM.extab sections they use
> -// will be garbage-collected incorrectly.  Hence we override the same function
> -// in the base class to handle these implicit references.
> -
> -template<bool big_endian>
> -void
> -Arm_relobj<big_endian>::do_gc_process_relocs(Symbol_table* symtab,
> -                                            Layout* layout,
> -                                            Read_relocs_data* rd)
> -{
> -  // First, call base class method to process relocations in this object.
> -  Sized_relobj_file<32, big_endian>::do_gc_process_relocs(symtab, layout, rd);
> -
> -  // If --gc-sections is not specified, there is nothing more to do.
> -  // This happens when --icf is used but --gc-sections is not.
> -  if (!parameters->options().gc_sections())
> -    return;
> -
> -  unsigned int shnum = this->shnum();
> -  const unsigned int shdr_size = elfcpp::Elf_sizes<32>::shdr_size;
> -  const unsigned char* pshdrs = this->get_view(this->elf_file()->shoff(),
> -                                              shnum * shdr_size,
> -                                              true, true);
> -
> -  // Scan section headers for sections of type SHT_ARM_EXIDX.  Add references
> -  // to these from the linked text sections.
> -  const unsigned char* ps = pshdrs + shdr_size;
> -  for (unsigned int i = 1; i < shnum; ++i, ps += shdr_size)
> -    {
> -      elfcpp::Shdr<32, big_endian> shdr(ps);
> -      if (shdr.get_sh_type() == elfcpp::SHT_ARM_EXIDX)
> -       {
> -         // Found an .ARM.exidx section, add it to the set of reachable
> -         // sections from its linked text section.
> -         unsigned int text_shndx = this->adjust_shndx(shdr.get_sh_link());
> -         symtab->gc()->add_reference(this, text_shndx, this, i);
> -       }
> -    }
> -}
> -
>  // Update output local symbol count.  Owing to EXIDX entry merging, some local
>  // symbols  will be removed in output.  Adjust output local symbol count
>  // accordingly.  We can only changed the static output local symbol count.  It
> diff --git a/gold/object.cc b/gold/object.cc
> index a631c9937c..160156397d 100644
> --- a/gold/object.cc
> +++ b/gold/object.cc
> @@ -1623,10 +1623,12 @@ Sized_relobj_file<size, big_endian>::do_layout(Symbol_table* symtab,
>               symtab->gc()->worklist().push_back(Section_id(this, i));
>             }
>           // If the section name XXX can be represented as a C identifier
> +          // and the section does not have SHF_LINK_ORDER flag
>           // it cannot be discarded if there are references to
>           // __start_XXX and __stop_XXX symbols.  These need to be
>           // specially handled.
> -         if (is_cident(name))
> +         if (!(shdr.get_sh_flags() & elfcpp::SHF_LINK_ORDER)
> +             && is_cident(name))
>             {
>               symtab->gc()->add_cident_section(name, Section_id(this, i));
>             }
> diff --git a/gold/reloc.cc b/gold/reloc.cc
> index ca54f153a3..f782627f26 100644
> --- a/gold/reloc.cc
> +++ b/gold/reloc.cc
> @@ -35,6 +35,7 @@
>  #include "icf.h"
>  #include "compressed_output.h"
>  #include "incremental.h"
> +#include "gc.h"
>
>  namespace gold
>  {
> @@ -415,6 +416,31 @@ Sized_relobj_file<size, big_endian>::do_gc_process_relocs(Symbol_table* symtab,
>                                          local_symbols);
>          }
>      }
> +
> +  // If --gc-sections is not specified, there is nothing more to do.
> +  // This happens when --icf is used but --gc-sections is not.
> +  if (!parameters->options().gc_sections())
> +    return;
> +
> +  unsigned int shnum = this->shnum();
> +  const unsigned char* pshdrs = this->get_view(this->elf_file_.shoff(),
> +                                              shnum * This::shdr_size,
> +                                              true, true);
> +
> +  // Scan section headers for sections with SHF_LINK_ORDER.  Add references
> +  // to these from the linked text sections.
> +  const unsigned char* ps = pshdrs + This::shdr_size;
> +  for (unsigned int i = 1; i < shnum; ++i, ps += This::shdr_size)
> +    {
> +      This::Shdr shdr(ps);
> +      if (shdr.get_sh_flags() & elfcpp::SHF_LINK_ORDER)
> +       {
> +         // Found an SHF_LINK_ORDER section, add it to the set of reachable
> +         // sections from its linked text section.
> +         unsigned int text_shndx = this->adjust_shndx(shdr.get_sh_link());
> +         symtab->gc()->add_reference(this, text_shndx, this, i);
> +       }
> +    }
>  }
>
>
> --
> 2.12.2.564.g063fe858b8-goog
>