[RFC] [gold] Simplify relocation strategy logic

[RFC] [gold] Simplify relocation strategy logic

[Cary Coutant]

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

Re: [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;
>     }
>

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

[Richard Sandiford]

Cary Coutant <ccoutant@google.com> writes:
>      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;

I don't believe this is correct.  ABSOLUTE_REF is defined as:

    // A reference to the symbol's absolute address.  This includes
    // references that cause an absolute address to be stored in the GOT.
    ABSOLUTE_REF = 1,

and the final symbol reference (by the GOT entry) is absolute
in all these cases.  ("Absolute in GOT" was supposed to mean
"an absolute reference _in_ the global offset table", not "to".)
Whether the reference to the GOT entry itself is absolute, relative
to the GP, or relative to the PC, isn't modelled by these flags.

Does the patch rely on this, or was it just something you noticed
by inspection?

Richard

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

[Cary Coutant]

> I don't believe this is correct.  ABSOLUTE_REF is defined as:
>
>    // A reference to the symbol's absolute address.  This includes
>    // references that cause an absolute address to be stored in the GOT.
>    ABSOLUTE_REF = 1,
>
> and the final symbol reference (by the GOT entry) is absolute
> in all these cases.  ("Absolute in GOT" was supposed to mean
> "an absolute reference _in_ the global offset table", not "to".)
> Whether the reference to the GOT entry itself is absolute, relative
> to the GP, or relative to the PC, isn't modelled by these flags.
>
> Does the patch rely on this, or was it just something you noticed
> by inspection?

My understanding of the ABI is that R_X86_64_GOTPCREL is a pc-relative
relocation to the GOT entry, and R_X86_64_GOTPLT64 is an absolute
reference to a GOT entry. In both cases, I believe the GOT entry
itself is expected to contain an absolute address. I rely on these
flags modeling the reference to the GOT entry, not the GOT entry
itself, and I think they're correct as they stand (although I'll
definitely take a closer look before committing to this). I'll fix the
comments, though.

-cary

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

[Richard Sandiford]

Cary Coutant <ccoutant@google.com> writes:
>> I don't believe this is correct.  ABSOLUTE_REF is defined as:
>>
>>    // A reference to the symbol's absolute address.  This includes
>>    // references that cause an absolute address to be stored in the GOT.
>>    ABSOLUTE_REF = 1,
>>
>> and the final symbol reference (by the GOT entry) is absolute
>> in all these cases.  ("Absolute in GOT" was supposed to mean
>> "an absolute reference _in_ the global offset table", not "to".)
>> Whether the reference to the GOT entry itself is absolute, relative
>> to the GP, or relative to the PC, isn't modelled by these flags.
>>
>> Does the patch rely on this, or was it just something you noticed
>> by inspection?
>
> My understanding of the ABI is that R_X86_64_GOTPCREL is a pc-relative
> relocation to the GOT entry, and R_X86_64_GOTPLT64 is an absolute
> reference to a GOT entry. In both cases, I believe the GOT entry
> itself is expected to contain an absolute address.

Right.

> I rely on these flags modeling the reference to the GOT entry, not the
> GOT entry itself,

In that case, you're changing the meaning of the flags, so I think
you'll need to adjust all other ports as well the comment.  The current
code really was written to the current definition of ABSOLUTE_REF.

Richard

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

[Richard Sandiford]

Richard Sandiford <richard.sandiford@linaro.org> writes:
> Cary Coutant <ccoutant@google.com> writes:
>>> I don't believe this is correct.  ABSOLUTE_REF is defined as:
>>>
>>>    // A reference to the symbol's absolute address.  This includes
>>>    // references that cause an absolute address to be stored in the GOT.
>>>    ABSOLUTE_REF = 1,
>>>
>>> and the final symbol reference (by the GOT entry) is absolute
>>> in all these cases.  ("Absolute in GOT" was supposed to mean
>>> "an absolute reference _in_ the global offset table", not "to".)
>>> Whether the reference to the GOT entry itself is absolute, relative
>>> to the GP, or relative to the PC, isn't modelled by these flags.
>>>
>>> Does the patch rely on this, or was it just something you noticed
>>> by inspection?
>>
>> My understanding of the ABI is that R_X86_64_GOTPCREL is a pc-relative
>> relocation to the GOT entry, and R_X86_64_GOTPLT64 is an absolute
>> reference to a GOT entry. In both cases, I believe the GOT entry
>> itself is expected to contain an absolute address.
>
> Right.

Er, that was "right" to the last bit.  R_X86_64_GOTPLT64 is a GOT offset,
isn't it?  That's GP-relative rather than absolute.

