powerpc: Use global register variable in <thread_pointer.h>

  A local register variable is merely a compiler hint, and so not
appropriate in this context.  Move the global register variable into
<thread_pointer.h> and include it from <tls.h>, as there can only
be one global definition for one particular register.

Fixes commit 8dbeb0561eeb876f557ac9eef5721912ec074ea5
("nptl: Add <thread_pointer.h> for defining __thread_pointer").

Built with build-many-glibcs.py.

Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

---
 sysdeps/powerpc/nptl/thread_pointer.h | 13 +++++++------
 sysdeps/powerpc/nptl/tls.h            |  7 +------
 2 files changed, 8 insertions(+), 12 deletions(-)
  

LGTM

Reviewed-by: Raphael M Zinsly <rzinsly@linux.ibm.com>

On 15/12/2021 06:54, Florian Weimer via Libc-alpha wrote:
> A local register variable is merely a compiler hint, and so not
> appropriate in this context.  Move the global register variable into
> <thread_pointer.h> and include it from <tls.h>, as there can only
> be one global definition for one particular register.
> 
> Fixes commit 8dbeb0561eeb876f557ac9eef5721912ec074ea5
> ("nptl: Add <thread_pointer.h> for defining __thread_pointer").
> 
> Built with build-many-glibcs.py.
> 
> Reported-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> 
> ---
>   sysdeps/powerpc/nptl/thread_pointer.h | 13 +++++++------
>   sysdeps/powerpc/nptl/tls.h            |  7 +------
>   2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/powerpc/nptl/thread_pointer.h b/sysdeps/powerpc/nptl/thread_pointer.h
> index 8fd5ba671f..4feba59610 100644
> --- a/sysdeps/powerpc/nptl/thread_pointer.h
> +++ b/sysdeps/powerpc/nptl/thread_pointer.h
> @@ -19,15 +19,16 @@
>   #ifndef _SYS_THREAD_POINTER_H
>   #define _SYS_THREAD_POINTER_H
> 
> -static inline void *
> -__thread_pointer (void)
> -{
>   #ifdef __powerpc64__
> -  register void *__result asm ("r13");
> +register void *__thread_register asm ("r13");
>   #else
> -  register void *__result asm ("r2");
> +register void *__thread_register asm ("r2");
>   #endif
> -  return __result;
> +
> +static inline void *
> +__thread_pointer (void)
> +{
> +  return __thread_register;
>   }
> 
>   #endif /* _SYS_THREAD_POINTER_H */
> diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h
> index 63098f4048..d03fcb6983 100644
> --- a/sysdeps/powerpc/nptl/tls.h
> +++ b/sysdeps/powerpc/nptl/tls.h
> @@ -26,6 +26,7 @@
>   # include <stddef.h>
>   # include <stdint.h>
>   # include <dl-dtv.h>
> +# include <thread_pointer.h>
> 
>   #else /* __ASSEMBLER__ */
>   # include <tcb-offsets.h>
> @@ -36,16 +37,10 @@
>   #ifndef __powerpc64__
>   /* Register r2 (tp) is reserved by the ABI as "thread pointer". */
>   # define PT_THREAD_POINTER PT_R2
> -# ifndef __ASSEMBLER__
> -register void *__thread_register __asm__ ("r2");
> -# endif
> 
>   #else /* __powerpc64__ */
>   /* Register r13 (tp) is reserved by the ABI as "thread pointer". */
>   # define PT_THREAD_POINTER PT_R13
> -# ifndef __ASSEMBLER__
> -register void *__thread_register __asm__ ("r13");
> -# endif
>   #endif /* __powerpc64__ */
> 
>   #ifndef __ASSEMBLER__
>
  

----- On Dec 15, 2021, at 4:54 AM, Florian Weimer fweimer@redhat.com wrote:

> A local register variable is merely a compiler hint, and so not
> appropriate in this context.  Move the global register variable into
> <thread_pointer.h> and include it from <tls.h>, as there can only
> be one global definition for one particular register.

I'm considering an alternative approach to solve this. Perhaps the global
definition is fine as long as it is never exposed in a public header, but
in the context of librseq, I would prefer that rseq_thread_pointer() (which
will be in a public header) does not conflict with a global resource.

For instance, a library/application already defining its own global variable:

register void *myvar asm ("r2");

would not be able to include a header also declaring a global definition
for register "r2".

I suspect that you chose a global definition because the local register
variable is "just a hint" to the compiler. (if there are other motivations
for your choice please let me know) AFAIU, it's just a hint when it is
not used by an extended ASM:

https://gcc.gnu.org/onlinedocs/gcc-6.1.0/gcc/Local-Register-Variables.html

"The only supported use for this feature is to specify registers for input
and output operands when calling Extended asm (see Extended Asm). This may
be necessary if the constraints for a particular machine don't provide sufficient
control to select the desired register. To force an operand into a register,
create a local variable and specify the register name after the variable's
declaration. Then use the local variable for the asm operand and specify
any constraint letter that matches the register: [...]"

Do you see anything wrong with the following implementation ?

static inline void *rseq_thread_pointer(void)
{
#ifdef __powerpc64__
	register void *__result asm ("r13");
#else
	register void *__result asm ("r2");
#endif
	asm ("" : "=r" (__result));
	return __result;
}

Thanks,

Mathieu
  

* Mathieu Desnoyers:

> For instance, a library/application already defining its own global variable:
>
> register void *myvar asm ("r2");
>
> would not be able to include a header also declaring a global definition
> for register "r2".

This may be a good thing, though, if the uses are actually incompatible.
Even local register variables (correctly used) could clash, see below.

> Do you see anything wrong with the following implementation ?
>
> static inline void *rseq_thread_pointer(void)
> {
> #ifdef __powerpc64__
> 	register void *__result asm ("r13");
> #else
> 	register void *__result asm ("r2");
> #endif
> 	asm ("" : "=r" (__result));
> 	return __result;
> }

If there is a global register variable in scope for r13/r2, it is
unclear whether the compiler is expected to spill and restore it around
the inline assembly.  It's just a performance issue in this case, I
think.  It would still be annoying if it happened.

Thanks,
Florian
  

----- On Dec 15, 2021, at 10:34 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>> For instance, a library/application already defining its own global variable:
>>
>> register void *myvar asm ("r2");
>>
>> would not be able to include a header also declaring a global definition
>> for register "r2".
> 
> This may be a good thing, though, if the uses are actually incompatible.
> Even local register variables (correctly used) could clash, see below.

Two headers independently trying to read the content of the ABI-defined
r2 register appear to be perfectly legitimate use-cases.

> 
>> Do you see anything wrong with the following implementation ?
>>
>> static inline void *rseq_thread_pointer(void)
>> {
>> #ifdef __powerpc64__
>> 	register void *__result asm ("r13");
>> #else
>> 	register void *__result asm ("r2");
>> #endif
>> 	asm ("" : "=r" (__result));
>> 	return __result;
>> }
> 
> If there is a global register variable in scope for r13/r2, it is
> unclear whether the compiler is expected to spill and restore it around
> the inline assembly.  It's just a performance issue in this case, I
> think.  It would still be annoying if it happened.

True. However, if both glibc and rseq agree on using local register
variables with the asm output operand rather than the global register
variable in public headers, we would not have to worry about this.

Thanks,

Mathieu
  

* Mathieu Desnoyers:

> ----- On Dec 15, 2021, at 10:34 AM, Florian Weimer fweimer@redhat.com wrote:
>
>> * Mathieu Desnoyers:
>> 
>>> For instance, a library/application already defining its own global variable:
>>>
>>> register void *myvar asm ("r2");
>>>
>>> would not be able to include a header also declaring a global definition
>>> for register "r2".
>> 
>> This may be a good thing, though, if the uses are actually incompatible.
>> Even local register variables (correctly used) could clash, see below.
>
> Two headers independently trying to read the content of the ABI-defined
> r2 register appear to be perfectly legitimate use-cases.

Then why does GCC cause these two declarations (definitions) to clash?

>>> Do you see anything wrong with the following implementation ?
>>>
>>> static inline void *rseq_thread_pointer(void)
>>> {
>>> #ifdef __powerpc64__
>>> 	register void *__result asm ("r13");
>>> #else
>>> 	register void *__result asm ("r2");
>>> #endif
>>> 	asm ("" : "=r" (__result));
>>> 	return __result;
>>> }
>> 
>> If there is a global register variable in scope for r13/r2, it is
>> unclear whether the compiler is expected to spill and restore it around
>> the inline assembly.  It's just a performance issue in this case, I
>> think.  It would still be annoying if it happened.
>
> True. However, if both glibc and rseq agree on using local register
> variables with the asm output operand rather than the global register
> variable in public headers, we would not have to worry about this.

Fair point.  GCC should simply support __builtin_thread_pointer on
POWER, though. 8-)

