[committed, PATCH] Always create dynsym section with dynamic sections

[committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

We should always create the dynsym section, even if it is empty, with
dynamic sections.

	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always create the
	dynsym section, even if it is empty, with dynamic sections.
---
 bfd/ChangeLog | 5 +++++
 bfd/elflink.c | 6 +++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/bfd/ChangeLog b/bfd/ChangeLog
index dd1a308..1e0f6a1 100644
--- a/bfd/ChangeLog
+++ b/bfd/ChangeLog
@@ -1,5 +1,10 @@
 2016-02-22  H.J. Lu  <hongjiu.lu@intel.com>
 
+	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always create the
+	dynsym section, even if it is empty, with dynamic sections.
+
+2016-02-22  H.J. Lu  <hongjiu.lu@intel.com>
+
 	* syms.c: Remove BSF_COMMON from comments.
 	* bfd-in2.h: Regenerated.
 
diff --git a/bfd/elflink.c b/bfd/elflink.c
index c7672ed..89a6dea 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -875,9 +875,9 @@ _bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
 			  &dynsymcount);
 
   /* There is an unused NULL entry at the head of the table which
-     we must account for in our count.  Unless there weren't any
-     symbols, which means we'll have no table at all.  */
-  if (dynsymcount != 0)
+     we must account for in our count.  We always create the dynsym
+     section, even if it is empty, with dynamic sections.  */
+  if (elf_hash_table (info)->dynamic_sections_created)
     ++dynsymcount;
 
   elf_hash_table (info)->dynsymcount = dynsymcount;
-- 
2.5.0

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

Hi,

This is breaking a gc-sections test-case for MIPS. MIPS at least, uses the
elf_link_hash_table to track symbols even for static executables.
The renumbering violates assertions about how MIPS expects the table to look.

We could change the assert conditions, but I think thank since the NULL entry
originated when the table was initialized, it should exist just
the same after renumbering. If there is an additional case
(dynamic_sections_created), it should be OR'd to the pre-existing condition.

Thanks,
Faraz Shahbazker

On 02/22/2016 04:32 PM, H.J. Lu wrote:
> We should always create the dynsym section, even if it is empty, with
> dynamic sections.
> 
> 	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always create the
> 	dynsym section, even if it is empty, with dynamic sections.
> ---
>  bfd/ChangeLog | 5 +++++
>  bfd/elflink.c | 6 +++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/bfd/ChangeLog b/bfd/ChangeLog
> index dd1a308..1e0f6a1 100644
> --- a/bfd/ChangeLog
> +++ b/bfd/ChangeLog
> @@ -1,5 +1,10 @@
>  2016-02-22  H.J. Lu  <hongjiu.lu@intel.com>
>  
> +	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always create the
> +	dynsym section, even if it is empty, with dynamic sections.
> +
> +2016-02-22  H.J. Lu  <hongjiu.lu@intel.com>
> +
>  	* syms.c: Remove BSF_COMMON from comments.
>  	* bfd-in2.h: Regenerated.
>  
> diff --git a/bfd/elflink.c b/bfd/elflink.c
> index c7672ed..89a6dea 100644
> --- a/bfd/elflink.c
> +++ b/bfd/elflink.c
> @@ -875,9 +875,9 @@ _bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
>  			  &dynsymcount);
>  
>    /* There is an unused NULL entry at the head of the table which
> -     we must account for in our count.  Unless there weren't any
> -     symbols, which means we'll have no table at all.  */
> -  if (dynsymcount != 0)
> +     we must account for in our count.  We always create the dynsym
> +     section, even if it is empty, with dynamic sections.  */
> +  if (elf_hash_table (info)->dynamic_sections_created)
>      ++dynsymcount;
>  
>    elf_hash_table (info)->dynsymcount = dynsymcount;
> 

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 10:04 AM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> Hi,
>
> This is breaking a gc-sections test-case for MIPS. MIPS at least, uses the
> elf_link_hash_table to track symbols even for static executables.
> The renumbering violates assertions about how MIPS expects the table to look.
>
> We could change the assert conditions, but I think thank since the NULL entry
> originated when the table was initialized, it should exist just
> the same after renumbering. If there is an additional case
> (dynamic_sections_created), it should be OR'd to the pre-existing condition.

Please see:

https://groups.google.com/d/msg/generic-abi/P9oE0srJtJI/ilktL8KJEQAJ

You need to find a different way to track it.

H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/22/2016 11:11 AM, H.J. Lu wrote:
> https://groups.google.com/d/msg/generic-abi/P9oE0srJtJI/ilktL8KJEQAJ
> 
> You need to find a different way to track it.

My problem is not with the motivation, but the method. ->dynsymcount is
initialized to 1 to account for the NULL symbol when an elf_link_hash_table
is initialized. So while the table remains non-empty, any renumbering must
still account for the NULL symbol, *irrespective* of whether on not dynamic
sections were created. Either that or you move the initial NULL symbol
counting from link_hash_table_init() to link_create_dynamic_sections().

