diff mbox series

LoongArch: Update comment about bottom bit usage in TLS GOT construction

Message ID 20231228145802.74719-3-ishitatsuyuki@gmail.com
State Accepted
Headers
Series LoongArch: Update comment about bottom bit usage in TLS GOT construction |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Tatsuyuki Ishi Dec. 28, 2023, 2:58 p.m. UTC 
  The GOT assignment logic, likely copied from another backend, does not in
fact require multiple flags to construct multiple type of slots. Instead,
all slots are initialized on the first relocation encountered. Update
comment to avoid confusion.
---
 bfd/elfnn-loongarch.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

Comments

mengqinggang Dec. 29, 2023, 8:50 a.m. UTC | #1 
At the beginning of implementation, we try to IE and GD only generate 
their own GOT entry,
so we need two bits to do the flags.


在 2023/12/28 下午10:58, Tatsuyuki Ishi 写道:
> The GOT assignment logic, likely copied from another backend, does not in
> fact require multiple flags to construct multiple type of slots. Instead,
> all slots are initialized on the first relocation encountered. Update
> comment to avoid confusion.
> ---
>   bfd/elfnn-loongarch.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
> index 64c34e99261..d66dcee1100 100644
> --- a/bfd/elfnn-loongarch.c
> +++ b/bfd/elfnn-loongarch.c
> @@ -3656,16 +3656,13 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>   	  relocation -= elf_hash_table (info)->tls_sec->vma;
>   	  break;
>   
> -	/* TLS IE LD/GD process separately is troublesome.
> -	   When a symbol is both ie and LD/GD, h->got.off |= 1
> -	   make only one type be relocated.  We must use
> -	   h->got.offset |= 1 and h->got.offset |= 2
> -	   diff IE and LD/GD.  And all (got_off & (~(bfd_vma)1))
> -	   (IE LD/GD and reusable GOT reloc) must change to
> -	   (got_off & (~(bfd_vma)3)), beause we use lowest 2 bits
> -	   as a tag.
> -	   Now, LD and GD is both GOT_TLS_GD type, LD seems to
> -	   can be omitted.  */
> +	/* The bottom bit of h->got.offset is (ab)used as a flag.
> +	   Upon encountering the first TLS relocation, we initialize all GOT
> +	   slots for the corresponding symbol and set the bottom bit to 1.
> +
> +	   The second and subsequent relocations will check the flag, see that
> +	   the slot is already initialized, and move on to just relocating the
> +	   entry.  */
>   	case R_LARCH_TLS_IE_PC_HI20:
>   	case R_LARCH_TLS_IE_HI20:
>   	case R_LARCH_TLS_LD_PC_HI20:
Tatsuyuki Ishi Dec. 29, 2023, 9:11 a.m. UTC | #2 
> On Dec 29, 2023, at 17:50, mengqinggang <mengqinggang@loongson.cn> wrote:
> 
> At the beginning of implementation, we try to IE and GD only generate their own GOT entry,
> so we need two bits to do the flags.