Richard

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

[Cary Coutant]

>>> My understanding of the ABI is that R_X86_64_GOTPCREL is a pc-relative
>>> relocation to the GOT entry, and R_X86_64_GOTPLT64 is an absolute
>>> reference to a GOT entry. In both cases, I believe the GOT entry
>>> itself is expected to contain an absolute address.
>>
>> Right.
>
> Er, that was "right" to the last bit.  R_X86_64_GOTPLT64 is a GOT offset,
> isn't it?  That's GP-relative rather than absolute.

Right, as are GOT32 and GOT64. For the purposes of
relocation_strategy, GP-relative is as good as pc-relative (no dynamic
relocation required). Now I'm wondering why my patch works at all -- I
think we'll need some additional test cases.

> In that case, you're changing the meaning of the flags, so I think
> you'll need to adjust all other ports as well the comment.  The current
> code really was written to the current definition of ABSOLUTE_REF.

Are you sure? It looks to me like Symbol::needs_dynamic_reloc()
assumes that ABSOLUTE_REF describes the location being relocated
rather than the GOT entry that is created as a by-product.

-cary

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

[Cary Coutant]

>> Er, that was "right" to the last bit.  R_X86_64_GOTPLT64 is a GOT offset,
>> isn't it?  That's GP-relative rather than absolute.
>
> Right, as are GOT32 and GOT64. For the purposes of
> relocation_strategy, GP-relative is as good as pc-relative (no dynamic
> relocation required). Now I'm wondering why my patch works at all -- I
> think we'll need some additional test cases.

I have answer to that -- none of the test cases in our testsuite use
GOTPLT64, GOT32, or GOT64 (I checked both opt and debug).

-cary

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

[Richard Sandiford]

Cary Coutant <ccoutant@google.com> writes:
>> In that case, you're changing the meaning of the flags, so I think
>> you'll need to adjust all other ports as well the comment.  The current
>> code really was written to the current definition of ABSOLUTE_REF.
>
> Are you sure? It looks to me like Symbol::needs_dynamic_reloc()
> assumes that ABSOLUTE_REF describes the location being relocated
> rather than the GOT entry that is created as a by-product.

Well, I don't know how much it counts for, but I know I was deliberately
following that model when doing the get_reference_flags() stuff. :-)

I agree it's confusing[*], but as things stand, I don't think
Symbol::needs_dynamic_reloc() inherently assumes what you say.
It depends on the calling context.  If you wanted to call
Symbol::needs_dynamic_reloc() directly from relocate() to decide
whether a GOT reloc itself needs to become dynamic, then yes,
the function would give the wrong answer.  But nothing calls
the function in that context, because GOT relocs themselves
are always resolved statically.  The only question is whether
the GOT _entry_ needs a reloc.

The case where Symbol::needs_dynamic_reloc() _does_ need to describe
the GOT entry itself is for the current definition of use_plt_offset().
That function is called for all relocations (including GOT relocations)
at the beginning of the target's relocate() function.  In the case of
GOT relocs, use_plt_offset() says whether the _GOT entry_ should be
redirected to the PLT.  (This is of course sometimes true for ifuncs,
but should never be true otherwise.)  So I claim that, in the context
of use_plt_offset(), needs_dynamic_reloc() really is telling you about
the GOT entry rather than the GOT reloc itself.

I hope I'm remembering this right...

Richard

[*] and I'd like to think that the confusion predates my changes. :-)
    Don't get me wrong, your plans to clean this up are very welcome to me.
    I just dispute that the current code is wrong.

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

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> The case where Symbol::needs_dynamic_reloc() _does_ need to describe
> the GOT entry itself is for the current definition of use_plt_offset().
> That function is called for all relocations (including GOT relocations)
> at the beginning of the target's relocate() function.  In the case of
> GOT relocs, use_plt_offset() says whether the _GOT entry_ should be
> redirected to the PLT.  (This is of course sometimes true for ifuncs,
> but should never be true otherwise.)

Um, sorry, obviously it's too late for me to be thinking straight.
Of course it's true sometimes for executables.

Richard

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

[Cary Coutant]

> Well, I don't know how much it counts for, but I know I was deliberately
> following that model when doing the get_reference_flags() stuff. :-)

Sorry for not noticing the discrepancy then. When I first added the
ABSOLUTE_REF flag, it was meant to indicate the type of reference at
the location being relocated.

By the way, have you noticed that the RELATIVE_REF flag is returned
for many relocs, but gold never tests for it? I think I'm going to get
rid of it.

