[PATCH v14 0/9] Add rseq extensible ABI support

[Michael Jeanson]

On 2025-01-07 06:15, Florian Weimer wrote:
> * Michael Jeanson:
> 
>> Your patch looks good to me but it's missing the same change for the
>> statically linked binary code, like this :
>>
>> diff --git a/csu/libc-tls.c b/csu/libc-tls.c                                                                                                 index 20d6b48a4d..65ce9ddb3d 100644
>> --- a/csu/libc-tls.c
>> +++ b/csu/libc-tls.c
>> @@ -20,6 +20,7 @@
>>  #include <errno.h>
>>  #include <ldsodefs.h>
>>  #include <tls.h>
>> +#include <dl-tls.h>
>>  #include <unistd.h>
>>  #include <stdio.h>
>>  #include <sys/param.h>
>> @@ -195,7 +196,7 @@ __libc_setup_tls (void)
>>      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);
>> +  _dl_extra_tls_set_offset(tls_blocks_size - extra_tls_size - TLS_TP_OFFSET);
>>  
>>    tlsblock = _dl_early_allocate (tls_blocks_size + max_align
>>                                  + TLS_PRE_TCB_SIZE
>>
> 
> Ah, makes sense.
> 
> I've worked on posting a patch for exposing TLS_TP_OFFSET
> unconditionally, but the test case for it hits a snag: we still don't
> have <thread_pointer.h> on all architectures.  I see build failures on:
> 
>   alpha
>   arc
>   arm
>   mips
>   s390
>   s390x
>   sh
> 
> Of these, only mips and sh do not yet assume that GCC's
> __builtin_thread_pointer work.
> 
> We could disable the new test, but perhaps we should complete the
> <thread_pointer.h> implementation?  Not sure where this puts us in terms
> of inclusion in the upcoming release.  I'll try to add a generic
> <thread_pointer.h> and implementations for mips and sh.

I agree we should complete the <thread_pointer.h> implementation, I was
under the impression it was mostly done with my previous patches. I'll
have a look at the architectures you listed.

I can do mips and sh if you haven't already started and it saves you
some time?

> 
> I also looked at an alternative implementation: make the size of struct
> pthread dynamic.  This minimizes changes on architectures such as
> x86-64, at least if we assume that 32-byte alignment of the rseq area is
> enough.  It's more complicated on architectures such as AArch64, where
> THREAD_SELF would need to perform variable adjustment.  It does not look
> like an overall win in terms of complexity.

I don't think we can assume 32-byte alignment will always be enough and
as you say it doesn't seem to reduce complexity compared to this patchset.

> 
> Michael, before reposting your changes, you could fix some style nits?
> Mostly missing space before '(' in function calls, missing two spaces
> after '.' at the end of the comment.  Copyright years will need
> updating, too.

Will do.

> 
> grep -E '^\+.*(Contributed by|http://|Copyright.*(19[0-9][0-9]|20[01][0-9]|202[0-4])|XXX|FIXME|TODO|\?\?\?|internal_function|\bu_(char|int)|the\s+the|[^	 *$({~]\(|\b(if|for|while|do)\b.*\{|[[:space:]]$|\($|\. \*/)'
> 
> Thanks,
> Florian
>