[PATCH v15 2/8] Add generic 'extra TLS'

[Michael Jeanson]

On 2025-01-08 05:26, Florian Weimer wrote:
> * Michael Jeanson:
> 
>> diff --git a/csu/libc-tls.c b/csu/libc-tls.c
>> index 15f470aa87..73ba399e8d 100644
>> --- a/csu/libc-tls.c
>> +++ b/csu/libc-tls.c
> 
>> + /* Record the extra TLS block offset from the thread pointer.
>> +
>> +    With TLS_DTV_AT_TP the TLS blocks are allocated after the thread pointer in
>> +    order. Our block is added last which results in it being the last in the
>> +    static TLS block, thus record the offset as the size of the static TLS
>> +    block minus the size of our block.
>> +
>> +    On some architectures the TLS blocks are offset from the thread pointer,
>> +    include this offset in the extra TLS block offset.
>> +
>> +    The alignment requirements of the pointer resulting from this offset and
>> +    the thread pointer are enforced by 'max_align' which is used to align the
>> +    tcb_offset.  */
>> +  _dl_extra_tls_set_offset (tls_blocks_size - extra_tls_size - TLS_TP_OFFSET);
> 
> Is the API contract that _dl_extra_tls_get_size () is a multiple of
> _dl_extra_tls_get_align ()?  It's not clear to me.  The !TLS_DTV_AT_TP
> code does not assume it, I think but the subtraction
> 
>   tls_blocks_size - extra_tls_size
> 
> in the expression above does: tls_blocks_size is aligned, but if
> extra_tls_size isn't a multiple, the difference is not.
> 
> I don't know what the Linux auxiliary vector interface promises.  If
> there is no alignment requirement, we should change how the difference
> is computed.

So _dl_extra_tls_get_size () is indeed a multiple of _dl_extra_tls_get_align (), it's
stated in the subsequent patch in the linux implementation of 'extra TLS'. I should
also explicitly state this in this patch.

For the TLS_DTV_AT_TP implementation, we compute tls_blocks_size by adding 
tcb_offset, memsz and extra_tls_size and then rounding up this whole thing to
extra_tls_align:

  tls_blocks_size = roundup (tcb_offset + memsz + extra_tls_size, extra_tls_align);

We then substract the extra_tls_size from it to get the rseq offset, since both
tls_blocks_size and extra_tls_size are aligned to extra_tls_align, the resulting
offset should also be.

I'll try to update the comments to make this clearer.


> 
>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
>> index c2d17265fb..97566ce45a 100644
>> --- a/elf/dl-tls.c
>> +++ b/elf/dl-tls.c
> 
>> +  /* Insert the extra TLS block after the last TLS block.  */
>> +
>> +  /* Extra TLS block for internal usage to append at the end of the TLS blocks
>> +     (in allocation order).  On Linux systems this is where the rseq area will
>> +     be allocated.  On other systems it is currently unused and both values
>> +     will be '0'.  */
>> +  size_t extra_tls_size = _dl_extra_tls_get_size ();
>> +  size_t extra_tls_align = _dl_extra_tls_get_align ();
>> +
>> +  /* Align and add the extra TLS block to the global offset.  */
>> +  offset = roundup (offset, extra_tls_align) + extra_tls_size;
> 
> Zero alignment passed to roundup?  I don't think that's valid.

Right, I misread the manpage, I should use this instead?

  offset = roundup (offset, extra_tls_align ?: 1) + extra_tls_size;

> 
>> @@ -368,6 +397,39 @@ _dl_determine_tlsoffset (void)
>>        offset = off + slotinfo[cnt].map->l_tls_blocksize - firstbyte;
>>      }
>>  
>> +  /* Insert the extra TLS block after the last TLS block.  */
>> +
>> +  /* Extra TLS block for internal usage to append at the end of the TLS blocks
>> +     (in allocation order).  On Linux systems this is where the rseq area will
>> +     be allocated.  On other systems it is currently unused and both values
>> +     will be '0'.  */
>> +  size_t extra_tls_size = _dl_extra_tls_get_size ();
>> +  size_t extra_tls_align = _dl_extra_tls_get_align ();
>> +
>> +  /* Align the global offset to the beginning of the extra TLS block.  */
>> +  offset = roundup (offset, extra_tls_align);
> 
> See above about zero argument.
> 
> Thanks,
> Florian
>