This change is causing a two-way dependence between creating a 
link_hash_table (an aspect of implementation) and having dynamic sections
in the output (your ABI issue). You want indirectly ensure that the table
is non-empty whenever dynamic sections are created. But do you also need to
ensure that it is empty when dynamic sections are not created?

Just to be clear, we're talking about the elf_link_hash_table
(implementation), not the dynamic symbol table in the output ELF. The two 
may coincide for dynamic ELFs but not necessarily for static ones.

Please consider:

+ if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
+     ++dynsymcount;

Regards,
Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> On 04/22/2016 11:11 AM, H.J. Lu wrote:
>> https://groups.google.com/d/msg/generic-abi/P9oE0srJtJI/ilktL8KJEQAJ
>>
>> You need to find a different way to track it.
>
> My problem is not with the motivation, but the method. ->dynsymcount is
> initialized to 1 to account for the NULL symbol when an elf_link_hash_table
> is initialized. So while the table remains non-empty, any renumbering must
> still account for the NULL symbol, *irrespective* of whether on not dynamic
> sections were created. Either that or you move the initial NULL symbol
> counting from link_hash_table_init() to link_create_dynamic_sections().
>
> This change is causing a two-way dependence between creating a
> link_hash_table (an aspect of implementation) and having dynamic sections
> in the output (your ABI issue). You want indirectly ensure that the table
> is non-empty whenever dynamic sections are created. But do you also need to
> ensure that it is empty when dynamic sections are not created?
>
> Just to be clear, we're talking about the elf_link_hash_table
> (implementation), not the dynamic symbol table in the output ELF. The two
> may coincide for dynamic ELFs but not necessarily for static ones.
>
> Please consider:
>
> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
> +     ++dynsymcount;

Are you saying dynamic_sections_created is 0 for MIPS here
and will become 1 later?


-- 
H.J.

RE: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/22/2016 12:28 PM, H.J. Lu wrote:
> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>> +     ++dynsymcount;
> 
> Are you saying dynamic_sections_created is 0 for MIPS here
> and will become 1 later?

No, it will remain 0. The link is static, but the hash_table is still used to
record global symbols that have GOT relocations against them. Ofc, this
hash_table does not result in creation of a dynsym section, because well,
dynamic_sections_created is 0.

Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of 
architectures use the link_hash_table in situations where it is not clear whether it is
being used to track dynamic symbols for a dynamic executable, as it is for x86.

Regards,
Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
<Faraz.Shahbazker@imgtec.com> wrote:
> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>> +     ++dynsymcount;
>>
>> Are you saying dynamic_sections_created is 0 for MIPS here
>> and will become 1 later?
>
> No, it will remain 0. The link is static, but the hash_table is still used to
> record global symbols that have GOT relocations against them. Ofc, this
> hash_table does not result in creation of a dynsym section, because well,
> dynamic_sections_created is 0.
>
> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
> architectures use the link_hash_table in situations where it is not clear whether it is
> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>

Can you show me the output of "readelf -SWl" on such executable?

-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

Here goes:
# mips-mti-linux-gnu-readelf -SWl gcsec-1.exe