> I agree it's confusing[*], but as things stand, I don't think
> Symbol::needs_dynamic_reloc() inherently assumes what you say.
> It depends on the calling context.  If you wanted to call
> Symbol::needs_dynamic_reloc() directly from relocate() to decide
> whether a GOT reloc itself needs to become dynamic, then yes,
> the function would give the wrong answer.  But nothing calls
> the function in that context, because GOT relocs themselves
> are always resolved statically.  The only question is whether
> the GOT _entry_ needs a reloc.

I started this little exercise with the premise that there were only
three strategies: normal, reference to PLT, and dynamic relocation.
(The "normal" case included references to the GOT.) As I worked
through the logic, it became clear that it was (at least
theoretically) possible for a relocation that references a PLT entry
to also require a dynamic relocation (e.g., an absolute reference to a
PLT entry in a position-independent output). I'm not sure if that's
ever a sensible thing, but it seems possible, and that's one of the
cases I support. I suppose I could add "reference to a GOT entry" as
another strategy, but it doesn't seem to simplify any logic (although
it might simplify it conceptually). [On second thought, it might
actually simplify the code at the top of Relocate::relocate(). I'll
play with that idea...]

If the GOT entry itself needs a dynamic reloc, that's already taken
care of in Scan::global(). The GOT entries never have static
relocations, either -- their values are computed during
Output_data_got::Got_entry::write(). The only thing we care about when
using get_reference_flags() is whether the relocation we're applying
should reference the address of a PLT entry, a GOT entry, or the
actual symbol value, and whether a dynamic relocation is necessary at
that location.

> The case where Symbol::needs_dynamic_reloc() _does_ need to describe
> the GOT entry itself is for the current definition of use_plt_offset().
> That function is called for all relocations (including GOT relocations)
> at the beginning of the target's relocate() function.  In the case of
> GOT relocs, use_plt_offset() says whether the _GOT entry_ should be
> redirected to the PLT.  (This is of course sometimes true for ifuncs,
> but should never be true otherwise.)  So I claim that, in the context
> of use_plt_offset(), needs_dynamic_reloc() really is telling you about
> the GOT entry rather than the GOT reloc itself.

We never call Symbol::use_plt_offset() when relocating a GOT entry. I
think you're confusing that with the use_plt_offset_ flag in the
Got_entry. That bit is set explicitly for GOT entries that reference a
PLT entry directly, and as far as I can tell, it is set only for IFUNC
symbols.

> [*] and I'd like to think that the confusion predates my changes. :-)
>    Don't get me wrong, your plans to clean this up are very welcome to me.
>    I just dispute that the current code is wrong.

Yes, it was plenty confusing before (some was my fault for sure, but I
think some is just intrinsic complexity), and will probably always
remain so. Our best hope is just to minimize how confusing it is. :-)

Oh, and thanks to the newly-simplified logic and our subsequent
discussion, I've discovered what I think is another lurking bug --
gold says "no dynamic reloc necessary" for any reference to an
absolute symbol, but it doesn't consider the case of a pc-relative
reference to an absolute symbol in position-independent code!

-cary

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

[Richard Sandiford]

Cary Coutant <ccoutant@google.com> writes:
>> The case where Symbol::needs_dynamic_reloc() _does_ need to describe
>> the GOT entry itself is for the current definition of use_plt_offset().
>> That function is called for all relocations (including GOT relocations)
>> at the beginning of the target's relocate() function.  In the case of
>> GOT relocs, use_plt_offset() says whether the _GOT entry_ should be
>> redirected to the PLT.  (This is of course sometimes true for ifuncs,
>> but should never be true otherwise.)  So I claim that, in the context
>> of use_plt_offset(), needs_dynamic_reloc() really is telling you about
>> the GOT entry rather than the GOT reloc itself.
>
> We never call Symbol::use_plt_offset() when relocating a GOT entry. I
> think you're confusing that with the use_plt_offset_ flag in the
> Got_entry.

No.  What I meant was: we call Symbol::use_plt_offset() for all GOT
_relocations_.  At that stage, we're trying to work out what value the
relocation symbol resolves to, and in the case of GOT relocations,
that symbol value is the value that should be stored in the GOT _entry_.
The relocation-specific handling will then convert that (modified)
symbol value into a GOT offset.

So when we call Symbol::use_plt_offset() for GOT _relocations_,
it is effectively telling us whether the GOT _entry_ should be
directed to the PLT.    Symbol::use_plt_offset() calls
Symbol::needs_dynamic_reloc(), so in this case,
Symbol::needs_dynamic_reloc() is also telling us about the GOT entry.

Richard