Re: [PATCH] PR gold/17640

[Cary Coutant <ccoutant at google dot com>]

+ // If we convert this from
+ // mov foo@GOT(%reg), %reg
+ // to
+ // lea foo@GOTOFF(%reg), %reg
+ // in Relocate::relocate, then there is nothing to do here.
+ // Avoid converting _DYNAMIC, because its address may be used.

I'm sorry, but I still don't understand the rationale for treating
_DYNAMIC specially. If you convert all of the references to @GOTOFF,
what's the point of having the GOT entry? If the loader is going to
use it, how does it find it?

At any rate, the comment here is wrong, because you *will* convert mov
to lea for this reference -- all you're doing here is keeping the GOT
entry, even though you're not going to use it.

+    && strcmp(gsym->name(), "_DYNAMIC"))

strcmp doesn't return a boolean; I'd prefer to see an explicit "!= 0".

And later, in Relocate::relocate, you cut & pasted the comment from Scan::local:

+      // If the relocation symbol isn't IFUNC,
+      // and is local, then we convert
+      // mov foo@GOT(%reg), %reg
+      // to
+      // lea foo@GOTOFF(%reg), %reg

But here, you're checking for either local or global relocations.

+      if (rel.get_r_offset() >= 2
+         && view[-2] == 0x8b
+         && ((gsym == NULL && !psymval->is_ifunc_symbol())
+             || (gsym != NULL
+                 && gsym->type() != elfcpp::STT_GNU_IFUNC
+                 && !gsym->is_from_dynobj()
+                 && !gsym->is_undefined ()
+                 && (!parameters->options().shared()
+                     || (gsym->visibility() != elfcpp::STV_DEFAULT
+                         && gsym->visibility() != elfcpp::STV_PROTECTED)
+                     || parameters->options().Bsymbolic())
+                 && !gsym->is_preemptible())))

This is a complicated condition, and it shares a lot with the
condition in Scan::global(). I'd still like to see the common parts of
the two refactored into a predicate function.

-cary