There are 36 section headers, starting at offset 0x3f21a8:

Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] .note.ABI-tag     NOTE            004000d4 0000d4 000020 00   A  0   0  4
  [ 2] .MIPS.abiflags    MIPS_ABIFLAGS   004000f8 0000f8 000018 18   A  0   0  8
  [ 3] .init             PROGBITS        00400110 000110 000074 00  AX  0   0  4
  [ 4] .text             PROGBITS        00400190 000190 069090 00  AX  0   0 16
  [ 5] .fini             PROGBITS        00469220 069220 000044 00  AX  0   0  4
  [ 6] .rodata           PROGBITS        00469270 069270 0168b0 00   A  0   0 16
  [ 7] __libc_atexit     PROGBITS        0047fb20 07fb20 000004 00   A  0   0  4
  [ 8] .eh_frame         PROGBITS        0047fb24 07fb24 002fe8 00   A  0   0  4
  [ 9] .gcc_except_table PROGBITS        00482b0c 082b0c 0000af 00   A  0   0  1
  [10] .tdata            PROGBITS        00493000 083000 000010 00 WAT  0   0  4
  [11] .tbss             NOBITS          00493010 083010 000018 00 WAT  0   0  4
  [12] .fini_array       FINI_ARRAY      00493010 083010 000004 00  WA  0   0  4
  [13] .ctors            PROGBITS        00493014 083014 000008 00  WA  0   0  4
  [14] .dtors            PROGBITS        0049301c 08301c 000008 00  WA  0   0  4
  [15] .jcr              PROGBITS        00493024 083024 000004 00  WA  0   0  4
  [16] .data.rel.ro      PROGBITS        00493028 083028 000038 00  WA  0   0  4
  [17] .data             PROGBITS        00493060 083060 000ff0 00  WA  0   0 16
  [18] .got              PROGBITS        00494050 084050 0000e0 04 WAp  0   0 16
  [19] .sbss             NOBITS          00494130 084130 0000c8 00 WAp  0   0  8
  [20] .bss              NOBITS          00494200 084130 000cf0 00  WA  0   0 16
  [21] __libc_freeres_ptrs NOBITS          00494ef0 084130 000018 00  WA  0   0  4
  [22] .comment          PROGBITS        00000000 084130 000029 01  MS  0   0  1
  [23] .debug_aranges    MIPS_DWARF      00000000 084160 002820 00      0   0  8
  [24] .debug_info       MIPS_DWARF      00000000 086980 21fd8e 00      0   0  1
  [25] .debug_abbrev     MIPS_DWARF      00000000 2a670e 02efb0 00      0   0  1
  [26] .debug_line       MIPS_DWARF      00000000 2d56be 04bbae 00      0   0  1
  [27] .debug_frame      MIPS_DWARF      00000000 32126c 0069e0 00      0   0  4
  [28] .debug_str        MIPS_DWARF      00000000 327c4c 012835 01  MS  0   0  1
  [29] .debug_loc        MIPS_DWARF      00000000 33a481 093917 00      0   0  1
  [30] .debug_ranges     MIPS_DWARF      00000000 3cdd98 0166c0 00      0   0  8
  [31] .gnu.attributes   LOOS+0xffffff5  00000000 3e4458 000010 00      0   0  1
  [32] .mdebug.abi32     PROGBITS        00000000 3e4468 000000 00      0   0  1
  [33] .shstrtab         STRTAB          00000000 3f203c 00016c 00      0   0  1
  [34] .symtab           SYMTAB          00000000 3e4468 0075a0 10     35 886  4
  [35] .strtab           STRTAB          00000000 3eba08 006634 00      0   0  1
Key to Flags:
  W (write), A (alloc), X (execute), M (merge), S (strings), I (info),
  L (link order), O (extra OS processing required), G (group), T (TLS),
  C (compressed), x (unknown), o (OS specific), E (exclude),
  p (processor specific)

Elf file type is EXEC (Executable file)
Entry point 0x4003e0
There are 5 program headers, starting at offset 52

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  ABIFLAGS       0x0000f8 0x004000f8 0x004000f8 0x00018 0x00018 R   0x8
  LOAD           0x000000 0x00400000 0x00400000 0x82bbb 0x82bbb R E 0x10000
  LOAD           0x083000 0x00493000 0x00493000 0x01130 0x01f08 RW  0x10000
  NOTE           0x0000d4 0x004000d4 0x004000d4 0x00020 0x00020 R   0x4
  TLS            0x083000 0x00493000 0x00493000 0x00010 0x00028 R   0x4

 Section to Segment mapping:
  Segment Sections...
   00     .MIPS.abiflags 
   01     .note.ABI-tag .MIPS.abiflags .init .text .fini .rodata __libc_atexit .eh_frame .gcc_except_table 
   02     .tdata .fini_array .ctors .dtors .jcr .data.rel.ro .data .got .sbss .bss __libc_freeres_ptrs 
   03     .note.ABI-tag 
   04     .tdata .tbss 

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
<Faraz.Shahbazker@imgtec.com> wrote:
> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>> +     ++dynsymcount;
>>
>> Are you saying dynamic_sections_created is 0 for MIPS here
>> and will become 1 later?
>
> No, it will remain 0. The link is static, but the hash_table is still used to
> record global symbols that have GOT relocations against them. Ofc, this
> hash_table does not result in creation of a dynsym section, because well,
> dynamic_sections_created is 0.
>
> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
> architectures use the link_hash_table in situations where it is not clear whether it is
> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>

So MIPS doesn't have dynamic symbols in this case.  It just borrows
dynsymcount for different purpose.  Is this correct?


-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 4:24 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
> <Faraz.Shahbazker@imgtec.com> wrote:
>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>> +     ++dynsymcount;
>>>
>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>> and will become 1 later?
>>
>> No, it will remain 0. The link is static, but the hash_table is still used to
>> record global symbols that have GOT relocations against them. Ofc, this
>> hash_table does not result in creation of a dynsym section, because well,
>> dynamic_sections_created is 0.
>>
>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>> architectures use the link_hash_table in situations where it is not clear whether it is
>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>
>
> So MIPS doesn't have dynamic symbols in this case.  It just borrows
> dynsymcount for different purpose.  Is this correct?
>

The comments for elf_link_hash_table say:

  /* The number of symbols found in the link which must be put into
     the .dynsym section.  */
  bfd_size_type dynsymcount;


