Re: [RFC] [gold] Simplify relocation strategy logic

[Cary Coutant <ccoutant at google dot com>]

[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;
> ? ? }
>