Thanks,
Florian
  

----- On Dec 15, 2021, at 10:49 AM, Florian Weimer fweimer@redhat.com wrote:

> * Mathieu Desnoyers:
> 
>> ----- On Dec 15, 2021, at 10:34 AM, Florian Weimer fweimer@redhat.com wrote:
>>
>>> * Mathieu Desnoyers:
>>> 
>>>> For instance, a library/application already defining its own global variable:
>>>>
>>>> register void *myvar asm ("r2");
>>>>
>>>> would not be able to include a header also declaring a global definition
>>>> for register "r2".
>>> 
>>> This may be a good thing, though, if the uses are actually incompatible.
>>> Even local register variables (correctly used) could clash, see below.
>>
>> Two headers independently trying to read the content of the ABI-defined
>> r2 register appear to be perfectly legitimate use-cases.
> 
> Then why does GCC cause these two declarations (definitions) to clash?

Not sure why, but with:

register void *abc asm ("r2");
register void *def asm ("r2");

building in a ppc32 environment:

In file included from ../include/rseq/rseq-thread-pointer.h:14,
                 from ../include/rseq/rseq.h:55,
                 from basic_percpu_ops_test.c:14:
../include/rseq/rseq-ppc-thread-pointer.h:16:16: warning: register of ‘def’ used for multiple global register variables
   16 | register void *def asm ("r2");
      |                ^~~
