[RFC] [gold] Simplify relocation strategy logic

[Cary Coutant]

[fixed Andreas' email address]

On Fri, Mar 11, 2011 at 6:41 PM, Cary Coutant <ccoutant@google.com> wrote:
> Paul recently noticed that gold was generating PLT entries for lots of
> symbols that didn't actually need PLT entries. I took a look at the
> code and found an inconsistency in the logic for deciding when to
> create a PLT entry (when scanning relocations) and the logic for
> deciding whether or not to use the PLT entry as the target of the
> relocation (when applying relocations). I suggested a small change
> that would fix the problem, but Ian countered with a suggestion that
> the logic should be cleaned up so that there's one new routine where
> we used to use three -- needs_plt_entry(), needs_dynamic_reloc(), and
> use_plt_offset().
>
> Here's a start at doing that for x86_64 only, for your review and
> comments. I've left in the old logic, so that it runs it side-by-side
> with the new logic and prints any differences as warnings. (The
> ChangeLog snippet below ignores those routines I've put in just for
> the consistency checks.) Before this could be committed, I'll need to
> update the other targets besides x86_64. I'll also need some help
> testing the sparc, powerpc, and arm targets at that point.
>
> The new routine, Symbol::relocation_strategy, returns two bit flags
> that indicate whether the relocation should target a PLT entry (vs.
> the symbol itself), and whether a dynamic relocation will be
> necessary. I added two more Reference_flags to classify relocations as
> PLT_REF (those that explicitly ask for a PLT offset) and GOT_REF
> (those that explicitly target a GOT entry, which effectively makes it
> a local reference). I also fixed a couple of mis-classified
> relocations in the x86_64 version of Scan::get_reference_flags().
>
> I get two sets of differences between the old logic and new logic
> (using the gold testsuite). The first set are the cases I set out to
> fix, where we used to generate unused PLT entries and now don't. These
> all show up in the test log as messages of this form (always an
> R_X86_64_64 relocation from the data segment to a function symbol):
>
> gcctestdir/ld: warning: strategy conflict: needs_plt_entry() true,
> strategy DYNAMIC (r_type 1, sym __gxx_personality_v0)
> gcctestdir/ld: warning: strategy conflict: needs_plt_entry() true,
> strategy DYNAMIC (r_type 1, sym f10())
>
> The second set are from the TLS tests, and are all identical:
>
> gcctestdir/ld: warning: strategy conflict: reloc implies PLT, strategy
> NORMAL (r_type 4, sym __tls_get_addr)
>
> These result from an R_X86_64_PLT32 referencing the undefined symbol
> __tls_get_addr, which the new logic claims does not need either a PLT
> entry or a dynamic relocation. In truth, it doesn't, because we always
> optimize away the calls to __tls_get_addr when building an executable.
> When building a shared library, the new logic does build the PLT
> entry. I could check for gsym->is_defined_by_abi(), or I could fix it
> so that the PLT32 relocation (now flagged as PLT_REF) always creates
> the PLT entry, but it's not necessary in this case, and I can't think
> of any other cases where it would be necessary.
>
> -cary
>
>
>        * symtab.cc (Symbol::relocation_strategy): New function.
>        * symtab.h (Symbol::Reference_flags): Add PLT_REF, GOT_REF.
>        (Symbol::Relocation_strategy): New enum type.
>        * x86_64.cc (Scan::get_reference_flags): Return extra flags
>        for certain relocations.
>        (Scan::global): Use Symbol::relocation_strategy.
>        (Relocate::relocate): Likewise.
>
> Index: symtab.cc
> ===================================================================
> RCS file: /cvs/src/src/gold/symtab.cc,v
> retrieving revision 1.149
> diff -u -p -r1.149 symtab.cc
> --- symtab.cc   10 Mar 2011 01:31:32 -0000      1.149
> +++ symtab.cc   12 Mar 2011 01:44:08 -0000
> @@ -200,6 +200,88 @@ Symbol::allocate_base_common(Output_data
>   this->u_.in_output_data.offset_is_from_end = false;
>  }
>
> +// Determine how a relocation should be applied to this symbol.
> +// The bits in FLAGS indicates the type of reference implied by
> +// the relocation (see Symbol::Reference_flags).  Returns flags
> +// RELOCATE_PLT for a relocation that should relocate to the
> +// address of a PLT entry, and RELOCATE_DYNAMIC if a dynamic
> +// relocation is required.  Returns 0 if the relocation can be
> +// applied normally.
> +
> +int
> +Symbol::relocation_strategy(int flags) const
> +{
> +  int needs_dyn = 0;
> +  int needs_plt = 0;
> +
> +  if ((flags & FUNCTION_CALL)
> +      && this->is_weak_undefined()
> +      && !parameters->doing_static_link())
> +    {
> +      // If this is a call to a weak undefined symbol, we need to use
> +      // a PLT entry, because the symbol may be defined by a library
> +      // loaded at runtime.
> +      needs_plt = RELOCATE_PLT;
> +    }
> +  else if (this->is_undefined() && !parameters->options().shared())
> +    {
> +      // A reference to an undefined symbol from an executable (when not
> +      // generating an error) will resolve to 0.
> +      return 0;
> +    }
> +  else if (this->is_absolute())
> +    {
> +      // A reference to an absolute symbol can be relocated statically.
> +      return 0;
> +    }
> +
> +  if (this->type() == elfcpp::STT_GNU_IFUNC)
> +    {
> +      // An STT_GNU_IFUNC symbol always needs a PLT entry, even when
> +      // doing a static link.
> +      needs_plt = RELOCATE_PLT;
> +    }
> +
> +  // A symbol may need a PLT entry or a dynamic relocation if it is
> +  // defined in a dynamic object, undefined (when not building an
> +  // executable -- references to undefs from an executable are handled
> +  // above), or subject to pre-emption.
> +  bool is_dynamic = (this->is_from_dynobj()
> +                    || this->is_undefined()
> +                    || this->is_preemptible());
> +
> +  if ((flags & ABSOLUTE_REF)
> +      && parameters->options().output_is_position_independent())
> +    {
> +      // An absolute reference within a position-independent output file
> +      // will need a dynamic relocation.
> +      needs_dyn = RELOCATE_DYNAMIC;
> +    }
> +
> +  if (is_dynamic
> +      && (this->is_func() || (flags & PLT_REF))
> +      && !(flags & GOT_REF)
> +      && !parameters->doing_static_link())
> +    {
> +      // A function call to a dynamic symbol will use a PLT entry.
> +      // Exception:  If we're already planning to create a dynamic
> +      // relocation and the relocation doesn't specifically ask for
> +      // the PLT offset, we can ignore the PLT entry.
> +      if (!needs_dyn || (flags & PLT_REF))
> +        needs_plt = RELOCATE_PLT;
> +    }
> +  else if (is_dynamic
> +          && !(flags & GOT_REF)
> +          && !parameters->doing_static_link())
> +    {
> +      // Any other reference to a dynamic, undefined, or pre-emptable
> +      // symbol will need a dynamic relocation.
> +      needs_dyn = RELOCATE_DYNAMIC;
> +    }
> +
> +  return needs_plt | needs_dyn;
> +}
> +
>  // Initialize the fields in Sized_symbol for SYM in OBJECT.
>
>  template<int size>
> Index: symtab.h
> ===================================================================
> RCS file: /cvs/src/src/gold/symtab.h,v
> retrieving revision 1.118
> diff -u -p -r1.118 symtab.h
> --- symtab.h    10 Mar 2011 01:31:32 -0000      1.118
> +++ symtab.h    12 Mar 2011 01:44:08 -0000
> @@ -627,9 +627,31 @@ class Symbol
>     // A TLS-related reference.
>     TLS_REF = 4,
>     // A reference that can always be treated as a function call.
> -    FUNCTION_CALL = 8
> +    FUNCTION_CALL = 8,
> +    // A specific reference to a PLT entry.
> +    PLT_REF = 0x10,
> +    // A reference to a GOT entry.
> +    GOT_REF = 0x20
>   };
>
> +  // When scanning and applying relocations, we need to know whether
> +  // the relocation should be applied normally, as a reference to a PLT
> +  // entry, or by creating a dynamic relocation.  These bits may be
> +  // or'ed together.  0 means the relocation should be applied normally.
> +  enum Relocation_strategy
> +  {
> +    // Relocate to the address of a PLT entry.
> +    RELOCATE_PLT = 1,
> +    // Create a dynamic relocation.
> +    RELOCATE_DYNAMIC = 2
> +  };
> +
> +  // Determine how a relocation should be applied to this symbol.
> +  // The bits in FLAGS indicates the type of reference implied by
> +  // the relocation:  absolute, relative, TLS, function call.
> +  int
> +  relocation_strategy(int flags) const;
> +
>   // Given a direct absolute or pc-relative static relocation against
>   // the global symbol, this function returns whether a dynamic relocation
>   // is needed.
> Index: x86_64.cc
> ===================================================================
> RCS file: /cvs/src/src/gold/x86_64.cc,v
> retrieving revision 1.121
> diff -u -p -r1.121 x86_64.cc
> --- x86_64.cc   15 Dec 2010 15:35:27 -0000      1.121
> +++ x86_64.cc   12 Mar 2011 01:44:08 -0000
> @@ -1270,15 +1270,19 @@ Target_x86_64::Scan::get_reference_flags
>
>     case elfcpp::R_X86_64_PLT32:
>     case elfcpp::R_X86_64_PLTOFF64:
> -      return Symbol::FUNCTION_CALL | Symbol::RELATIVE_REF;
> +      return Symbol::FUNCTION_CALL | Symbol::RELATIVE_REF | Symbol::PLT_REF;
>
>     case elfcpp::R_X86_64_GOT64:
>     case elfcpp::R_X86_64_GOT32:
> +      // Absolute in GOT.
> +      return Symbol::ABSOLUTE_REF | Symbol::GOT_REF;
>     case elfcpp::R_X86_64_GOTPCREL64:
>     case elfcpp::R_X86_64_GOTPCREL:
> +      // PC-Relative in GOT.
> +      return Symbol::RELATIVE_REF | Symbol::GOT_REF;
> +
>     case elfcpp::R_X86_64_GOTPLT64:
> -      // Absolute in GOT.
> -      return Symbol::ABSOLUTE_REF;
> +      return Symbol::ABSOLUTE_REF | Symbol::GOT_REF;
>
>     case elfcpp::R_X86_64_TLSGD:            // Global-dynamic
>     case elfcpp::R_X86_64_GOTPC32_TLSDESC:  // Global-dynamic (from ~oliva url)
> @@ -1764,6 +1768,85 @@ Target_x86_64::Scan::global_reloc_may_be
>           || possible_function_pointer_reloc(r_type));
>  }
>
> +// TEMPORARY: Sanity check for relocation_strategy().
> +// Compare the strategy with the older logic.
> +
> +const char*
> +strategy_name(int strategy)
> +{
> +  switch (strategy)
> +    {
> +    case 0:
> +      return "NORMAL";
> +    case Symbol::RELOCATE_PLT:
> +      return "PLT";
> +    case Symbol::RELOCATE_DYNAMIC:
> +      return "DYNAMIC";
> +    case Symbol::RELOCATE_PLT | Symbol::RELOCATE_DYNAMIC:
> +      return "PLT+DYN";
> +    default:
> +      gold_unreachable();
> +    }
> +}
> +
> +void
> +check_plt_strategy(const Symbol* gsym, int r_type, int strategy)
> +{
> +  bool needs_plt = gsym->needs_plt_entry();
> +  bool strat_plt = (strategy & Symbol::RELOCATE_PLT) != 0;
> +  if (needs_plt != strat_plt)
> +    gold_warning("strategy conflict: needs_plt_entry() %s, "
> +                "strategy %s "
> +                "(r_type %d, sym %s)",
> +                needs_plt ? "true" : "false",
> +                strategy_name(strategy),
> +                r_type, gsym->name());
> +}
> +
> +void
> +check_forced_plt_strategy(const Symbol* gsym, int r_type, int strategy)
> +{
> +  bool needs_plt = true;
> +  bool strat_plt = (strategy & Symbol::RELOCATE_PLT) != 0;
> +  if (needs_plt != strat_plt)
> +    gold_warning("strategy conflict: reloc implies PLT, "
> +                "strategy %s "
> +                "(r_type %d, sym %s)",
> +                strategy_name(strategy),
> +                r_type, gsym->name());
> +}
> +
> +void
> +check_dynamic_strategy(const Symbol* gsym, int r_type, int flags, int strategy)
> +{
> +  bool needs_dyn_reloc = gsym->needs_dynamic_reloc(flags);
> +  bool strat_dyn = (strategy & Symbol::RELOCATE_DYNAMIC) != 0;
> +  if (needs_dyn_reloc != strat_dyn)
> +    gold_warning("strategy conflict: needs_dynamic_reloc() %s, "
> +                "strategy %s "
> +                "(r_type %d, sym %s)",
> +                needs_dyn_reloc ? "true" : "false",
> +                strategy_name(strategy),
> +                r_type, gsym->name());
> +}
> +
> +void
> +check_use_plt_strategy(const Symbol* gsym, int r_type, int flags, int strategy)
> +{
> +  bool use_plt = false;
> +  if (gsym != NULL)
> +    use_plt = gsym->use_plt_offset(flags);
> +  bool strat_plt = (strategy & Symbol::RELOCATE_PLT) != 0;
> +  if (use_plt != strat_plt)
> +    gold_warning("strategy conflict: use_plt_offset() %s, "
> +                "strategy %s "
> +                "(r_type %d, sym %s)",
> +                use_plt ? "true" : "false",
> +                strategy_name(strategy),
> +                r_type,
> +                gsym == NULL ? "(local)" : gsym->name());
> +}
> +
>  // Scan a relocation for a global symbol.
>
>  inline void
> @@ -1777,9 +1860,10 @@ Target_x86_64::Scan::global(Symbol_table
>                             unsigned int r_type,
>                             Symbol* gsym)
>  {
> -  // A STT_GNU_IFUNC symbol may require a PLT entry.
> -  if (gsym->type() == elfcpp::STT_GNU_IFUNC
> -      && this->reloc_needs_plt_for_ifunc(object, r_type))
> +  int strategy = gsym->relocation_strategy(Scan::get_reference_flags(r_type));
> +
> +  // Make a PLT entry if necessary.
> +  if (strategy & Symbol::RELOCATE_PLT)
>     target->make_plt_entry(symtab, layout, gsym);
>
>   switch (r_type)
> @@ -1795,19 +1879,21 @@ Target_x86_64::Scan::global(Symbol_table
>     case elfcpp::R_X86_64_16:
>     case elfcpp::R_X86_64_8:
>       {
> -        // Make a PLT entry if necessary.
> -        if (gsym->needs_plt_entry())
> +        check_plt_strategy(gsym, r_type, strategy);
> +        if (strategy & Symbol::RELOCATE_PLT
> +            && gsym->is_from_dynobj()
> +            && !parameters->options().shared())
>           {
> -            target->make_plt_entry(symtab, layout, gsym);
>             // Since this is not a PC-relative relocation, we may be
>             // taking the address of a function. In that case we need to
>             // set the entry in the dynamic symbol table to the address of
>             // the PLT entry.
> -            if (gsym->is_from_dynobj() && !parameters->options().shared())
> -              gsym->set_needs_dynsym_value();
> +            gsym->set_needs_dynsym_value();
>           }
>         // Make a dynamic relocation if necessary.
> -        if (gsym->needs_dynamic_reloc(Scan::get_reference_flags(r_type)))
> +        check_dynamic_strategy(gsym, r_type, Scan::get_reference_flags(r_type),
> +                              strategy);
> +        if (strategy & Symbol::RELOCATE_DYNAMIC)
>           {
>             if (gsym->may_need_copy_reloc())
>               {
> @@ -1860,11 +1946,11 @@ Target_x86_64::Scan::global(Symbol_table
>     case elfcpp::R_X86_64_PC16:
>     case elfcpp::R_X86_64_PC8:
>       {
> -        // Make a PLT entry if necessary.
> -        if (gsym->needs_plt_entry())
> -          target->make_plt_entry(symtab, layout, gsym);
> +        check_plt_strategy(gsym, r_type, strategy);
> +        check_dynamic_strategy(gsym, r_type, Scan::get_reference_flags(r_type),
> +                              strategy);
>         // Make a dynamic relocation if necessary.
> -        if (gsym->needs_dynamic_reloc(Scan::get_reference_flags(r_type)))
> +        if (strategy & Symbol::RELOCATE_DYNAMIC)
>           {
>             if (gsym->may_need_copy_reloc())
>               {
> @@ -1948,16 +2034,14 @@ Target_x86_64::Scan::global(Symbol_table
>     case elfcpp::R_X86_64_PLT32:
>       // If the symbol is fully resolved, this is just a PC32 reloc.
>       // Otherwise we need a PLT entry.
> -      if (gsym->final_value_is_known())
> -       break;
>       // If building a shared library, we can also skip the PLT entry
>       // if the symbol is defined in the output file and is protected
>       // or hidden.
> -      if (gsym->is_defined()
> -          && !gsym->is_from_dynobj()
> -          && !gsym->is_preemptible())
> -       break;
> -      target->make_plt_entry(symtab, layout, gsym);
> +      if (!gsym->final_value_is_known()
> +          && !(gsym->is_defined()
> +               && !gsym->is_from_dynobj()
> +               && !gsym->is_preemptible()))
> +       check_forced_plt_strategy(gsym, r_type, strategy);
>       break;
>
>     case elfcpp::R_X86_64_GOTPC32:
> @@ -2264,10 +2348,13 @@ Target_x86_64::Relocate::relocate(const
>
>   const Sized_relobj<64, false>* object = relinfo->object;
>
> +  int strategy = 0;
> +  if (gsym != NULL)
> +    strategy = gsym->relocation_strategy(Scan::get_reference_flags(r_type));
> +
>   // Pick the value to use for symbols defined in the PLT.
>   Symbol_value<64> symval;
> -  if (gsym != NULL
> -      && gsym->use_plt_offset(Scan::get_reference_flags(r_type)))
> +  if (strategy & Symbol::RELOCATE_PLT)
>     {
>       symval.set_output_value(target->plt_section()->address()
>                              + gsym->plt_offset());
> @@ -2315,6 +2402,8 @@ Target_x86_64::Relocate::relocate(const
>       break;
>
>     default:
> +      check_use_plt_strategy(gsym, r_type, Scan::get_reference_flags(r_type),
> +                            strategy);
>       break;
>     }
>