I checked the tree back at the date the comment was introduced (6d13722a: "bfd: Add supported for LoongArch new relocations.”). Even at that time, there was no |= 2 accesses, so if my understanding is correct, this comment was wrong from the beginning.

Could you clarify what you mean here, in particular what “At the beginning of implementation” refer to?

> 在 2023/12/28 下午10:58, Tatsuyuki Ishi 写道:
>> The GOT assignment logic, likely copied from another backend, does not in
>> fact require multiple flags to construct multiple type of slots. Instead,
>> all slots are initialized on the first relocation encountered. Update
>> comment to avoid confusion.
>> ---
>>  bfd/elfnn-loongarch.c | 17 +++++++----------
>>  1 file changed, 7 insertions(+), 10 deletions(-)
>> 
>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>> index 64c34e99261..d66dcee1100 100644
>> --- a/bfd/elfnn-loongarch.c
>> +++ b/bfd/elfnn-loongarch.c
>> @@ -3656,16 +3656,13 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>>  	  relocation -= elf_hash_table (info)->tls_sec->vma;
>>  	  break;
>>  -	/* TLS IE LD/GD process separately is troublesome.
>> -	   When a symbol is both ie and LD/GD, h->got.off |= 1
>> -	   make only one type be relocated.  We must use
>> -	   h->got.offset |= 1 and h->got.offset |= 2
>> -	   diff IE and LD/GD.  And all (got_off & (~(bfd_vma)1))
>> -	   (IE LD/GD and reusable GOT reloc) must change to
>> -	   (got_off & (~(bfd_vma)3)), beause we use lowest 2 bits
>> -	   as a tag.
>> -	   Now, LD and GD is both GOT_TLS_GD type, LD seems to
>> -	   can be omitted.  */
>> +	/* The bottom bit of h->got.offset is (ab)used as a flag.
>> +	   Upon encountering the first TLS relocation, we initialize all GOT
>> +	   slots for the corresponding symbol and set the bottom bit to 1.
>> +
>> +	   The second and subsequent relocations will check the flag, see that
>> +	   the slot is already initialized, and move on to just relocating the
>> +	   entry.  */
>>  	case R_LARCH_TLS_IE_PC_HI20:
>>  	case R_LARCH_TLS_IE_HI20:
>>  	case R_LARCH_TLS_LD_PC_HI20:
>
mengqinggang Dec. 29, 2023, 9:59 a.m. UTC | #3 
These comments mainly explain the issue of another implementation method.


If the code like below, TLS IE and GD processed separately, we need two 
bits as flags.

case  R_LARCH_TLS_IE_PC_HI20:
     if ((got_off & 1) == 0)
         {
             ...
         }

case R_LARCH_TLS_GD_PC_HI20:
     if ((got_off & 2) == 0)
         {
             ...
         }


在 2023/12/29 下午5:11, Tatsuyuki Ishi 写道:
>> On Dec 29, 2023, at 17:50, mengqinggang <mengqinggang@loongson.cn> wrote:
>>
>> At the beginning of implementation, we try to IE and GD only generate their own GOT entry,
>> so we need two bits to do the flags.
> I checked the tree back at the date the comment was introduced (6d13722a: "bfd: Add supported for LoongArch new relocations.”). Even at that time, there was no |= 2 accesses, so if my understanding is correct, this comment was wrong from the beginning.
>
> Could you clarify what you mean here, in particular what “At the beginning of implementation” refer to?
>
>> 在 2023/12/28 下午10:58, Tatsuyuki Ishi 写道:
>>> The GOT assignment logic, likely copied from another backend, does not in
>>> fact require multiple flags to construct multiple type of slots. Instead,
>>> all slots are initialized on the first relocation encountered. Update
>>> comment to avoid confusion.
>>> ---
>>>   bfd/elfnn-loongarch.c | 17 +++++++----------
>>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>>> index 64c34e99261..d66dcee1100 100644
>>> --- a/bfd/elfnn-loongarch.c
>>> +++ b/bfd/elfnn-loongarch.c
>>> @@ -3656,16 +3656,13 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>>>   	  relocation -= elf_hash_table (info)->tls_sec->vma;
>>>   	  break;
>>>   -	/* TLS IE LD/GD process separately is troublesome.
>>> -	   When a symbol is both ie and LD/GD, h->got.off |= 1
>>> -	   make only one type be relocated.  We must use
>>> -	   h->got.offset |= 1 and h->got.offset |= 2
>>> -	   diff IE and LD/GD.  And all (got_off & (~(bfd_vma)1))
>>> -	   (IE LD/GD and reusable GOT reloc) must change to
>>> -	   (got_off & (~(bfd_vma)3)), beause we use lowest 2 bits
>>> -	   as a tag.
>>> -	   Now, LD and GD is both GOT_TLS_GD type, LD seems to
>>> -	   can be omitted.  */
>>> +	/* The bottom bit of h->got.offset is (ab)used as a flag.
>>> +	   Upon encountering the first TLS relocation, we initialize all GOT
>>> +	   slots for the corresponding symbol and set the bottom bit to 1.
>>> +
>>> +	   The second and subsequent relocations will check the flag, see that
>>> +	   the slot is already initialized, and move on to just relocating the
>>> +	   entry.  */
>>>   	case R_LARCH_TLS_IE_PC_HI20:
>>>   	case R_LARCH_TLS_IE_HI20:
>>>   	case R_LARCH_TLS_LD_PC_HI20:
diff mbox series

Patch

diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
index 64c34e99261..d66dcee1100 100644
--- a/bfd/elfnn-loongarch.c
+++ b/bfd/elfnn-loongarch.c
@@ -3656,16 +3656,13 @@  loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
 	  relocation -= elf_hash_table (info)->tls_sec->vma;
 	  break;
 
-	/* TLS IE LD/GD process separately is troublesome.
-	   When a symbol is both ie and LD/GD, h->got.off |= 1
-	   make only one type be relocated.  We must use
-	   h->got.offset |= 1 and h->got.offset |= 2
-	   diff IE and LD/GD.  And all (got_off & (~(bfd_vma)1))
-	   (IE LD/GD and reusable GOT reloc) must change to
-	   (got_off & (~(bfd_vma)3)), beause we use lowest 2 bits
-	   as a tag.
-	   Now, LD and GD is both GOT_TLS_GD type, LD seems to
-	   can be omitted.  */
+	/* The bottom bit of h->got.offset is (ab)used as a flag.
+	   Upon encountering the first TLS relocation, we initialize all GOT
+	   slots for the corresponding symbol and set the bottom bit to 1.
+
+	   The second and subsequent relocations will check the flag, see that
+	   the slot is already initialized, and move on to just relocating the
+	   entry.  */
 	case R_LARCH_TLS_IE_PC_HI20:
 	case R_LARCH_TLS_IE_HI20:
 	case R_LARCH_TLS_LD_PC_HI20: