Re: [ARM] PR ld/21402, only override the symbol dynamic decision on undefined weak symbol

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi Jiong,

Thanks for your review! Here is my explanation below.

> diff --git a/ld/testsuite/ld-arm/tls-app.r b/ld/testsuite/ld-arm/tls-app.r
> index b156d52..518c18c 100644
> --- a/ld/testsuite/ld-arm/tls-app.r
> +++ b/ld/testsuite/ld-arm/tls-app.r
> @@ -3,8 +3,5 @@
>
>  DYNAMIC RELOCATION RECORDS
>  OFFSET   TYPE              VALUE
> -[0-9a-f]+ R_ARM_TLS_DTPMOD32  app_gd
> -[0-9a-f]+ R_ARM_TLS_DTPOFF32  app_gd
>  [0-9a-f]+ R_ARM_TLS_DTPMOD32  lib_gd
>  [0-9a-f]+ R_ARM_TLS_DTPOFF32  lib_gd
> -[0-9a-f]+ R_ARM_TLS_TPOFF32  app_ie

For this test case
GOT_TLS_GD relocation type against app_gd.
GOT_TLS_IE relocation type against app_ie.

The relocations for those two symbols in GOT are omitted because they are not dynamic 
symbols and no relocation are needed.

For example for app_gd case. The following code snippet shows the result.
> 	    if ((bfd_link_pic (info) || indx != 0)
> 		&& (h == NULL
> 		    || ELF_ST_VISIBILITY (h->other) == STV_DEFAULT
> 		    || h->root.type != bfd_link_hash_undefweak))
> 	      {
> 		need_relocs = TRUE;
> 		BFD_ASSERT (srelgot != NULL);
> 	      }
need_relocs is FALSE in this case, and
> 		  {
> 		    /* If we are not emitting relocations for a
> 		       general dynamic reference, then we must be in a
> 		       static link or an executable link with the
> 		       symbol binding locally.  Mark it as belonging
> 		       to module 1, the executable.  */
> 		    bfd_put_32 (output_bfd, 1,
> 				sgot->contents + cur_off);
> 		    bfd_put_32 (output_bfd, value - dtpoff_base (info),
> 				sgot->contents + cur_off + 4);
> 		  }

That is the same case for aarch64.

> diff --git a/ld/testsuite/ld-arm/unresolved-1-dyn.d b/ld/testsuite/ld-arm/unresolved-1-dyn.d
> index 21cd959..529da37 100644
> --- a/ld/testsuite/ld-arm/unresolved-1-dyn.d
> +++ b/ld/testsuite/ld-arm/unresolved-1-dyn.d
> @@ -5,4 +5,4 @@
>
>  Relocation section '\.rel\.dyn' .*
>   Offset .*
> -.* R_ARM_GLOB_DAT +00000000 +foo
> +^.*  00000000 R_ARM_NONE.+


Initially, symbol FOO will request a slot in GOT table.
Once the dynamic section is created, it will reserve a slot for GOT entry's R_ARM_GLOB_DAT 
relocation. It's initialized to zero. And this is the same value for R_ARM_NONE relocation.

At the moment, the srelgot->reloc_count is not synchronized with srelgot->size.
In this particular case:
srelgot->reloc_count == 0
srelgot->size == 8

Originally, when the symbol FOO is recored as dynamic symbol. A R_ARM_GLOB_DAT will be 
create against its GOT slot. And this relocation will use the space reserved above as
the srelgot->reloc_count is zero. That's why there is only a R_ARM_GLOB_DAT relocation for 
foo in the relocation table before the change here.

After the change, FOO is not put into the dynamic symbol table. And there is no 
R_ARM_RELATIVE elocation created for it as we don't create this for non-pic case.

And I think, when readelf dump the relocation table, the only the size is used. And 0s are 
dumped as R_ARM_NONE.

It looks this part could be cleaned a little bit.

Regards,
Renlin

On 27/09/17 10:07, Jiong Wang wrote:
> >      * testsuite/ld-arm/tls-app.d: Update address.
> >      * testsuite/ld-arm/tls-app.r: Remove relocations.
>
> Hi Renlin,
>
>    I have concerns on these testcases changes.
>
>    They look not like something straightforward to understand.
>
>    Could you explain why some dynamic TLS relocations are removed? This
> patch doesn't touch TLS relaxation, so how could some dynamic TLS
> relocations been resolved at static linking time?
>
>    My concern is this patch has break some internal logic that caused
> runtime TLS relocations missing, this will cause wrong tls address
> returned.
>
>   >     * testsuite/ld-arm/unresolved-1-dyn.d: Update.
>
>     This looks to me is redundant dynamic relocation that the linker
> should not generate it at all.
>
> Regards,
> Jiong