-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/22/16 16:24, H.J. Lu wrote:
> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
> <Faraz.Shahbazker@imgtec.com> wrote:
>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>> +     ++dynsymcount;
>>>
>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>> and will become 1 later?
>>
>> No, it will remain 0. The link is static, but the hash_table is still used to
>> record global symbols that have GOT relocations against them. Ofc, this
>> hash_table does not result in creation of a dynsym section, because well,
>> dynamic_sections_created is 0.
>>
>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>> architectures use the link_hash_table in situations where it is not clear whether it is
>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>
> 
> So MIPS doesn't have dynamic symbols in this case.  It just borrows
> dynsymcount for different purpose.  Is this correct?

Not quite! MIPS is expecting dynsymcount to count the number of symbols
that would have gone in to the .dynsym, even for a static executable. That way
parts of the arch-specific code can remain agnostic to the static/dynamic nature
of the link. It may not be used exactly as documented, but its not being used
for what one would call a different purpose.

All we need is for handling of dynsymcount when renumbering to be consistent with
its initialization. If the initial increment for a NULL symbol was not gated by
dynamic_sections_created, then the increment when renumbering should also not.
If the increment when renumbering has to be gated by dynamic_sections_created,
then the initial increment must also be so.

Regards,
Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> On 04/22/16 16:24, H.J. Lu wrote:
>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>> <Faraz.Shahbazker@imgtec.com> wrote:
>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>> +     ++dynsymcount;
>>>>
>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>> and will become 1 later?
>>>
>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>> record global symbols that have GOT relocations against them. Ofc, this
>>> hash_table does not result in creation of a dynsym section, because well,
>>> dynamic_sections_created is 0.
>>>
>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>
>>
>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>> dynsymcount for different purpose.  Is this correct?
>
> Not quite! MIPS is expecting dynsymcount to count the number of symbols
> that would have gone in to the .dynsym, even for a static executable. That way
> parts of the arch-specific code can remain agnostic to the static/dynamic nature
> of the link. It may not be used exactly as documented, but its not being used
> for what one would call a different purpose.
>
> All we need is for handling of dynsymcount when renumbering to be consistent with
> its initialization. If the initial increment for a NULL symbol was not gated by
> dynamic_sections_created, then the increment when renumbering should also not.
> If the increment when renumbering has to be gated by dynamic_sections_created,
> then the initial increment must also be so.
>

From what you are saying, shouldn't dynsymcount be incremented
unconditionally?


-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/22/16 19:05, H.J. Lu wrote:
> On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
> <faraz.shahbazker@imgtec.com> wrote:
>> On 04/22/16 16:24, H.J. Lu wrote:
>>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>>> <Faraz.Shahbazker@imgtec.com> wrote:
>>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>>> +     ++dynsymcount;
>>>>>
>>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>>> and will become 1 later?
>>>>
>>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>>> record global symbols that have GOT relocations against them. Ofc, this
>>>> hash_table does not result in creation of a dynsym section, because well,
>>>> dynamic_sections_created is 0.
>>>>
>>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>>
>>>
>>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>>> dynsymcount for different purpose.  Is this correct?
>>
>> Not quite! MIPS is expecting dynsymcount to count the number of symbols
>> that would have gone in to the .dynsym, even for a static executable. That way
>> parts of the arch-specific code can remain agnostic to the static/dynamic nature
>> of the link. It may not be used exactly as documented, but its not being used
>> for what one would call a different purpose.
>>
>> All we need is for handling of dynsymcount when renumbering to be consistent with
>> its initialization. If the initial increment for a NULL symbol was not gated by
>> dynamic_sections_created, then the increment when renumbering should also not.
>> If the increment when renumbering has to be gated by dynamic_sections_created,
>> then the initial increment must also be so.
> 
> From what you are saying, shouldn't dynsymcount be incremented
> unconditionally?
No. Always, when the table is non-empty + whatever else you need.

Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Fri, Apr 22, 2016 at 7:31 PM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> On 04/22/16 19:05, H.J. Lu wrote:
>> On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
>> <faraz.shahbazker@imgtec.com> wrote:
>>> On 04/22/16 16:24, H.J. Lu wrote:
>>>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>>>> <Faraz.Shahbazker@imgtec.com> wrote:
>>>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>>>> +     ++dynsymcount;
>>>>>>
>>>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>>>> and will become 1 later?
>>>>>
>>>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>>>> record global symbols that have GOT relocations against them. Ofc, this
>>>>> hash_table does not result in creation of a dynsym section, because well,
>>>>> dynamic_sections_created is 0.
>>>>>
>>>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>>>
>>>>
>>>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>>>> dynsymcount for different purpose.  Is this correct?
>>>
>>> Not quite! MIPS is expecting dynsymcount to count the number of symbols
>>> that would have gone in to the .dynsym, even for a static executable. That way
>>> parts of the arch-specific code can remain agnostic to the static/dynamic nature
>>> of the link. It may not be used exactly as documented, but its not being used
>>> for what one would call a different purpose.
>>>
>>> All we need is for handling of dynsymcount when renumbering to be consistent with
>>> its initialization. If the initial increment for a NULL symbol was not gated by
>>> dynamic_sections_created, then the increment when renumbering should also not.
>>> If the increment when renumbering has to be gated by dynamic_sections_created,
>>> then the initial increment must also be so.
>>
>> From what you are saying, shouldn't dynsymcount be incremented
>> unconditionally?
> No. Always, when the table is non-empty + whatever else you need.
>

