Re: [AArch64] PR18270, fix handling of GOT entry for local symbol

Re: [AArch64] PR18270, fix handling of GOT entry for local symbol

[Jiong Wang]

2015-04-20 17:22 GMT+01:00 Jiong Wang <jiong.wang@arm.com>:
>
> I reproduced on current binutils-gdb HEAD. The reason is as what RTH has
> explained we are handling GOT entry for local symbol incorrectly thus
> that relocation still relocate the symbol itself instead of the
> associated GOT entry. While the symbol itself may contains value not
> 8bytes aligned, then trigger alignment check errors.
>
> Even worse, if that symbol contains a value 8bytes aligned, then we pass
> the later alignment check, report nothing, and generate buggy binary
> silently.
>
> The patch will handle GOT entry for local symbol as the following:
>
>   * apply the relocation using GOT entry address.
>   * generate a R_AARCH64_RELATIVE relocation for the GOT entry, so the local
>     symbol's address could be updated with the shared object's runtime base address.
>
> OK for trunk?
>
> no regression on binutils-gdb native test.
> pass the testcase given by David.
> pass my small "exectuable + .so" execution test, verified the runtime behavior is OK.
>


Sorry, should be PR18270.

2015-04-20  Jiong. Wang  <jiong.wang@arm.com>

bfd/
  PR18270
  * elfnn-aarch64.c (elfNN_aarch64_size_dynamic): Count local symbol for
  GOT_NORMAL for both sgot/srelgot section.
  (elfNN_aarch64_final_link_relocate): Relocate against GOT entry
  address and generate necessary runtime relocation for GOT entry.

Regards,
Jiong

Re: [AArch64] PR18270, fix handling of GOT entry for local symbol

[Nicholas Clifton]

Hi Jiong,

> bfd/
>    PR18270
>    * elfnn-aarch64.c (elfNN_aarch64_size_dynamic): Count local symbol for
>    GOT_NORMAL for both sgot/srelgot section.
>    (elfNN_aarch64_final_link_relocate): Relocate against GOT entry
>    address and generate necessary runtime relocation for GOT entry.

Approved - but ... I do not like having calls to abort() inside a 
library.  It rarely helps the user.  It would be better I think to 
replace them with calls to bdf_error_handler, providing an error message 
that tells the user that there has been an internal error.

Cheers
   Nick

Re: [AArch64] PR18270, fix handling of GOT entry for local symbol

[Jiong Wang]