../include/rseq/rseq-ppc-thread-pointer.h:15:16: note: conflicts with ‘abc’
   15 | register void *abc asm ("r2");
      |      

gcc 11.2.0 emits a warning. It's not an error, but nevertheless this is
something we'd like to avoid, because it would break builds with -Werror.

> 
>>>> Do you see anything wrong with the following implementation ?
>>>>
>>>> static inline void *rseq_thread_pointer(void)
>>>> {
>>>> #ifdef __powerpc64__
>>>> 	register void *__result asm ("r13");
>>>> #else
>>>> 	register void *__result asm ("r2");
>>>> #endif
>>>> 	asm ("" : "=r" (__result));
>>>> 	return __result;
>>>> }
>>> 
>>> If there is a global register variable in scope for r13/r2, it is
>>> unclear whether the compiler is expected to spill and restore it around
>>> the inline assembly.  It's just a performance issue in this case, I
>>> think.  It would still be annoying if it happened.
>>
>> True. However, if both glibc and rseq agree on using local register
>> variables with the asm output operand rather than the global register
>> variable in public headers, we would not have to worry about this.
> 
> Fair point.  GCC should simply support __builtin_thread_pointer on
> POWER, though. 8-)

That would be even better! :) OK so for now, I'll use the local register
variable + asm trick in librseq public headers.

Thanks,

Mathieu