You said dynsym should be treated treated the same for static and
dynamic executables.   dynsymcount is number of dynsym + 1 in
dynamic executable.  Why isn't it true for static executable?


-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/23/16 05:27, H.J. Lu wrote:
> On Fri, Apr 22, 2016 at 7:31 PM, Faraz Shahbazker
> <faraz.shahbazker@imgtec.com> wrote:
>> On 04/22/16 19:05, H.J. Lu wrote:
>>> On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
>>> <faraz.shahbazker@imgtec.com> wrote:
>>>> On 04/22/16 16:24, H.J. Lu wrote:
>>>>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>>>>> <Faraz.Shahbazker@imgtec.com> wrote:
>>>>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>>>>> +     ++dynsymcount;
>>>>>>>
>>>>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>>>>> and will become 1 later?
>>>>>>
>>>>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>>>>> record global symbols that have GOT relocations against them. Ofc, this
>>>>>> hash_table does not result in creation of a dynsym section, because well,
>>>>>> dynamic_sections_created is 0.
>>>>>>
>>>>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>>>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>>>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>>>>
>>>>>
>>>>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>>>>> dynsymcount for different purpose.  Is this correct?
>>>>
>>>> Not quite! MIPS is expecting dynsymcount to count the number of symbols
>>>> that would have gone in to the .dynsym, even for a static executable. That way
>>>> parts of the arch-specific code can remain agnostic to the static/dynamic nature
>>>> of the link. It may not be used exactly as documented, but its not being used
>>>> for what one would call a different purpose.
>>>>
>>>> All we need is for handling of dynsymcount when renumbering to be consistent with
>>>> its initialization. If the initial increment for a NULL symbol was not gated by
>>>> dynamic_sections_created, then the increment when renumbering should also not.
>>>> If the increment when renumbering has to be gated by dynamic_sections_created,
>>>> then the initial increment must also be so.
>>>
>>> From what you are saying, shouldn't dynsymcount be incremented
>>> unconditionally?
>> No. Always, when the table is non-empty + whatever else you need.
>>
> 
> You said dynsym should be treated treated the same for static and
> dynamic executables.   dynsymcount is number of dynsym + 1 in
> dynamic executable.  Why isn't it true for static executable?

It is, or at least used to be, before this patch. It still is for both,
before renumbering. But now the +1 only happens for dynamic executables
when renumbering.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Sat, Apr 23, 2016 at 8:24 AM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> On 04/23/16 05:27, H.J. Lu wrote:
>> On Fri, Apr 22, 2016 at 7:31 PM, Faraz Shahbazker
>> <faraz.shahbazker@imgtec.com> wrote:
>>> On 04/22/16 19:05, H.J. Lu wrote:
>>>> On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
>>>> <faraz.shahbazker@imgtec.com> wrote:
>>>>> On 04/22/16 16:24, H.J. Lu wrote:
>>>>>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>>>>>> <Faraz.Shahbazker@imgtec.com> wrote:
>>>>>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>>>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>>>>>> +     ++dynsymcount;
>>>>>>>>
>>>>>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>>>>>> and will become 1 later?
>>>>>>>
>>>>>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>>>>>> record global symbols that have GOT relocations against them. Ofc, this
>>>>>>> hash_table does not result in creation of a dynsym section, because well,
>>>>>>> dynamic_sections_created is 0.
>>>>>>>
>>>>>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>>>>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>>>>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>>>>>
>>>>>>
>>>>>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>>>>>> dynsymcount for different purpose.  Is this correct?
>>>>>
>>>>> Not quite! MIPS is expecting dynsymcount to count the number of symbols
>>>>> that would have gone in to the .dynsym, even for a static executable. That way
>>>>> parts of the arch-specific code can remain agnostic to the static/dynamic nature
>>>>> of the link. It may not be used exactly as documented, but its not being used
>>>>> for what one would call a different purpose.
>>>>>
>>>>> All we need is for handling of dynsymcount when renumbering to be consistent with
>>>>> its initialization. If the initial increment for a NULL symbol was not gated by
>>>>> dynamic_sections_created, then the increment when renumbering should also not.
>>>>> If the increment when renumbering has to be gated by dynamic_sections_created,
>>>>> then the initial increment must also be so.
>>>>
>>>> From what you are saying, shouldn't dynsymcount be incremented
>>>> unconditionally?
>>> No. Always, when the table is non-empty + whatever else you need.
>>>
>>
>> You said dynsym should be treated treated the same for static and
>> dynamic executables.   dynsymcount is number of dynsym + 1 in
>> dynamic executable.  Why isn't it true for static executable?
>
> It is, or at least used to be, before this patch. It still is for both,
> before renumbering. But now the +1 only happens for dynamic executables
> when renumbering.

