Re: [PATCH v5 2/5] LoongArch: Add support for TLSDESC in ld.

[Lulu Cai <cailulu@loongson.cn>]

On 2023/12/28 at 10:54 PM, Tatsuyuki Ishi Wrote:
> Hi, author of RISC-V TLSDESC patches here. I recently learned that LoongArch is working on a similar TLSDESC extension too. I’ll leave some of my thoughts on the patch set.
Thank you for your suggestions, I will add a reply after these suggestions.
>
>> On Dec 22, 2023, at 20:42, Lulu Cai <cailulu@loongson.cn> wrote:
>>
>> 1.The linker for each DESC generates a R_LARCH_TLS_DESC64 dynamic
>>   relocation, which relocation is placed at .rela.dyn.
>>   TLSDESC always allocates two GOT slots and one dynamic relocation
>>   space to TLSDESC.
>> 2. When using multiple ways to access the same TLS variable, a
>>    maximum of 5 GOT slots are used. For example, using GD, TLSDESC,
>>    and IE to access the same TLS variable, GD always uses the first
>>    two of the five GOT, TLSDESC uses the third and fourth, and IE
>>    uses the last.
>> ---
>> bfd/elfnn-loongarch.c | 168 ++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 146 insertions(+), 22 deletions(-)
>>
>> diff --git a/bfd/elfnn-loongarch.c b/bfd/elfnn-loongarch.c
>> index faffe27294a..95a39148f73 100644
>> --- a/bfd/elfnn-loongarch.c
>> +++ b/bfd/elfnn-loongarch.c
>> @@ -48,6 +48,12 @@ struct loongarch_elf_link_hash_entry
>> #define GOT_TLS_GD  2
>> #define GOT_TLS_IE  4
>> #define GOT_TLS_LE  8
>> +#define GOT_TLS_GDESC 16
>> +
>> +#define GOT_TLS_GD_BOTH_P(tls_type) \
>> +  ((tls_type & GOT_TLS_GD) && (tls_type & GOT_TLS_GDESC))
>> +#define GOT_TLS_GD_ANY_P(tls_type) \
>> +  ((tls_type & GOT_TLS_GD) || (tls_type & GOT_TLS_GDESC))
>>    char tls_type;
>> };
>>
>> @@ -563,6 +569,7 @@ loongarch_elf_record_tls_and_got_reference (bfd *abfd,
>>      case GOT_NORMAL:
>>      case GOT_TLS_GD:
>>      case GOT_TLS_IE:
>> +    case GOT_TLS_GDESC:
>>        /* Need GOT.  */
>>        if (htab->elf.sgot == NULL
>> 	  && !loongarch_elf_create_got_section (htab->elf.dynobj, info))
>> @@ -750,6 +757,14 @@ loongarch_elf_check_relocs (bfd *abfd, struct bfd_link_info *info,
>> 	    return false;
>> 	  break;
>>
>> +	case R_LARCH_TLS_DESC_PC_HI20:
>> +	case R_LARCH_TLS_DESC_HI20:
>> +	  if (!loongarch_elf_record_tls_and_got_reference (abfd, info, h,
>> +							   r_symndx,
>> +							   GOT_TLS_GDESC))
>> +	    return false;
>> +	  break;
>> +
>> 	case R_LARCH_ABS_HI20:
>> 	case R_LARCH_SOP_PUSH_ABSOLUTE:
>> 	  if (h != NULL)
>> @@ -1130,7 +1145,7 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf)
>>
>>        s = htab->elf.sgot;
>>        h->got.offset = s->size;
>> -      if (tls_type & (GOT_TLS_GD | GOT_TLS_IE))
>> +      if (tls_type & (GOT_TLS_GD | GOT_TLS_IE | GOT_TLS_GDESC))
>> 	{
>> 	  /* TLS_GD needs two dynamic relocs and two GOT slots.  */
>> 	  if (tls_type & GOT_TLS_GD)
>> @@ -1167,7 +1182,15 @@ allocate_dynrelocs (struct elf_link_hash_entry *h, void *inf)
>> 		  htab->elf.srelgot->size += sizeof (ElfNN_External_Rela);
>> 		}
>> 	    }
>> +
>> +	  /* TLS_DESC needs one dynamic reloc and two GOT slot.  */
>> +	  if (tls_type & GOT_TLS_GDESC)
>> +	    {
>> +	      s->size += GOT_ENTRY_SIZE * 2;
>> +	      htab->elf.srelgot->size += sizeof (ElfNN_External_Rela);
>> +	    }
>> 	}
>> +
>>        else
>> 	{
>> 	  s->size += GOT_ENTRY_SIZE;
>> @@ -1670,19 +1693,34 @@ loongarch_elf_size_dynamic_sections (bfd *output_bfd,
>> 	  if (0 < *local_got)
>> 	    {
>> 	      *local_got = s->size;
>> +	      if (*local_tls_type & (GOT_TLS_GD | GOT_TLS_IE | GOT_TLS_GDESC))
>> +		{
>> +		  /* TLS gd use two got.  */
>> +		  if (*local_tls_type & GOT_TLS_GD)
>> +		    {
>> +		      s->size += 2 * GOT_ENTRY_SIZE;
>> +		      if (!bfd_link_executable (info))
>> +			srel->size += sizeof (ElfNN_External_Rela);
>> +		    }
>>
>> -	      /* TLS gd use two got.  */
>> -	      if (*local_tls_type & GOT_TLS_GD)
>> -		s->size += GOT_ENTRY_SIZE * 2;
>> -	      else
>> -		/* Normal got, tls ie/ld use one got.  */
>> -		s->size += GOT_ENTRY_SIZE;
>> +		  /* TLS_DESC use two got.  */
>> +		  if (*local_tls_type & GOT_TLS_GDESC)
>> +		    {
>> +		      s->size += 2 * GOT_ENTRY_SIZE;
>> +		      srel->size += sizeof (ElfNN_External_Rela);
>> +		    }
>>
>> -	      if (bfd_link_executable (info)
>> -		  && (*local_tls_type & (GOT_TLS_GD| GOT_TLS_IE)))
>> -		;/* Do nothing.  */
>> +		  /* TLS ie and use one got.  */
>> +		  if (*local_tls_type & GOT_TLS_IE)
>> +		    {
>> +		      s->size += GOT_ENTRY_SIZE;
>> +		      if (!bfd_link_executable (info))
>> +			srel->size += sizeof (ElfNN_External_Rela);
>> +		    }
>> +		}
>> 	      else
>> 		{
>> +		  s->size += GOT_ENTRY_SIZE;
>> 		  srel->size += sizeof (ElfNN_External_Rela);
>> 		}
>> 	    }
>> @@ -2126,6 +2164,15 @@ perform_relocation (const Elf_Internal_Rela *rel, asection *input_section,
>>      case R_LARCH_TLS_GD_HI20:
>>      case R_LARCH_PCREL20_S2:
>>      case R_LARCH_CALL36:
>> +    case R_LARCH_TLS_DESC_PC_HI20:
>> +    case R_LARCH_TLS_DESC_PC_LO12:
>> +    case R_LARCH_TLS_DESC64_PC_LO20:
>> +    case R_LARCH_TLS_DESC64_PC_HI12:
>> +    case R_LARCH_TLS_DESC_HI20:
>> +    case R_LARCH_TLS_DESC_LO12:
>> +    case R_LARCH_TLS_DESC64_LO20:
>> +    case R_LARCH_TLS_DESC64_HI12:
>> +
>>        r = loongarch_check_offset (rel, input_section);
>>        if (r != bfd_reloc_ok)
>> 	break;
>> @@ -2135,6 +2182,11 @@ perform_relocation (const Elf_Internal_Rela *rel, asection *input_section,
>> 					    contents, value);
>>        break;
>>
>> +    case R_LARCH_TLS_DESC_LD:
>> +    case R_LARCH_TLS_DESC_CALL:
>> +      r = bfd_reloc_ok;
>> +      break;
>> +
>>      case R_LARCH_RELAX:
>>        break;
>>
>> @@ -2383,10 +2435,10 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>>        struct elf_link_hash_entry *h = NULL;
>>        const char *name;
>>        bfd_reloc_status_type r = bfd_reloc_ok;
>> -      bool is_ie, is_undefweak, unresolved_reloc, defined_local;
>> +      bool is_ie, is_desc, is_undefweak, unresolved_reloc, defined_local;
>>        bool resolved_local, resolved_dynly, resolved_to_const;
>>        char tls_type;
>> -      bfd_vma relocation, off, ie_off;
>> +      bfd_vma relocation, off, ie_off, desc_off;
>>        int i, j;
>>
>>        howto = loongarch_elf_rtype_to_howto (input_bfd, r_type);
>> @@ -2515,6 +2567,7 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>>
>>        BFD_ASSERT (!resolved_local || defined_local);
>>
>> +      is_desc = false;
>>        is_ie = false;
>>        switch (r_type)
>> 	{
>> @@ -3405,6 +3458,8 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>> 	case R_LARCH_TLS_LD_HI20:
>> 	case R_LARCH_TLS_GD_PC_HI20:
>> 	case R_LARCH_TLS_GD_HI20:
>> +	case R_LARCH_TLS_DESC_PC_HI20:
>> +	case R_LARCH_TLS_DESC_HI20:
>> 	  BFD_ASSERT (rel->r_addend == 0);
>> 	  unresolved_reloc = false;
>>
>> @@ -3412,6 +3467,10 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>> 	      || r_type == R_LARCH_TLS_IE_HI20)
>> 	    is_ie = true;
>>
>> +	  if (r_type == R_LARCH_TLS_DESC_PC_HI20
>> +	      || r_type == R_LARCH_TLS_DESC_HI20)
>> +	    is_desc = true;
>> +
> I see that the got_off assignment logic is duplicated for R_LARCH_SOP_PUSH_TLS_{GOT, GD}, but that path was not modified in this patch. Is this OK? Ideally, the logic should not be duplicated at all.
R_LARCH_SOP_PUSH_TLS_{GOT, GD} are relocations in the old toolchain that 
are no longer used by the new toolchain. So we are not going to modify this.
>
>> 	  bfd_vma got_off = 0;
>> 	  if (h != NULL)
>> 	    {
>> @@ -3426,9 +3485,19 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>>
>> 	  BFD_ASSERT (got_off != MINUS_ONE);
>>
>> -	  ie_off = 0;
>> 	  tls_type = _bfd_loongarch_elf_tls_type (input_bfd, h, r_symndx);
>> -	  if ((tls_type & GOT_TLS_GD) && (tls_type & GOT_TLS_IE))
>> +
>> +	  /* If a tls variable is accessed in multiple ways, GD uses
>> +	     the first two slots of GOT, desc follows with two slots,
>> +	     and IE uses one slot at the end.  */
>> +	  desc_off = 0;
>> +	  if (GOT_TLS_GD_BOTH_P (tls_type))
>> +	    desc_off = 2 * GOT_ENTRY_SIZE;
>> +
>> +	  ie_off = 0;
>> +	  if (GOT_TLS_GD_BOTH_P (tls_type) && (tls_type & GOT_TLS_IE))
>> +	    ie_off = 4 * GOT_ENTRY_SIZE;
>> +	  else if (GOT_TLS_GD_ANY_P (tls_type) && (tls_type & GOT_TLS_IE))
> I think the calculation should be done additively, instead of checking for every possible combination. That is,
>
> off = 0;
> if (tls gd)
>    off += gd size;
> desc_off = off;
> if (tls ie)
>    off += desc size;
> ie_off = off;
>
> would be much simpler.
Agreed, I will add these changes in a later version.
>> 	    ie_off = 2 * GOT_ENTRY_SIZE;
>>
>> 	  if ((got_off & 1) == 0)
>> @@ -3477,6 +3546,21 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>> 		      loongarch_elf_append_rela (output_bfd, relgot, &rela);
>> 		    }
>> 		}
>> +	      if (tls_type & GOT_TLS_GDESC)
>> +		{
>> +		  /* Unless it is a static link, DESC always emits a
>> +		     dynamic relocation.  */
>> +		  int indx = h && h->dynindx != -1 ? h->dynindx : 0;
>> +		  rela.r_offset = sec_addr (got) + got_off + desc_off;
>> +		  rela.r_addend = 0;
>> +		  if (indx == 0)
>> +		    rela.r_addend = relocation - elf_hash_table (info)->tls_sec->vma;
>> +
>> +		  rela.r_info = ELFNN_R_INFO (indx, R_LARCH_TLS_DESCNN);
>> +		  loongarch_elf_append_rela (output_bfd, relgot, &rela);
>> +		  bfd_put_NN (output_bfd, 0,
>> +			      got->contents + got_off + desc_off);
>> +		}
>> 	      if (tls_type & GOT_TLS_IE)
>> 		{
>> 		  rela.r_offset = sec_addr (got) + got_off + ie_off;
>> @@ -3504,16 +3588,52 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>> 		    }
>> 		}
>> 	    }
>> -	  relocation = (got_off & (~(bfd_vma)1)) + sec_addr (got)
>> -			+ (is_ie ? ie_off : 0);
>> +	  relocation = (got_off & (~(bfd_vma)1)) + sec_addr (got);
>> +	  if (is_desc)
>> +	    relocation += desc_off;
>> +	  else if (is_ie)
>> +	    relocation += ie_off;
>>
>> 	  if (r_type == R_LARCH_TLS_LD_PC_HI20
>> 	      || r_type == R_LARCH_TLS_GD_PC_HI20
>> -	      || r_type == R_LARCH_TLS_IE_PC_HI20)
>> +	      || r_type == R_LARCH_TLS_IE_PC_HI20
>> +	      || r_type == R_LARCH_TLS_DESC_PC_HI20)
>> 	    RELOCATE_CALC_PC32_HI20 (relocation, pc);
>>
>> 	  break;
>>
>> +	case R_LARCH_TLS_DESC_PC_LO12:
>> +	case R_LARCH_TLS_DESC64_PC_LO20:
>> +	case R_LARCH_TLS_DESC64_PC_HI12:
>> +	case R_LARCH_TLS_DESC_LO12:
>> +	case R_LARCH_TLS_DESC64_LO20:
>> +	case R_LARCH_TLS_DESC64_HI12:
>> +	  {
>> +	    unresolved_reloc = false;
>> +
>> +	    if (h)
>> +	      relocation = sec_addr (got) + (h->got.offset & (~(bfd_vma)1));
>> +	    else
>> +	      relocation = sec_addr (got)
>> +			   + (local_got_offsets[r_symndx] & (~(bfd_vma)1));
>> +
>> +	    tls_type = _bfd_loongarch_elf_tls_type (input_bfd, h, r_symndx);
>> +	    /* Use both TLS_GD and TLS_DESC.  */
>> +	    if ((tls_type & GOT_TLS_GD) && (tls_type & GOT_TLS_GDESC))
>> +	      relocation += 2 * GOT_ENTRY_SIZE;
> For simplicity, I think the ie_off and desc_off calculation should be moved out of the switch. Then you can just reuse the desc/ie_off calculated there.
I will modify it in a later version.
>> +	  }
>> +
>> +	    if (r_type == R_LARCH_TLS_DESC64_PC_LO20
>> +		|| r_type == R_LARCH_TLS_DESC64_PC_HI12)
>> +	      RELOCATE_CALC_PC64_HI32 (relocation, pc);
>> +
>> +	    break;
>> +
>> +	case R_LARCH_TLS_DESC_LD:
>> +	case R_LARCH_TLS_DESC_CALL:
>> +	  unresolved_reloc = false;
>> +	  break;
>> +
>> 	case R_LARCH_TLS_IE_PC_LO12:
>> 	case R_LARCH_TLS_IE64_PC_LO20:
>> 	case R_LARCH_TLS_IE64_PC_HI12:
>> @@ -3523,14 +3643,17 @@ loongarch_elf_relocate_section (bfd *output_bfd, struct bfd_link_info *info,
>> 	  unresolved_reloc = false;
>>
>> 	  if (h)
>> -	    relocation = sec_addr (got) + (h->got.offset & (~(bfd_vma)3));
>> +	    relocation = sec_addr (got) + (h->got.offset & (~(bfd_vma)1));
>> 	  else
>> 	    relocation = sec_addr (got)
>> -	      + (local_got_offsets[r_symndx] & (~(bfd_vma)3));
>> +	      + (local_got_offsets[r_symndx] & (~(bfd_vma)1));
>>
>> 	  tls_type = _bfd_loongarch_elf_tls_type (input_bfd, h, r_symndx);
>> -	  /* Use both TLS_GD and TLS_IE.  */
>> -	  if ((tls_type & GOT_TLS_GD) && (tls_type & GOT_TLS_IE))
>> +	  /* Use TLS_GD TLS_DESC and TLS_IE.  */
>> +	  if (GOT_TLS_GD_BOTH_P (tls_type) && (tls_type & GOT_TLS_IE))
>> +	    relocation += 4 * GOT_ENTRY_SIZE;
>> +	  /* Use GOT_TLS_GD_ANY_P (tls_type) and TLS_IE.  */
>> +	  else if (GOT_TLS_GD_ANY_P (tls_type) && (tls_type & GOT_TLS_IE))
>> 	    relocation += 2 * GOT_ENTRY_SIZE;
>>
>> 	  if (r_type == R_LARCH_TLS_IE64_PC_LO20
>> @@ -4174,7 +4297,8 @@ loongarch_elf_finish_dynamic_symbol (bfd *output_bfd,
>>
>>    if (h->got.offset != MINUS_ONE
>>        /* TLS got entry have been handled in elf_relocate_section.  */
>> -      && !(loongarch_elf_hash_entry (h)->tls_type & (GOT_TLS_GD | GOT_TLS_IE))
>> +      && !(loongarch_elf_hash_entry (h)->tls_type
>> +	   & (GOT_TLS_GD | GOT_TLS_IE | GOT_TLS_GDESC))
>>        /* Have allocated got entry but not allocated rela before.  */
>>        && !UNDEFWEAK_NO_DYNAMIC_RELOC (info, h))
>>      {
>> -- 
>> 2.43.0
>>
>>