[-- Attachment #1: Type: text/plain, Size: 2060 bytes --]

2015-04-24 15:23 GMT+01:00 Nicholas Clifton <nickc@redhat.com>:
> Hi Jiong,
>
>> bfd/
>>    PR18270
>>    * elfnn-aarch64.c (elfNN_aarch64_size_dynamic): Count local symbol for
>>    GOT_NORMAL for both sgot/srelgot section.
>>    (elfNN_aarch64_final_link_relocate): Relocate against GOT entry
>>    address and generate necessary runtime relocation for GOT entry.
>
>
> Approved - but ... I do not like having calls to abort() inside a library.
> It rarely helps the user.  It would be better I think to replace them with
> calls to bdf_error_handler, providing an error message that tells the user
> that there has been an internal error.

Nick,

  Thanks for the review.

  Agree, we should give more error information here. I was copying similar
  code from x86.

  Updated the abort code into.

+       if (locals == NULL)
+         {
+           int howto_index = bfd_r_type - BFD_RELOC_AARCH64_RELOC_START;
+           (*_bfd_error_handler)
+             (_("%B: Local symbol descriptor table be NULL when applying "
+                "relocation %s against local symbol"),
+              input_bfd, elfNN_aarch64_howto_table[howto_index].name);
+           abort ();
+         }

  Now, it's giving more information while still use abort because,
    * Let the linker stop earlier, as missing target private local
symbol descriptor table
      is quite serious and internal error. It's unlike gas parse .s
file where we want the
      assembler to report as many errors as they can in one time.

    * I haven't found proper return value here.
elfNN_aarch64_final_link_relocate's
      return type is bfd_reloc_status_type, no proper value to return,
it most proper
      maybe "bfd_reloc_other" but still not very good. And I also
found current AArch64
      code return "FALSE" in this function which is wrong, "FALSE"
equals 0 which is
      bfd_reloc_ok, also we also invoke bfd_set_error so that the
linker finally know there
      is something wrong happen and stop in later stage.

 Have checked in the revised patch.

 Thanks,
 Regards,
 Jiong

[-- Attachment #2: new.patch --]
[-- Type: text/x-diff, Size: 4148 bytes --]

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index 2a6b6a4..367b5ec 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,3 +1,11 @@
+2015-04-24  Jiong Wang  <jiong.wang@arm.com>
+
+	PR ld/18270
+	* elfnn-aarch64.c (elfNN_aarch64_size_dynamic): Count local symbol for
+	GOT_NORMAL for both sgot/srelgot section.
+	(elfNN_aarch64_final_link_relocate): Relocate against GOT entry address
+	and generate necessary runtime relocation for GOT entry.
+
 2015-04-24  H.J. Lu  <hongjiu.lu@intel.com>
 
 	PR binutils/18209
diff --git a/bfd/elfnn-aarch64.c b/bfd/elfnn-aarch64.c
index c98987b..c252b13 100644
--- a/bfd/elfnn-aarch64.c
+++ b/bfd/elfnn-aarch64.c
@@ -4447,10 +4447,11 @@ elfNN_aarch64_final_link_relocate (reloc_howto_type *howto,
   bfd_reloc_code_real_type new_bfd_r_type;
   unsigned long r_symndx;
   bfd_byte *hit_data = contents + rel->r_offset;
-  bfd_vma place;
+  bfd_vma place, off;
   bfd_signed_vma signed_addend;
   struct elf_aarch64_link_hash_table *globals;
   bfd_boolean weak_undef_p;
+  asection *base_got;
 
   globals = elf_aarch64_hash_table (info);
 
@@ -4490,8 +4491,6 @@ elfNN_aarch64_final_link_relocate (reloc_howto_type *howto,
     {
       asection *plt;
       const char *name;
-      asection *base_got;
-      bfd_vma off;
 
       if ((input_section->flags & SEC_ALLOC) == 0
 	  || h->plt.offset == (bfd_vma) -1)
@@ -4866,6 +4865,58 @@ elfNN_aarch64_final_link_relocate (reloc_howto_type *howto,
 	  value = _bfd_aarch64_elf_resolve_relocation (bfd_r_type, place, value,
 						       0, weak_undef_p);
 	}
+      else
+      {
+	struct elf_aarch64_local_symbol *locals
+	  = elf_aarch64_locals (input_bfd);
+
+	if (locals == NULL)
+	  {
+	    int howto_index = bfd_r_type - BFD_RELOC_AARCH64_RELOC_START;
+	    (*_bfd_error_handler)
+	      (_("%B: Local symbol descriptor table be NULL when applying "
+		 "relocation %s against local symbol"),
+	       input_bfd, elfNN_aarch64_howto_table[howto_index].name);
+	    abort ();
+	  }
+
+	off = symbol_got_offset (input_bfd, h, r_symndx);
+	base_got = globals->root.sgot;
+	bfd_vma got_entry_addr = (base_got->output_section->vma
+				  + base_got->output_offset + off);
+
+	if (!symbol_got_offset_mark_p (input_bfd, h, r_symndx))
+	  {
+	    bfd_put_64 (output_bfd, value, base_got->contents + off);
+
+	    if (info->shared)
+	      {
+		asection *s;
+		Elf_Internal_Rela outrel;
+
+		/* For local symbol, we have done absolute relocation in static
+		   linking stageh. While for share library, we need to update
+		   the content of GOT entry according to the share objects
+		   loading base address. So we need to generate a
+		   R_AARCH64_RELATIVE reloc for dynamic linker.  */
+		s = globals->root.srelgot;
+		if (s == NULL)
+		  abort ();
+
+		outrel.r_offset = got_entry_addr;
+		outrel.r_info = ELFNN_R_INFO (0, AARCH64_R (RELATIVE));
+		outrel.r_addend = value;
+		elf_append_rela (output_bfd, s, &outrel);
+	      }
+
+	    symbol_got_offset_mark (input_bfd, h, r_symndx);
+	  }
+
+	/* Update the relocation value to GOT entry addr as we have transformed
+	   the direct data access into indirect data access through GOT.  */
+	value = got_entry_addr;
+      }
+
       break;
 
     case BFD_RELOC_AARCH64_TLSGD_ADR_PAGE21:
@@ -7521,7 +7572,8 @@ elfNN_aarch64_size_dynamic_sections (bfd *output_bfd ATTRIBUTE_UNUSED,
 		  htab->root.sgot->size += GOT_ENTRY_SIZE * 2;
 		}
 
-	      if (got_type & GOT_TLS_IE)
+	      if (got_type & GOT_TLS_IE
+		  || got_type & GOT_NORMAL)
 		{
 		  locals[i].got_offset = htab->root.sgot->size;
 		  htab->root.sgot->size += GOT_ENTRY_SIZE;
@@ -7531,10 +7583,6 @@ elfNN_aarch64_size_dynamic_sections (bfd *output_bfd ATTRIBUTE_UNUSED,
 		{
 		}
 
-	      if (got_type == GOT_NORMAL)
-		{
-		}
-
 	      if (info->shared)
 		{
 		  if (got_type & GOT_TLSDESC_GD)
@@ -7547,7 +7595,8 @@ elfNN_aarch64_size_dynamic_sections (bfd *output_bfd ATTRIBUTE_UNUSED,
 		  if (got_type & GOT_TLS_GD)
 		    htab->root.srelgot->size += RELOC_SIZE (htab) * 2;
 
-		  if (got_type & GOT_TLS_IE)
+		  if (got_type & GOT_TLS_IE
+		      || got_type & GOT_NORMAL)
 		    htab->root.srelgot->size += RELOC_SIZE (htab);
 		}
 	    }