Then what is wrong to always +1 for both dynamic and static
executables?

-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/23/16 08:36, H.J. Lu wrote:
> On Sat, Apr 23, 2016 at 8:24 AM, Faraz Shahbazker
> <faraz.shahbazker@imgtec.com> wrote:
>> On 04/23/16 05:27, H.J. Lu wrote:
>>> On Fri, Apr 22, 2016 at 7:31 PM, Faraz Shahbazker
>>> <faraz.shahbazker@imgtec.com> wrote:
>>>> On 04/22/16 19:05, H.J. Lu wrote:
>>>>> On Fri, Apr 22, 2016 at 6:37 PM, Faraz Shahbazker
>>>>> <faraz.shahbazker@imgtec.com> wrote:
>>>>>> On 04/22/16 16:24, H.J. Lu wrote:
>>>>>>> On Fri, Apr 22, 2016 at 2:49 PM, Faraz Shahbazker
>>>>>>> <Faraz.Shahbazker@imgtec.com> wrote:
>>>>>>>> On 04/22/2016 12:28 PM, H.J. Lu wrote:
>>>>>>>>> On Fri, Apr 22, 2016 at 11:55 AM, Faraz Shahbazker
>>>>>>>>>> + if (dynsymcount != 0 || elf_hash_table (info)->dynamic_sections_created)
>>>>>>>>>> +     ++dynsymcount;
>>>>>>>>>
>>>>>>>>> Are you saying dynamic_sections_created is 0 for MIPS here
>>>>>>>>> and will become 1 later?
>>>>>>>>
>>>>>>>> No, it will remain 0. The link is static, but the hash_table is still used to
>>>>>>>> record global symbols that have GOT relocations against them. Ofc, this
>>>>>>>> hash_table does not result in creation of a dynsym section, because well,
>>>>>>>> dynamic_sections_created is 0.
>>>>>>>>
>>>>>>>> Check the list of callers to bfd_elf_link_record_dynamic_symbol(), a number of
>>>>>>>> architectures use the link_hash_table in situations where it is not clear whether it is
>>>>>>>> being used to track dynamic symbols for a dynamic executable, as it is for x86.
>>>>>>>>
>>>>>>>
>>>>>>> So MIPS doesn't have dynamic symbols in this case.  It just borrows
>>>>>>> dynsymcount for different purpose.  Is this correct?
>>>>>>
>>>>>> Not quite! MIPS is expecting dynsymcount to count the number of symbols
>>>>>> that would have gone in to the .dynsym, even for a static executable. That way
>>>>>> parts of the arch-specific code can remain agnostic to the static/dynamic nature
>>>>>> of the link. It may not be used exactly as documented, but its not being used
>>>>>> for what one would call a different purpose.
>>>>>>
>>>>>> All we need is for handling of dynsymcount when renumbering to be consistent with
>>>>>> its initialization. If the initial increment for a NULL symbol was not gated by
>>>>>> dynamic_sections_created, then the increment when renumbering should also not.
>>>>>> If the increment when renumbering has to be gated by dynamic_sections_created,
>>>>>> then the initial increment must also be so.
>>>>>
>>>>> From what you are saying, shouldn't dynsymcount be incremented
>>>>> unconditionally?
>>>> No. Always, when the table is non-empty + whatever else you need.
>>>>
>>>
>>> You said dynsym should be treated treated the same for static and
>>> dynamic executables.   dynsymcount is number of dynsym + 1 in
>>> dynamic executable.  Why isn't it true for static executable?
>>
>> It is, or at least used to be, before this patch. It still is for both,
>> before renumbering. But now the +1 only happens for dynamic executables
>> when renumbering.
> 
> Then what is wrong to always +1 for both dynamic and static
> executables?

Aah, now I see! Ever since the table was created, the count was at least 1. 
You are saying it should always remain at least 1 no matter what. Had a quick
look through other uses of the field and it looks safe enough to me.

Regards,
Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

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

