Re: [PATCH 1/5] nptl: Add <thread_pointer.h> for defining __thread_pointer

[Florian Weimer <fweimer@redhat.com>]

* Mathieu Desnoyers:

> Why do we find a mix of macros and static inline across architectures ?

It's following the existing snippets in <tls.h> for those architectures
which cannot use __builtin_thread_pointer.

> Defining macros end up clobbering the preprocessor namespace (output
> of "gcc -dM -E"), which is one reason why static inlines are sometimes
> preferred. Or do we intend users to use this for feature detection ?
> e.g. #ifdef __thread_pointer ?

It's currently not an installed header, so this isn't a concern.

>
> OTOH, static inline functions will not be inlined when compiling with "-fno-inline"
> (default at -O0). An attribute "always_inline" is needed for this.
>
>
>> +++ b/sysdeps/m68k/nptl/thread_pointer.h
> [...]
>> +extern void * __m68k_read_tp (void);
>> +#define __thread_pointer() __m68k_read_tp ()
> [...]
>
>> +++ b/sysdeps/microblaze/nptl/thread_pointer.h
> [...]
>> +#define __thread_pointer() ({ register void *__reg asm ("r21"); __reg; })
>
> Not that applications are supposed to use variable names with a leading
> "__" prefix (AFAIU this is reserved to libc), but one downside of these
> statement expressions is that they would shadow any "__reg" variables.

It doesn't matter in this context because there are no macro arguments.
The only thing that would break this would be a __reg macro.

>> +++ b/sysdeps/x86/nptl/thread_pointer.h
> [...]
>> +#if __GNUC_PREREQ (11, 1)
>
> I suspect this requires a #include "features.h" ?

It's an internal header.  We'd also need include guards if we made this
into an installed header.

>> +# define __thread_pointer() __builtin_thread_pointer ()
>> +#else
>> +static inline void *
>> +__thread_pointer (void)
>> +{
>> +  void *__result;
>> +# ifdef __x86_64__
>> +  __asm__ volatile ("mov %%fs:0, %0" : "=r" (__result));
>
> Just out of curiosity, is x32 supported by glibc, and is this the
> expected behavior ?

x32 is supported and uses %fs.

>> +# else
>> +  __asm__ volatile ("mov %%gs:0, %0" : "=r" (__result));
>
> Do we really want a "volatile" asm here ? E.g. see:

With sufficiently separate compilation, we do not need it.  It's
currently not needed.

THREAD_SELF should not use it because it is often used in ways that
isn't needed after all, e.g. if THREAD_SELF is passed to THREAD_GETMEM
on an architecture that does not need a materialized thread pointer to
change TCB fields.

> Is this expected to be a complete list of all architectures ? I
> notably identify that arm, aarch64, and s390 are missing. Or is it
> that all the other architectures map to
> "sysdeps/nptl/thread_pointers.h" ? Maybe it would be good to document
> this in the commit message.

Yes, those can use __builtin_thread_pointer.  The patch as is builds on
all targets with GCC 11.

Thanks,
Florian