Re: [gold][aarch64] PR gold/19163: zero-out ABS64/32/16/ in .rela.dyn.

[Han Shen <shenhan at google dot com>]

Hi Cary, thanks for providing another patch, it seems much cleaner and neat.

I've tested this on aarch64 box, here is a few comments

diff --git a/gold/aarch64.cc b/gold/aarch64.cc
index 2dcd620..723d244 100644
--- a/gold/aarch64.cc
+++ b/gold/aarch64.cc
@@ -6911,16 +6911,38 @@ Target_aarch64<size, big_endian>::Relocate::relocate(
       break;

     case elfcpp::R_AARCH64_ABS64:
+      if (!parameters->options().apply_dynamic_relocs()

>> Han: we need to test if gsym is NULL, in one of my cases, gsym is NULL and gold crashes.

+          && parameters->options().output_is_position_independent()
+          && gsym->needs_dynamic_reloc(reloc_property->reference_flags())
+          && !gsym->can_use_relative_reloc(false))
+        // We have generated an absolute dynamic relocation, so do not
+        // apply the relocation statically. (Works around bugs in older
+        // Android dynamic linkers.)

>> Han: do we need to explicitly write zeros to this location instead of just do nothing? (Although I've checked the values of the resulting binary, the location contains all zeros)

+        break;
       reloc_status = Reloc::template rela_ua<64>(
  view, object, psymval, addend, reloc_property);
       break;

     case elfcpp::R_AARCH64_ABS32:

>> Han: similar to R_AARCH64_ABS64

+      if (!parameters->options().apply_dynamic_relocs()
+          && parameters->options().output_is_position_independent()
+          && gsym->needs_dynamic_reloc(reloc_property->reference_flags()))
+        // We have generated an absolute dynamic relocation, so do not
+        // apply the relocation statically. (Works around bugs in older
+        // Android dynamic linkers.)
+        break;
       reloc_status = Reloc::template rela_ua<32>(
  view, object, psymval, addend, reloc_property);
       break;

     case elfcpp::R_AARCH64_ABS16:

>> Han: similar to R_AARCH64_ABS64

+      if (!parameters->options().apply_dynamic_relocs()
+          && parameters->options().output_is_position_independent()
+          && gsym->needs_dynamic_reloc(reloc_property->reference_flags()))
+        // We have generated an absolute dynamic relocation, so do not
+        // apply the relocation statically. (Works around bugs in older
+        // Android dynamic linkers.)
+        break;
       reloc_status = Reloc::template rela_ua<16>(
  view, object, psymval, addend, reloc_property);
       break;
diff --git a/gold/options.h b/gold/options.h
index 4d65225..6ef53cb 100644
--- a/gold/options.h
+++ b/gold/options.h
@@ -644,6 +644,10 @@ class General_options
       N_("Allow unresolved references in shared libraries"),
       N_("Do not allow unresolved references in shared libraries"));

+  DEFINE_bool(apply_dynamic_relocs, options::TWO_DASHES, '\0', true,
+      N_("Write link-time values before dynamic relocations"),
+      N_("(aarch64 only) Write zeroes for values with dynamic relocations"));
+
   DEFINE_bool(as_needed, options::TWO_DASHES, '\0', false,
       N_("Only set DT_NEEDED for shared libraries if used"),
       N_("Always DT_NEEDED for shared libraries"));


On Wed, Oct 28, 2015 at 5:29 PM, Cary Coutant <ccoutant@gmail.com> wrote:
>> 2015-10-26  Han Shen  <shenhan@google.com>
>>
>>         PR gold/19163 - zero out ABS64/32/16 in .rela.dyn.
>>
>>         * aarch64.cc (AArch64_relobj::ZeroOutRecord): New data struct.
>>         (AArch64_relobj::zero_out_list_): New member.
>>         (AArch64_relobj::ZeroOutIter): New typedef.
>>         (AArch64_relobj::ZeroOutList): Likewise.
>>         (AArch64_relobj::zero_out_relocs): New method.
>>         (AArch64_relobj::do_relocate_sections): Add hook to call
>>         zero_out_relocs.
>>         (AArch64_relocate_functions::zero_out): New helper method.
>>         (Target_aarch64::Scan::global): Record global symbols that are
>>         added to .rela.dyn.
>
> It doesn't seem necessary to me to make a list of all those
> relocations, then apply them, then zero them out. Rather than make a
> list, how about just skipping the relocation when applicable?
>
> I'd also like to see this under the control of an option, so that we
> only need to use it for as long as the buggy dynamic linkers are out
> there.
>
> The attached patch adds a --no-apply-dynamic-relocs flag. When set,
> Relocate::relocate() will skip applying the link-time relocation for
> those cases where we've generated an absolute dynamic relocation.
> Please give this a try; if it works, I'd prefer this approach.
>
> -cary
>
>
> 2015-10-28  Cary Coutant  <ccoutant@gmail.com>
>
> gold/
>         * aarch64.cc (Target_aarch64::Relocate::relocate): Don't apply
>         certain relocations if --no-apply-dynamic-relocs is set.
>         * options.h (--apply-dynamic-relocs): New aarch64-specific option.

Han Shen