On Sat, Apr 23, 2016 at 12:25 PM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
>>>> You said dynsym should be treated treated the same for static and
>>>> dynamic executables.   dynsymcount is number of dynsym + 1 in
>>>> dynamic executable.  Why isn't it true for static executable?
>>>
>>> It is, or at least used to be, before this patch. It still is for both,
>>> before renumbering. But now the +1 only happens for dynamic executables
>>> when renumbering.
>>
>> Then what is wrong to always +1 for both dynamic and static
>> executables?
>
> Aah, now I see! Ever since the table was created, the count was at least 1.
> You are saying it should always remain at least 1 no matter what. Had a quick
> look through other uses of the field and it looks safe enough to me.
>

Here is a patch.  Does it work for you?

-- 
H.J.

[-- Attachment #2: 0001-Always-count-the-NULL-entry-in-dynamic-symbol-table.patch --]
[-- Type: text/x-patch, Size: 1870 bytes --]

From f54270fc5a342ed8ead17e95731c3b0c2e7a1ecc Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 23 Apr 2016 18:37:59 -0700
Subject: [PATCH] Always count the NULL entry in dynamic symbol table

There is an unused NULL entry at the head of dynamic symbol table which
we must account for in our count even if the table is empty or unused.

	* elf-bfd.h (elf_link_hash_table): Update comments for
	dynsymcount.
	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always count for
	the unused NULL entry at the head of dynamic symbol table.
---
 bfd/elf-bfd.h | 2 +-
 bfd/elflink.c | 8 +++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 5dce70e..de721f0 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -520,7 +520,7 @@ struct elf_link_hash_table
   union gotplt_union init_got_offset;
   union gotplt_union init_plt_offset;
 
-  /* The number of symbols found in the link which must be put into
+  /* The number of symbols found in the link which is intended for
      the .dynsym section.  */
   bfd_size_type dynsymcount;
 
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 6f67266..17751cf 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -905,11 +905,9 @@ _bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
 			  elf_link_renumber_hash_table_dynsyms,
 			  &dynsymcount);
 
-  /* There is an unused NULL entry at the head of the table which
-     we must account for in our count.  We always create the dynsym
-     section, even if it is empty, with dynamic sections.  */
-  if (elf_hash_table (info)->dynamic_sections_created)
-    ++dynsymcount;
+  /* There is an unused NULL entry at the head of the table which we
+     must account for in our count even if the table is empty.  */
+  dynsymcount++;
 
   elf_hash_table (info)->dynsymcount = dynsymcount;
   return dynsymcount;
-- 
2.5.5

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Faraz Shahbazker]

On 04/23/16 18:42, H.J. Lu wrote:
> Here is a patch.  Does it work for you?
Yes, this works.

Regards,
Faraz Shahbazker

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

On Sun, Apr 24, 2016 at 7:18 PM, Faraz Shahbazker
<faraz.shahbazker@imgtec.com> wrote:
> On 04/23/16 18:42, H.J. Lu wrote:
>> Here is a patch.  Does it work for you?
> Yes, this works.
>

Any other comments, feedbacks for my patch?

-- 
H.J.

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Alan Modra]

On Mon, Apr 25, 2016 at 05:47:15AM -0700, H.J. Lu wrote:
> On Sun, Apr 24, 2016 at 7:18 PM, Faraz Shahbazker
> <faraz.shahbazker@imgtec.com> wrote:
> > On 04/23/16 18:42, H.J. Lu wrote:
> >> Here is a patch.  Does it work for you?
> > Yes, this works.
> >
> 
> Any other comments, feedbacks for my patch?

I think you'd better look at all places that test dynsymcount.  For
instance, bfd_elf_size_dynsym_hash_dynstr.

This should really have been done when you committed
https://sourceware.org/ml/binutils/2016-02/msg00326.html and that
patch should have explained why the change was made.  "We should
always create the dynsym section, even if it is empty, with dynamic
sections" doesn't explain anything about why the change was made.  In
fact, the explanation for the patch was no better than "We should do
what this patch does".

-- 
Alan Modra
Australia Development Lab, IBM

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[H.J. Lu]

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

On Mon, Apr 25, 2016 at 6:41 AM, Alan Modra <amodra@gmail.com> wrote:
> On Mon, Apr 25, 2016 at 05:47:15AM -0700, H.J. Lu wrote:
>> On Sun, Apr 24, 2016 at 7:18 PM, Faraz Shahbazker
>> <faraz.shahbazker@imgtec.com> wrote:
>> > On 04/23/16 18:42, H.J. Lu wrote:
>> >> Here is a patch.  Does it work for you?
>> > Yes, this works.
>> >
>>
>> Any other comments, feedbacks for my patch?
>
> I think you'd better look at all places that test dynsymcount.  For
> instance, bfd_elf_size_dynsym_hash_dynstr.

I checked.  They look OK.

> This should really have been done when you committed
> https://sourceware.org/ml/binutils/2016-02/msg00326.html and that
> patch should have explained why the change was made.  "We should
> always create the dynsym section, even if it is empty, with dynamic
> sections" doesn't explain anything about why the change was made.  In
> fact, the explanation for the patch was no better than "We should do
> what this patch does".

The issue is some dynamic tags (sections) are mandatory in .dynamic
section even if they are empty.  MIPS backend use dynsymcount, which
is intended for the mandatory DT_SYMTAB (.dynsym section), to build
static executable, which doesn't have dynamic section.  Here is the
updated patch.  Is it OK for master?


-- 
H.J.

[-- Attachment #2: 0001-Always-count-the-NULL-entry-in-dynamic-symbol-table.patch --]
[-- Type: text/x-patch, Size: 2217 bytes --]

From 8d941b3f99a57f89f564ed16f94d5cdcad6fede3 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Sat, 23 Apr 2016 18:37:59 -0700
Subject: [PATCH] Always count the NULL entry in dynamic symbol table

There is an unused NULL entry at the head of dynamic symbol table which
we must account for in our count even if the table is empty or unused
since it is intended for the mandatory DT_SYMTAB tag (.dynsym section)
in .dynamic section.

	* elf-bfd.h (elf_link_hash_table): Update comments for
	dynsymcount.
	* elflink.c (_bfd_elf_link_renumber_dynsyms): Always count for
	the unused NULL entry at the head of dynamic symbol table.
---
 bfd/elf-bfd.h |  4 ++--
 bfd/elflink.c | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
index 5dce70e..7447629 100644
--- a/bfd/elf-bfd.h
+++ b/bfd/elf-bfd.h
@@ -520,8 +520,8 @@ struct elf_link_hash_table
   union gotplt_union init_got_offset;
   union gotplt_union init_plt_offset;
 
-  /* The number of symbols found in the link which must be put into
-     the .dynsym section.  */
+  /* The number of symbols found in the link which is intended for the
+     mandatory DT_SYMTAB tag (.dynsym section) in .dynamic section.  */
   bfd_size_type dynsymcount;
 
   /* The string table of dynamic symbols, which becomes the .dynstr
diff --git a/bfd/elflink.c b/bfd/elflink.c
index 6f67266..7babfb9 100644
--- a/bfd/elflink.c
+++ b/bfd/elflink.c
@@ -905,11 +905,11 @@ _bfd_elf_link_renumber_dynsyms (bfd *output_bfd,
 			  elf_link_renumber_hash_table_dynsyms,
 			  &dynsymcount);
 
-  /* There is an unused NULL entry at the head of the table which
-     we must account for in our count.  We always create the dynsym
-     section, even if it is empty, with dynamic sections.  */
-  if (elf_hash_table (info)->dynamic_sections_created)
-    ++dynsymcount;
+  /* There is an unused NULL entry at the head of the table which we
+     must account for in our count even if the table is empty since it
+     is intended for the mandatory DT_SYMTAB tag (.dynsym section) in
+     .dynamic section.  */
+  dynsymcount++;
 
   elf_hash_table (info)->dynsymcount = dynsymcount;
   return dynsymcount;
-- 
2.5.5

Re: [committed, PATCH] Always create dynsym section with dynamic sections

[Alan Modra]

On Mon, Apr 25, 2016 at 09:03:06AM -0700, H.J. Lu wrote:
> On Mon, Apr 25, 2016 at 6:41 AM, Alan Modra <amodra@gmail.com> wrote:
> > On Mon, Apr 25, 2016 at 05:47:15AM -0700, H.J. Lu wrote:
> >> On Sun, Apr 24, 2016 at 7:18 PM, Faraz Shahbazker
> >> <faraz.shahbazker@imgtec.com> wrote:
> >> > On 04/23/16 18:42, H.J. Lu wrote:
> >> >> Here is a patch.  Does it work for you?
> >> > Yes, this works.
> >> >
> >>
> >> Any other comments, feedbacks for my patch?
> >
> > I think you'd better look at all places that test dynsymcount.  For
> > instance, bfd_elf_size_dynsym_hash_dynstr.
> 
> I checked.  They look OK.
> 
> > This should really have been done when you committed
> > https://sourceware.org/ml/binutils/2016-02/msg00326.html and that
> > patch should have explained why the change was made.  "We should
> > always create the dynsym section, even if it is empty, with dynamic
> > sections" doesn't explain anything about why the change was made.  In
> > fact, the explanation for the patch was no better than "We should do
> > what this patch does".
> 
> The issue is some dynamic tags (sections) are mandatory in .dynamic
> section even if they are empty.  MIPS backend use dynsymcount, which
> is intended for the mandatory DT_SYMTAB (.dynsym section), to build
> static executable, which doesn't have dynamic section.  Here is the
> updated patch.  Is it OK for master?

The dynsymcount != 0 tests in bfd_elf_size_dynsym_hash_dynstr will
never trigger with your patch.  Please remove them.  OK with that
change.

-- 
Alan Modra
Australia Development Lab, IBM