[PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[H.J. Lu]

Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
"movq reg64, mem64" to store 64-bit constant in TCB.
---
 sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..9ec8703e45 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
        asm volatile ("movl %0,%%fs:%P1" :				      \
 		     : IMM_MODE (value),				      \
 		       "i" (offsetof (struct pthread, member)));	      \
+     else if (__builtin_constant_p (value)				      \
+	      && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
+       asm volatile ("movq %0,%%fs:%P1" :				      \
+		     : "r" (value),					      \
+		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
 	 asm volatile ("movq %q0,%%fs:%P1" :				      \
@@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		     : IMM_MODE (value),				      \
 		       "i" (offsetof (struct pthread, member[0])),	      \
 		       "r" (idx));					      \
+     else if (__builtin_constant_p (value)				      \
+	      && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
+       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
+		     : "r" (value),					      \
+		       "i" (offsetof (struct pthread, member[0])),	      \
+		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
 	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-- 
2.29.2

Re: [PATCH] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Carlos O'Donell]

On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> "movq reg64, mem64" to store 64-bit constant in TCB.
> ---
>  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..9ec8703e45 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>         asm volatile ("movl %0,%%fs:%P1" :				      \
>  		     : IMM_MODE (value),				      \
>  		       "i" (offsetof (struct pthread, member)));	      \
> +     else if (__builtin_constant_p (value)				      \
> +	      && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> +       asm volatile ("movq %0,%%fs:%P1" :				      \
> +		     : "r" (value),					      \
> +		       "i" (offsetof (struct pthread, member)));	      \

(1) Move conditional into 'else /* 8 */' section.

The blocks of conditionals are predicated on the size of the member we are 
about to write to.

In the block below...

>       else /* 8 */							      \

... here, we are about to write value into a member that is size 8.

Your code changes the logical construction of the code, but in way that makes
it more difficult to understand.

We previously had:

if (sizeof() == 1)

else if (sizeof() == 4)

else /* Assume 8 */

In your case we must already be in the 'else /* Assume 8 */' because otherwise
we've be writing a 64-bit constant into a < 64-bit structure member.

I think we should put your code into the else clause.

272      else /* 8 */                                                             \
273        {                                                                      \

             if (__builtin_constant_p (value)
                 && ([the value is a >32-bit constant])
               asm volatile ([use movq reg64, mem64]);
             else

274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
276                          "i" (offsetof (struct pthread, member)));            \
277        }})

(2) What if gcc can't prove it's constant?

If the constant is >32-bit, but gcc can't prove it's constant, then don't we
try to put a >32-bit constant into a 32-bit immediate?

Shouldn't the code be structured the other way around?

e.g.

else /* 8 */
{
  if (__builtin_constant_p (value)
      && ([the value is a <= 32-bit constant])
    asm volatile ([use movq imm32, mem64]);
  else
    asm volatile ([use movq reg64, mem64]);
}

This way the code is always correct?

>         {								      \
>  	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		     : IMM_MODE (value),				      \
>  		       "i" (offsetof (struct pthread, member[0])),	      \
>  		       "r" (idx));					      \
> +     else if (__builtin_constant_p (value)				      \
> +	      && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
> +		     : "r" (value),					      \
> +		       "i" (offsetof (struct pthread, member[0])),	      \
> +		       "r" (idx));					      \
>       else /* 8 */							      \
>         {								      \
>  	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> 


-- 
Cheers,
Carlos.

[PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 5230 bytes --]

On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
> > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> > "movq reg64, mem64" to store 64-bit constant in TCB.
> > ---
> >  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> > index 20f0958780..9ec8703e45 100644
> > --- a/sysdeps/x86_64/nptl/tls.h
> > +++ b/sysdeps/x86_64/nptl/tls.h
> > @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >         asm volatile ("movl %0,%%fs:%P1" :                                  \
> >                    : IMM_MODE (value),                                      \
> >                      "i" (offsetof (struct pthread, member)));              \
> > +     else if (__builtin_constant_p (value)                                 \
> > +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> > +       asm volatile ("movq %0,%%fs:%P1" :                                  \
> > +                  : "r" (value),                                           \
> > +                    "i" (offsetof (struct pthread, member)));              \
>
> (1) Move conditional into 'else /* 8 */' section.
>
> The blocks of conditionals are predicated on the size of the member we are
> about to write to.
>
> In the block below...
>
> >       else /* 8 */                                                          \
>
> ... here, we are about to write value into a member that is size 8.
>
> Your code changes the logical construction of the code, but in way that makes
> it more difficult to understand.
>
> We previously had:
>
> if (sizeof() == 1)
>
> else if (sizeof() == 4)
>
> else /* Assume 8 */
>
> In your case we must already be in the 'else /* Assume 8 */' because otherwise
> we've be writing a 64-bit constant into a < 64-bit structure member.
>
> I think we should put your code into the else clause.
>
> 272      else /* 8 */                                                             \
> 273        {                                                                      \
>
>              if (__builtin_constant_p (value)
>                  && ([the value is a >32-bit constant])
>                asm volatile ([use movq reg64, mem64]);
>              else
>
> 274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
> 275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> 276                          "i" (offsetof (struct pthread, member)));            \
> 277        }})
>
> (2) What if gcc can't prove it's constant?
>
> If the constant is >32-bit, but gcc can't prove it's constant, then don't we
> try to put a >32-bit constant into a 32-bit immediate?
>
> Shouldn't the code be structured the other way around?
>
> e.g.
>
> else /* 8 */
> {
>   if (__builtin_constant_p (value)
>       && ([the value is a <= 32-bit constant])
>     asm volatile ([use movq imm32, mem64]);
>   else
>     asm volatile ([use movq reg64, mem64]);
> }
>
> This way the code is always correct?

I changed it to

         if (!__builtin_constant_p (value)                                    \
             || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
           asm volatile ("movq %q0,%%fs:%P1" :                                \
                         : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
                           "i" (offsetof (struct pthread, member)));          \
         else                                                                 \
           asm volatile ("movq %0,%%fs:%P1" :                                 \
                         : "r" (value),                                       \
                           "i" (offsetof (struct pthread, member)));          \

> >         {                                                                   \
> >        asm volatile ("movq %q0,%%fs:%P1" :                                  \
> > @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >                    : IMM_MODE (value),                                      \
> >                      "i" (offsetof (struct pthread, member[0])),            \
> >                      "r" (idx));                                            \
> > +     else if (__builtin_constant_p (value)                                 \
> > +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> > +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :                          \
> > +                  : "r" (value),                                           \
> > +                    "i" (offsetof (struct pthread, member[0])),            \
> > +                    "r" (idx));                                            \
> >       else /* 8 */                                                          \
> >         {                                                                   \
> >        asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :                          \
> >
>

Here is the v2 patch.  OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: v2-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --]
[-- Type: text/x-patch, Size: 2417 bytes --]

From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 8 Jan 2021 15:38:14 -0800
Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
"movq reg64, mem64" to store 64-bit constant.
---
 sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..0dbd66209c 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member)));	      \
+	 if (!__builtin_constant_p (value)				      \
+	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1" :				      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member)));	      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1" :				      \
+			 : "r" (value),					      \
+			   "i" (offsetof (struct pthread, member)));	      \
        }})
 
 
@@ -296,10 +302,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member[0])),	      \
-			 "r" (idx));					      \
+	 if (!__builtin_constant_p (value)				      \
+	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
+			 : "r" (value),					      \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
        }})
 
 
-- 
2.29.2

Re: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Carlos O'Donell]

On 3/2/21 9:21 AM, H.J. Lu wrote:
> On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
>>> "movq reg64, mem64" to store 64-bit constant in TCB.
>>> ---
>>>  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
>>> index 20f0958780..9ec8703e45 100644
>>> --- a/sysdeps/x86_64/nptl/tls.h
>>> +++ b/sysdeps/x86_64/nptl/tls.h
>>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>>>         asm volatile ("movl %0,%%fs:%P1" :                                  \
>>>                    : IMM_MODE (value),                                      \
>>>                      "i" (offsetof (struct pthread, member)));              \
>>> +     else if (__builtin_constant_p (value)                                 \
>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
>>> +       asm volatile ("movq %0,%%fs:%P1" :                                  \
>>> +                  : "r" (value),                                           \
>>> +                    "i" (offsetof (struct pthread, member)));              \
>>
>> (1) Move conditional into 'else /* 8 */' section.
>>
>> The blocks of conditionals are predicated on the size of the member we are
>> about to write to.
>>
>> In the block below...
>>
>>>       else /* 8 */                                                          \
>>
>> ... here, we are about to write value into a member that is size 8.
>>
>> Your code changes the logical construction of the code, but in way that makes
>> it more difficult to understand.
>>
>> We previously had:
>>
>> if (sizeof() == 1)
>>
>> else if (sizeof() == 4)
>>
>> else /* Assume 8 */
>>
>> In your case we must already be in the 'else /* Assume 8 */' because otherwise
>> we've be writing a 64-bit constant into a < 64-bit structure member.
>>
>> I think we should put your code into the else clause.
>>
>> 272      else /* 8 */                                                             \
>> 273        {                                                                      \
>>
>>              if (__builtin_constant_p (value)
>>                  && ([the value is a >32-bit constant])
>>                asm volatile ([use movq reg64, mem64]);
>>              else
>>
>> 274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
>> 275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
>> 276                          "i" (offsetof (struct pthread, member)));            \
>> 277        }})
>>
>> (2) What if gcc can't prove it's constant?
>>
>> If the constant is >32-bit, but gcc can't prove it's constant, then don't we
>> try to put a >32-bit constant into a 32-bit immediate?
>>
>> Shouldn't the code be structured the other way around?
>>
>> e.g.
>>
>> else /* 8 */
>> {
>>   if (__builtin_constant_p (value)
>>       && ([the value is a <= 32-bit constant])
>>     asm volatile ([use movq imm32, mem64]);
>>   else
>>     asm volatile ([use movq reg64, mem64]);
>> }
>>
>> This way the code is always correct?
> 
> I changed it to
> 
>          if (!__builtin_constant_p (value)                                    \
>              || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
>            asm volatile ("movq %q0,%%fs:%P1" :                                \
>                          : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
>                            "i" (offsetof (struct pthread, member)));          \
>          else                                                                 \
>            asm volatile ("movq %0,%%fs:%P1" :                                 \
>                          : "r" (value),                                       \
>                            "i" (offsetof (struct pthread, member)));          \
> 
>>>         {                                                                   \
>>>        asm volatile ("movq %q0,%%fs:%P1" :                                  \
>>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>>>                    : IMM_MODE (value),                                      \
>>>                      "i" (offsetof (struct pthread, member[0])),            \
>>>                      "r" (idx));                                            \
>>> +     else if (__builtin_constant_p (value)                                 \
>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
>>> +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :                          \
>>> +                  : "r" (value),                                           \
>>> +                    "i" (offsetof (struct pthread, member[0])),            \
>>> +                    "r" (idx));                                            \
>>>       else /* 8 */                                                          \
>>>         {                                                                   \
>>>        asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :                          \
>>>
>>
> 
> Here is the v2 patch.  OK for master?
> 
> Thanks.
> 

> From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 8 Jan 2021 15:38:14 -0800
> Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> 
> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> "movq reg64, mem64" to store 64-bit constant.
> ---
>  sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..0dbd66209c 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "i" (offsetof (struct pthread, member)));	      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member)));	      \
> +	 if (!__builtin_constant_p (value)				      \

Whis is this a "!__builtin_constant_p?"

I would have expected:

if ([the value is a constant a therefore known quantity]
    || [it is a 32-bit value])

Perhaps a quick single line comment here explaining the conditionals would help?

> +	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %q0,%%fs:%P1" :				      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member)));	      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1" :				      \
> +			 : "r" (value),					      \
> +			   "i" (offsetof (struct pthread, member)));	      \
>         }})
>  
>  
> @@ -296,10 +302,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "r" (idx));					      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member[0])),	      \
> -			 "r" (idx));					      \
> +	 if (!__builtin_constant_p (value)				      \
> +	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
> +			 : "r" (value),					      \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
>         }})
>  
>  
> -- 
> 2.29.2
> 

-- 
Cheers,
Carlos.

[PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 7429 bytes --]

On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 3/2/21 9:21 AM, H.J. Lu wrote:
> > On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
> >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> >>> "movq reg64, mem64" to store 64-bit constant in TCB.
> >>> ---
> >>>  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> >>> index 20f0958780..9ec8703e45 100644
> >>> --- a/sysdeps/x86_64/nptl/tls.h
> >>> +++ b/sysdeps/x86_64/nptl/tls.h
> >>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >>>         asm volatile ("movl %0,%%fs:%P1" :                                  \
> >>>                    : IMM_MODE (value),                                      \
> >>>                      "i" (offsetof (struct pthread, member)));              \
> >>> +     else if (__builtin_constant_p (value)                                 \
> >>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> >>> +       asm volatile ("movq %0,%%fs:%P1" :                                  \
> >>> +                  : "r" (value),                                           \
> >>> +                    "i" (offsetof (struct pthread, member)));              \
> >>
> >> (1) Move conditional into 'else /* 8 */' section.
> >>
> >> The blocks of conditionals are predicated on the size of the member we are
> >> about to write to.
> >>
> >> In the block below...
> >>
> >>>       else /* 8 */                                                          \
> >>
> >> ... here, we are about to write value into a member that is size 8.
> >>
> >> Your code changes the logical construction of the code, but in way that makes
> >> it more difficult to understand.
> >>
> >> We previously had:
> >>
> >> if (sizeof() == 1)
> >>
> >> else if (sizeof() == 4)
> >>
> >> else /* Assume 8 */
> >>
> >> In your case we must already be in the 'else /* Assume 8 */' because otherwise
> >> we've be writing a 64-bit constant into a < 64-bit structure member.
> >>
> >> I think we should put your code into the else clause.
> >>
> >> 272      else /* 8 */                                                             \
> >> 273        {                                                                      \
> >>
> >>              if (__builtin_constant_p (value)
> >>                  && ([the value is a >32-bit constant])
> >>                asm volatile ([use movq reg64, mem64]);
> >>              else
> >>
> >> 274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
> >> 275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> >> 276                          "i" (offsetof (struct pthread, member)));            \
> >> 277        }})
> >>
> >> (2) What if gcc can't prove it's constant?
> >>
> >> If the constant is >32-bit, but gcc can't prove it's constant, then don't we
> >> try to put a >32-bit constant into a 32-bit immediate?
> >>
> >> Shouldn't the code be structured the other way around?
> >>
> >> e.g.
> >>
> >> else /* 8 */
> >> {
> >>   if (__builtin_constant_p (value)
> >>       && ([the value is a <= 32-bit constant])
> >>     asm volatile ([use movq imm32, mem64]);
> >>   else
> >>     asm volatile ([use movq reg64, mem64]);
> >> }
> >>
> >> This way the code is always correct?
> >
> > I changed it to
> >
> >          if (!__builtin_constant_p (value)                                    \
> >              || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> >            asm volatile ("movq %q0,%%fs:%P1" :                                \
> >                          : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> >                            "i" (offsetof (struct pthread, member)));          \
> >          else                                                                 \
> >            asm volatile ("movq %0,%%fs:%P1" :                                 \
> >                          : "r" (value),                                       \
> >                            "i" (offsetof (struct pthread, member)));          \
> >
> >>>         {                                                                   \
> >>>        asm volatile ("movq %q0,%%fs:%P1" :                                  \
> >>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >>>                    : IMM_MODE (value),                                      \
> >>>                      "i" (offsetof (struct pthread, member[0])),            \
> >>>                      "r" (idx));                                            \
> >>> +     else if (__builtin_constant_p (value)                                 \
> >>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> >>> +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :                          \
> >>> +                  : "r" (value),                                           \
> >>> +                    "i" (offsetof (struct pthread, member[0])),            \
> >>> +                    "r" (idx));                                            \
> >>>       else /* 8 */                                                          \
> >>>         {                                                                   \
> >>>        asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :                          \
> >>>
> >>
> >
> > Here is the v2 patch.  OK for master?
> >
> > Thanks.
> >
>
> > From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Fri, 8 Jan 2021 15:38:14 -0800
> > Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> >
> > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> > "movq reg64, mem64" to store 64-bit constant.
> > ---
> >  sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> > index 20f0958780..0dbd66209c 100644
> > --- a/sysdeps/x86_64/nptl/tls.h
> > +++ b/sysdeps/x86_64/nptl/tls.h
> > @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >                      "i" (offsetof (struct pthread, member)));              \
> >       else /* 8 */                                                          \
> >         {                                                                   \
> > -      asm volatile ("movq %q0,%%fs:%P1" :                                  \
> > -                    : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> > -                      "i" (offsetof (struct pthread, member)));            \
> > +      if (!__builtin_constant_p (value)                                    \
>
> Whis is this a "!__builtin_constant_p?"
>
> I would have expected:
>
> if ([the value is a constant a therefore known quantity]
>     || [it is a 32-bit value])

IMM_MODE constraint is valid only if VALUE isn't a constant or
is a signed 32-bit constant.

> Perhaps a quick single line comment here explaining the conditionals would help?

Done.

Here is the v3 patch with comments.   OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: v3-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --]
[-- Type: text/x-patch, Size: 2675 bytes --]

From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 8 Jan 2021 15:38:14 -0800
Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
"movq reg64, mem64" to store 64-bit constant.
---
 sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..acea951d49 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member)));	      \
+	 /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant   \
+	    or is a signed 32-bit constant.  */ 			      \
+	 if (!__builtin_constant_p (value)				      \
+	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1" :				      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member)));	      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1" :				      \
+			 : "r" (value),					      \
+			   "i" (offsetof (struct pthread, member)));	      \
        }})
 
 
@@ -296,10 +304,19 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member[0])),	      \
-			 "r" (idx));					      \
+	 /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant   \
+	    or is a signed 32-bit constant.  */ 			      \
+	 if (!__builtin_constant_p (value)				      \
+	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
+			 : "r" (value),					      \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
        }})
 
 
-- 
2.29.2

Re: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Carlos O'Donell]

On 3/8/21 7:09 PM, H.J. Lu wrote:
> On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>
>> On 3/2/21 9:21 AM, H.J. Lu wrote:
>>> On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote:
>>>>
>>>> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
>>>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
>>>>> "movq reg64, mem64" to store 64-bit constant in TCB.
>>>>> ---
>>>>>  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
>>>>>  1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
>>>>> index 20f0958780..9ec8703e45 100644
>>>>> --- a/sysdeps/x86_64/nptl/tls.h
>>>>> +++ b/sysdeps/x86_64/nptl/tls.h
>>>>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>>>>>         asm volatile ("movl %0,%%fs:%P1" :                                  \
>>>>>                    : IMM_MODE (value),                                      \
>>>>>                      "i" (offsetof (struct pthread, member)));              \
>>>>> +     else if (__builtin_constant_p (value)                                 \
>>>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
>>>>> +       asm volatile ("movq %0,%%fs:%P1" :                                  \
>>>>> +                  : "r" (value),                                           \
>>>>> +                    "i" (offsetof (struct pthread, member)));              \
>>>>
>>>> (1) Move conditional into 'else /* 8 */' section.
>>>>
>>>> The blocks of conditionals are predicated on the size of the member we are
>>>> about to write to.
>>>>
>>>> In the block below...
>>>>
>>>>>       else /* 8 */                                                          \
>>>>
>>>> ... here, we are about to write value into a member that is size 8.
>>>>
>>>> Your code changes the logical construction of the code, but in way that makes
>>>> it more difficult to understand.
>>>>
>>>> We previously had:
>>>>
>>>> if (sizeof() == 1)
>>>>
>>>> else if (sizeof() == 4)
>>>>
>>>> else /* Assume 8 */
>>>>
>>>> In your case we must already be in the 'else /* Assume 8 */' because otherwise
>>>> we've be writing a 64-bit constant into a < 64-bit structure member.
>>>>
>>>> I think we should put your code into the else clause.
>>>>
>>>> 272      else /* 8 */                                                             \
>>>> 273        {                                                                      \
>>>>
>>>>              if (__builtin_constant_p (value)
>>>>                  && ([the value is a >32-bit constant])
>>>>                asm volatile ([use movq reg64, mem64]);
>>>>              else
>>>>
>>>> 274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
>>>> 275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
>>>> 276                          "i" (offsetof (struct pthread, member)));            \
>>>> 277        }})
>>>>
>>>> (2) What if gcc can't prove it's constant?
>>>>
>>>> If the constant is >32-bit, but gcc can't prove it's constant, then don't we
>>>> try to put a >32-bit constant into a 32-bit immediate?
>>>>
>>>> Shouldn't the code be structured the other way around?
>>>>
>>>> e.g.
>>>>
>>>> else /* 8 */
>>>> {
>>>>   if (__builtin_constant_p (value)
>>>>       && ([the value is a <= 32-bit constant])
>>>>     asm volatile ([use movq imm32, mem64]);
>>>>   else
>>>>     asm volatile ([use movq reg64, mem64]);
>>>> }
>>>>
>>>> This way the code is always correct?
>>>
>>> I changed it to
>>>
>>>          if (!__builtin_constant_p (value)                                    \
>>>              || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
>>>            asm volatile ("movq %q0,%%fs:%P1" :                                \
>>>                          : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
>>>                            "i" (offsetof (struct pthread, member)));          \
>>>          else                                                                 \
>>>            asm volatile ("movq %0,%%fs:%P1" :                                 \
>>>                          : "r" (value),                                       \
>>>                            "i" (offsetof (struct pthread, member)));          \
>>>
>>>>>         {                                                                   \
>>>>>        asm volatile ("movq %q0,%%fs:%P1" :                                  \
>>>>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>>>>>                    : IMM_MODE (value),                                      \
>>>>>                      "i" (offsetof (struct pthread, member[0])),            \
>>>>>                      "r" (idx));                                            \
>>>>> +     else if (__builtin_constant_p (value)                                 \
>>>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
>>>>> +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :                          \
>>>>> +                  : "r" (value),                                           \
>>>>> +                    "i" (offsetof (struct pthread, member[0])),            \
>>>>> +                    "r" (idx));                                            \
>>>>>       else /* 8 */                                                          \
>>>>>         {                                                                   \
>>>>>        asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :                          \
>>>>>
>>>>
>>>
>>> Here is the v2 patch.  OK for master?
>>>
>>> Thanks.
>>>
>>
>>> From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001
>>> From: "H.J. Lu" <hjl.tools@gmail.com>
>>> Date: Fri, 8 Jan 2021 15:38:14 -0800
>>> Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
>>>
>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
>>> "movq reg64, mem64" to store 64-bit constant.
>>> ---
>>>  sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++-------
>>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
>>> index 20f0958780..0dbd66209c 100644
>>> --- a/sysdeps/x86_64/nptl/tls.h
>>> +++ b/sysdeps/x86_64/nptl/tls.h
>>> @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>>>                      "i" (offsetof (struct pthread, member)));              \
>>>       else /* 8 */                                                          \
>>>         {                                                                   \
>>> -      asm volatile ("movq %q0,%%fs:%P1" :                                  \
>>> -                    : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
>>> -                      "i" (offsetof (struct pthread, member)));            \
>>> +      if (!__builtin_constant_p (value)                                    \
>>
>> Whis is this a "!__builtin_constant_p?"
>>
>> I would have expected:
>>
>> if ([the value is a constant a therefore known quantity]
>>     || [it is a 32-bit value])
> 
> IMM_MODE constraint is valid only if VALUE isn't a constant or
> is a signed 32-bit constant.
> 
>> Perhaps a quick single line comment here explaining the conditionals would help?
> 
> Done.
> 
> Here is the v3 patch with comments.   OK for master?

No, this still doesn't make logical sense to me.

See my comments below.

> From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 8 Jan 2021 15:38:14 -0800
> Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> 
> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> "movq reg64, mem64" to store 64-bit constant.
> ---
>  sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..acea951d49 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "i" (offsetof (struct pthread, member)));	      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member)));	      \
> +	 /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant   \
> +	    or is a signed 32-bit constant.  */ 			      \

We are trying to gate this asm volatile so it is only used by the compiler
when the immediate is known to be 32-bit wide.

The old code was using IMM_MODE, which resolves to "nr" which should as a
constraint attempt to pass in a compile-time constant 32-bit value. My
understanding is that this constraint could fail to be met and the we get
a compile-time failure?

The value of !__builtin_constant_p only tells you that the value is not a
constant. What prevents that value from being a 64-bit register with a 64-bit
value which can't be encoded via 'nr' with movq?

Why isn't the right solution?

if (__builtin_constant_p (value)
    && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value))
  /* The value is constant and known to be <= 32-bit immediate value.
     In this case we allow the compiler to optimize and use movq with
     an immediate value.  */
  ...
else
  /* The value is not known and so we must assume it is at worst a 64-bit value.
     We can't optimize this and so we must use a register for the move. */
  ...

> +	 if (!__builtin_constant_p (value)				      \
> +	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %q0,%%fs:%P1" :				      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member)));	      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1" :				      \
> +			 : "r" (value),					      \
> +			   "i" (offsetof (struct pthread, member)));	      \
>         }})
>  
>  
> @@ -296,10 +304,19 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "r" (idx));					      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member[0])),	      \
> -			 "r" (idx));					      \
> +	 /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant   \
> +	    or is a signed 32-bit constant.  */ 			      \
> +	 if (!__builtin_constant_p (value)				      \
> +	     || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
> +			 : "r" (value),					      \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
>         }})
>  
>  
> -- 
> 2.29.2
> 


-- 
Cheers,
Carlos.

[PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 10538 bytes --]

On Mon, Mar 15, 2021 at 5:49 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 3/8/21 7:09 PM, H.J. Lu wrote:
> > On Mon, Mar 8, 2021 at 2:28 PM Carlos O'Donell <carlos@redhat.com> wrote:
> >>
> >> On 3/2/21 9:21 AM, H.J. Lu wrote:
> >>> On Mon, Mar 1, 2021 at 5:30 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >>>>
> >>>> On 2/2/21 2:12 PM, H.J. Lu via Libc-alpha wrote:
> >>>>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> >>>>> "movq reg64, mem64" to store 64-bit constant in TCB.
> >>>>> ---
> >>>>>  sysdeps/x86_64/nptl/tls.h | 11 +++++++++++
> >>>>>  1 file changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> >>>>> index 20f0958780..9ec8703e45 100644
> >>>>> --- a/sysdeps/x86_64/nptl/tls.h
> >>>>> +++ b/sysdeps/x86_64/nptl/tls.h
> >>>>> @@ -269,6 +269,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >>>>>         asm volatile ("movl %0,%%fs:%P1" :                                  \
> >>>>>                    : IMM_MODE (value),                                      \
> >>>>>                      "i" (offsetof (struct pthread, member)));              \
> >>>>> +     else if (__builtin_constant_p (value)                                 \
> >>>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> >>>>> +       asm volatile ("movq %0,%%fs:%P1" :                                  \
> >>>>> +                  : "r" (value),                                           \
> >>>>> +                    "i" (offsetof (struct pthread, member)));              \
> >>>>
> >>>> (1) Move conditional into 'else /* 8 */' section.
> >>>>
> >>>> The blocks of conditionals are predicated on the size of the member we are
> >>>> about to write to.
> >>>>
> >>>> In the block below...
> >>>>
> >>>>>       else /* 8 */                                                          \
> >>>>
> >>>> ... here, we are about to write value into a member that is size 8.
> >>>>
> >>>> Your code changes the logical construction of the code, but in way that makes
> >>>> it more difficult to understand.
> >>>>
> >>>> We previously had:
> >>>>
> >>>> if (sizeof() == 1)
> >>>>
> >>>> else if (sizeof() == 4)
> >>>>
> >>>> else /* Assume 8 */
> >>>>
> >>>> In your case we must already be in the 'else /* Assume 8 */' because otherwise
> >>>> we've be writing a 64-bit constant into a < 64-bit structure member.
> >>>>
> >>>> I think we should put your code into the else clause.
> >>>>
> >>>> 272      else /* 8 */                                                             \
> >>>> 273        {                                                                      \
> >>>>
> >>>>              if (__builtin_constant_p (value)
> >>>>                  && ([the value is a >32-bit constant])
> >>>>                asm volatile ([use movq reg64, mem64]);
> >>>>              else
> >>>>
> >>>> 274          asm volatile ("movq %q0,%%fs:%P1" :                                  \
> >>>> 275                        : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> >>>> 276                          "i" (offsetof (struct pthread, member)));            \
> >>>> 277        }})
> >>>>
> >>>> (2) What if gcc can't prove it's constant?
> >>>>
> >>>> If the constant is >32-bit, but gcc can't prove it's constant, then don't we
> >>>> try to put a >32-bit constant into a 32-bit immediate?
> >>>>
> >>>> Shouldn't the code be structured the other way around?
> >>>>
> >>>> e.g.
> >>>>
> >>>> else /* 8 */
> >>>> {
> >>>>   if (__builtin_constant_p (value)
> >>>>       && ([the value is a <= 32-bit constant])
> >>>>     asm volatile ([use movq imm32, mem64]);
> >>>>   else
> >>>>     asm volatile ([use movq reg64, mem64]);
> >>>> }
> >>>>
> >>>> This way the code is always correct?
> >>>
> >>> I changed it to
> >>>
> >>>          if (!__builtin_constant_p (value)                                    \
> >>>              || (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> >>>            asm volatile ("movq %q0,%%fs:%P1" :                                \
> >>>                          : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> >>>                            "i" (offsetof (struct pthread, member)));          \
> >>>          else                                                                 \
> >>>            asm volatile ("movq %0,%%fs:%P1" :                                 \
> >>>                          : "r" (value),                                       \
> >>>                            "i" (offsetof (struct pthread, member)));          \
> >>>
> >>>>>         {                                                                   \
> >>>>>        asm volatile ("movq %q0,%%fs:%P1" :                                  \
> >>>>> @@ -294,6 +299,12 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >>>>>                    : IMM_MODE (value),                                      \
> >>>>>                      "i" (offsetof (struct pthread, member[0])),            \
> >>>>>                      "r" (idx));                                            \
> >>>>> +     else if (__builtin_constant_p (value)                                 \
> >>>>> +           && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value)  \
> >>>>> +       asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :                          \
> >>>>> +                  : "r" (value),                                           \
> >>>>> +                    "i" (offsetof (struct pthread, member[0])),            \
> >>>>> +                    "r" (idx));                                            \
> >>>>>       else /* 8 */                                                          \
> >>>>>         {                                                                   \
> >>>>>        asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :                          \
> >>>>>
> >>>>
> >>>
> >>> Here is the v2 patch.  OK for master?
> >>>
> >>> Thanks.
> >>>
> >>
> >>> From 4fbc9ab67933d662506a01658d4c81a922e50fdf Mon Sep 17 00:00:00 2001
> >>> From: "H.J. Lu" <hjl.tools@gmail.com>
> >>> Date: Fri, 8 Jan 2021 15:38:14 -0800
> >>> Subject: [PATCH v2] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> >>>
> >>> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> >>> "movq reg64, mem64" to store 64-bit constant.
> >>> ---
> >>>  sysdeps/x86_64/nptl/tls.h | 27 ++++++++++++++++++++-------
> >>>  1 file changed, 20 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> >>> index 20f0958780..0dbd66209c 100644
> >>> --- a/sysdeps/x86_64/nptl/tls.h
> >>> +++ b/sysdeps/x86_64/nptl/tls.h
> >>> @@ -271,9 +271,15 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >>>                      "i" (offsetof (struct pthread, member)));              \
> >>>       else /* 8 */                                                          \
> >>>         {                                                                   \
> >>> -      asm volatile ("movq %q0,%%fs:%P1" :                                  \
> >>> -                    : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> >>> -                      "i" (offsetof (struct pthread, member)));            \
> >>> +      if (!__builtin_constant_p (value)                                    \
> >>
> >> Whis is this a "!__builtin_constant_p?"
> >>
> >> I would have expected:
> >>
> >> if ([the value is a constant a therefore known quantity]
> >>     || [it is a 32-bit value])
> >
> > IMM_MODE constraint is valid only if VALUE isn't a constant or
> > is a signed 32-bit constant.
> >
> >> Perhaps a quick single line comment here explaining the conditionals would help?
> >
> > Done.
> >
> > Here is the v3 patch with comments.   OK for master?
>
> No, this still doesn't make logical sense to me.
>
> See my comments below.
>
> > From 0a5d266206be231ab11f3cfa0e213db1e04cd7bd Mon Sep 17 00:00:00 2001
> > From: "H.J. Lu" <hjl.tools@gmail.com>
> > Date: Fri, 8 Jan 2021 15:38:14 -0800
> > Subject: [PATCH v3] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> >
> > Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> > "movq reg64, mem64" to store 64-bit constant.
> > ---
> >  sysdeps/x86_64/nptl/tls.h | 31 ++++++++++++++++++++++++-------
> >  1 file changed, 24 insertions(+), 7 deletions(-)
> >
> > diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> > index 20f0958780..acea951d49 100644
> > --- a/sysdeps/x86_64/nptl/tls.h
> > +++ b/sysdeps/x86_64/nptl/tls.h
> > @@ -271,9 +271,17 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
> >                      "i" (offsetof (struct pthread, member)));              \
> >       else /* 8 */                                                          \
> >         {                                                                   \
> > -      asm volatile ("movq %q0,%%fs:%P1" :                                  \
> > -                    : IMM_MODE ((uint64_t) cast_to_integer (value)),       \
> > -                      "i" (offsetof (struct pthread, member)));            \
> > +      /* NB: IMM_MODE constraint is valid only if VALUE isn't a constant   \
> > +         or is a signed 32-bit constant.  */                               \
>
> We are trying to gate this asm volatile so it is only used by the compiler
> when the immediate is known to be 32-bit wide.
>
> The old code was using IMM_MODE, which resolves to "nr" which should as a
> constraint attempt to pass in a compile-time constant 32-bit value. My
> understanding is that this constraint could fail to be met and the we get
> a compile-time failure?
>
> The value of !__builtin_constant_p only tells you that the value is not a
> constant. What prevents that value from being a 64-bit register with a 64-bit
> value which can't be encoded via 'nr' with movq?
>
> Why isn't the right solution?
>
> if (__builtin_constant_p (value)
>     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value))
>   /* The value is constant and known to be <= 32-bit immediate value.
>      In this case we allow the compiler to optimize and use movq with
>      an immediate value.  */
>   ...
> else
>   /* The value is not known and so we must assume it is at worst a 64-bit value.
>      We can't optimize this and so we must use a register for the move. */
>   ...
>

Fixed.

Here is the v4 patch.   OK for master?

Thanks.

-- 
H.J.

[-- Attachment #2: v4-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --]
[-- Type: text/x-patch, Size: 2851 bytes --]

From d3854da28e1cfc2544226629b3b85ad6e2d6f4d4 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 8 Jan 2021 15:38:14 -0800
Subject: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
"movq reg64, mem64" to store 64-bit constant.
---
 sysdeps/x86_64/nptl/tls.h | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..2a4739df64 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member)));	      \
+	 /* The value is constant and known to be <= 32-bit immediate value.  \
+	    In this case we allow the compiler to optimize and use movq with  \
+	    an immediate value.  */					      \
+	 if (__builtin_constant_p (value)				      \
+	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1" :				      \
+			 : "i" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member)));	      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1" :				      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member)));	      \
        }})
 
 
@@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member[0])),	      \
-			 "r" (idx));					      \
+	 /* The value is constant and known to be <= 32-bit immediate value.  \
+	    In this case we allow the compiler to optimize and use movq with  \
+	    an immediate value.  */					      \
+	 if (__builtin_constant_p (value)				      \
+	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
+			 : "i" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
+	 else								      \
+	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
+			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
        }})
 
 
-- 
2.30.2

Re: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Carlos O'Donell]

On 3/15/21 9:29 AM, H.J. Lu wrote:
> Here is the v4 patch.   OK for master?

Let me start from the beginning, just to clarify a few things for my review.

The original sequence was this:

  asm volatile ("movq %0,%%fs:%P1" :
		: IMM_MODE ((uint64_t) cast_to_integer (value)),
		  "i" (offsetof (struct pthread, member)));

With IMM_MODE being 'nr'/'ir' and allows:

i/n - immediate integer operand

r - register operand

You attempt to write a 64-bit constant to the TCB and the above code fails.

Does gcc attempt to encode a 64-bit immediate into %0 and the assembler fails?

What was the error?

Then we try to fix this with the new sequence:

  asm volatile ("movq %q0,%%fs:%P1" :
		: "i" ((uint64_t) cast_to_integer (value)),
		  "i" (offsetof (struct pthread, member)));

Note that "%q0" is a machine constraint for any register.

This enforces only "i" (immediate integer operand) but requires the compiler
pick a register with the machine constraint for "%q0", so you can have 64-bit
constants but the compiler must arrange (via more machine instructions
to get the constant in into a 64-bit register).

Your v4 follows:

> From d3854da28e1cfc2544226629b3b85ad6e2d6f4d4 Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Fri, 8 Jan 2021 15:38:14 -0800
> Subject: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
> 
> Since there is only "movq imm32s, mem64" and no "movq imm64, mem64", use
> "movq reg64, mem64" to store 64-bit constant> ---
>  sysdeps/x86_64/nptl/tls.h | 33 ++++++++++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 7 deletions(-)
> 
> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..2a4739df64 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "i" (offsetof (struct pthread, member)));	      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member)));	      \
> +	 /* The value is constant and known to be <= 32-bit immediate value.  \
> +	    In this case we allow the compiler to optimize and use movq with  \
> +	    an immediate value.  */					      \
> +	 if (__builtin_constant_p (value)				      \
> +	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \

Note that I made a typo on my last review. My intent was to write this:

/* The value is constant and known to be <= 32-bit immediate value.  \
   In this case we allow the compiler to optimize and use movq with  \
   an immediate value.  */					      \
if (__builtin_constant_p (value)
    && (int64_t) (int32_t) (uintptr_t) value != (uintptr_t) value))
	   asm volatile ("movq %0,%%fs:%P1" :				      \
			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
			   "i" (offsetof (struct pthread, member)));	      \
else
/* The value is not known and so we must assume it is at worst a 64-bit value.
   We can't optimize this and so we must use a register for the move. */
	   asm volatile ("movq %q0,%%fs:%P1" :				      \
			 : "i" ((uint64_t) cast_to_integer (value)),	      \
			   "i" (offsetof (struct pthread, member)));	      \

So in the case you write a 64-bit constant you get the else case.

But in the case you have normal known 32-bit values you get the movq with a imm32s.

Does that work?

Can you please verify that you get the first case for all the normal 32-bit constants?

Can you please verify that you get the second for the 64-bit constant write?


> +	   asm volatile ("movq %q0,%%fs:%P1" :				      \
> +			 : "i" ((uint64_t) cast_to_integer (value)),	      \
> +			   "i" (offsetof (struct pthread, member)));	      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1" :				      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member)));	      \
>         }})
>  
>  
> @@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "r" (idx));					      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member[0])),	      \
> -			 "r" (idx));					      \
> +	 /* The value is constant and known to be <= 32-bit immediate value.  \
> +	    In this case we allow the compiler to optimize and use movq with  \
> +	    an immediate value.  */					      \
> +	 if (__builtin_constant_p (value)				      \
> +	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> +			 : "i" ((uint64_t) cast_to_integer (value)),	      \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
> +	 else								      \
> +	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
> +			 : IMM_MODE ((uint64_t) cast_to_integer (value)),     \
> +			   "i" (offsetof (struct pthread, member[0])),	      \
> +			   "r" (idx));					      \
>         }})
>  
>  
> -- 
> 2.30.2
> 

-- 
Cheers,
Carlos.

Re: [PATCH v4] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64

[Andreas Schwab]

On Mär 15 2021, Carlos O'Donell via Libc-alpha wrote:

> Then we try to fix this with the new sequence:
>
>   asm volatile ("movq %q0,%%fs:%P1" :
> 		: "i" ((uint64_t) cast_to_integer (value)),
> 		  "i" (offsetof (struct pthread, member)));
>
> Note that "%q0" is a machine constraint for any register.

An operand modifier isn't a constraint.  It does not change what the
compiler matches for the operand, it only modifies the way the operand
is output.  'q' has no meaning for an operand matched by the 'i'
constraint, since that is never a register.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

[PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Tue, Mar 16, 2021 at 2:04 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Mär 15 2021, Carlos O'Donell via Libc-alpha wrote:
>
> > Then we try to fix this with the new sequence:
> >
> >   asm volatile ("movq %q0,%%fs:%P1" :
> >               : "i" ((uint64_t) cast_to_integer (value)),
> >                 "i" (offsetof (struct pthread, member)));
> >
> > Note that "%q0" is a machine constraint for any register.
>
> An operand modifier isn't a constraint.  It does not change what the
> compiler matches for the operand, it only modifies the way the operand
> is output.  'q' has no meaning for an operand matched by the 'i'
> constraint, since that is never a register.

That is correct.

Since movq takes a register or a signed 32-bit immediate source operand,
select the immediate source operand only if it is known to be 32-bit
signed immediate at compile-time.

Here is the v5 patch with a testcase.  OK for master?

Thanks.

--
H.J.

[-- Attachment #2: v5-0001-x86_64-Update-THREAD_SETMEM-THREAD_SETMEM_NC-for-.patch --]
[-- Type: text/x-patch, Size: 5673 bytes --]

From 199f147a2c95549890fcb1c8d4c3598bce45545b Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 16 Mar 2021 07:41:46 -0700
Subject: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64
 [BZ #27591]

Since movq takes a register or a signed 32-bit immediate source operand,
select the immediate source operand only if it is known to be 32-bit
signed immediate at compile-time.
---
 sysdeps/x86_64/Makefile           |  2 ++
 sysdeps/x86_64/nptl/tls.h         | 33 +++++++++++++++-----
 sysdeps/x86_64/tst-x86-64-tls-1.c | 50 +++++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 7 deletions(-)
 create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c

diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index d1d7cb9d2e..06a444b6af 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests))
 tests-container += tst-glibc-hwcaps-cache
 endif
 
+tests-internal += tst-x86-64-tls-1
+
 endif # $(subdir) == elf
 
 ifeq ($(subdir),csu)
diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..b19d90433d 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member)));	      \
+	 /* Since movq takes a register or a signed 32-bit immediate source   \
+	    operand, select the immediate source operand only if it is known  \
+	    to be 32-bit signed immediate at compile-time.  */		      \
+	 if (__builtin_constant_p (value)				      \
+	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %0,%%fs:%P1" :				      \
+			 : "i" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member)));	      \
+	 else								      \
+	   asm volatile ("movq %q0,%%fs:%P1" :				      \
+			 : "r" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member)));	      \
        }})
 
 
@@ -296,10 +305,20 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
-	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
-			 "i" (offsetof (struct pthread, member[0])),	      \
-			 "r" (idx));					      \
+	 /* Since movq takes a register or a signed 32-bit immediate source   \
+	    operand, select the immediate source operand only if it is known  \
+	    to be 32-bit signed immediate at compile-time.  */		      \
+	 if (__builtin_constant_p (value)				      \
+	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
+	   asm volatile ("movq %0,%%fs:%P1(,%q2,8)" :			      \
+			 : "i" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
+	 else								      \
+	   asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
+			 : "r" ((uint64_t) cast_to_integer (value)),	      \
+			   "i" (offsetof (struct pthread, member[0])),	      \
+			   "r" (idx));					      \
        }})
 
 
diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c
new file mode 100644
index 0000000000..ac6b13d3b7
--- /dev/null
+++ b/sysdeps/x86_64/tst-x86-64-tls-1.c
@@ -0,0 +1,50 @@
+/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <tls.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  unsigned long long int saved_ssp_base
+    = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1);
+  unsigned long long int ssp_base
+    = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  if (ssp_base != ((1ULL << 57) - 1))
+    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx",
+		ssp_base, (1ULL << 57) - 1);
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base);
+#ifndef __ILP32__
+  struct pthread_key_data *saved_specific
+    = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  uintptr_t value = (1UL << 57) - 1;
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
+		    (struct pthread_key_data *) value);
+  struct pthread_key_data *specific
+    = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  if (specific != (struct pthread_key_data *) value)
+    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
+		specific, (struct pthread_key_data *) value);
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific);
+#endif
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591]

[Andreas Schwab]

On Mär 16 2021, H.J. Lu wrote:

> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..b19d90433d 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -271,9 +271,18 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "i" (offsetof (struct pthread, member)));	      \
>       else /* 8 */							      \
>         {								      \
> -	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> -			 "i" (offsetof (struct pthread, member)));	      \
> +	 /* Since movq takes a register or a signed 32-bit immediate source   \
> +	    operand, select the immediate source operand only if it is known  \
> +	    to be 32-bit signed immediate at compile-time.  */		      \
> +	 if (__builtin_constant_p (value)				      \
> +	     && (int64_t) (int32_t) (uintptr_t) value == (uintptr_t) value)   \
> +	   asm volatile ("movq %0,%%fs:%P1" :				      \
> +			 : "i" ((uint64_t) cast_to_integer (value)),	      \
> +			   "i" (offsetof (struct pthread, member)));	      \
> +	 else								      \
> +	   asm volatile ("movq %q0,%%fs:%P1" :				      \
> +			 : "r" ((uint64_t) cast_to_integer (value)),	      \
> +			   "i" (offsetof (struct pthread, member)));	      \

I don't think this will work in general.  Wouldn't it be possible that
an operands optimizes to a 64-bit constant in later passes?  What you
really need is a constraint that only matches a 32-bit immediate or a
register.  Like "rWf"?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591]

[Florian Weimer]

* Andreas Schwab:

> I don't think this will work in general.  Wouldn't it be possible that
> an operands optimizes to a 64-bit constant in later passes?  What you
> really need is a constraint that only matches a 32-bit immediate or a
> register.  Like "rWf"?

Maybe we should to the __seg_fs namespace instead?  Wouldn't that avoid
these issues?

Thanks,
Florian

Re: [PATCH v5] x86_64: Update THREAD_SETMEM/THREAD_SETMEM_NC for IMM64 [BZ #27591]

[Carlos O'Donell]

On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote:
> * Andreas Schwab:
> 
>> I don't think this will work in general.  Wouldn't it be possible that
>> an operands optimizes to a 64-bit constant in later passes?  What you
>> really need is a constraint that only matches a 32-bit immediate or a
>> register.  Like "rWf"?
> 
> Maybe we should to the __seg_fs namespace instead?  Wouldn't that avoid
> these issues?

It might.

HJ, Does it work to rewrite this using __seg_fs?

Otherwise I'll review v5, which I think is better than it was before and
makes forward progress.

-- 
Cheers,
Carlos.

[PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591]

[H.J. Lu]

On Tue, Mar 16, 2021 at 9:34 AM Carlos O'Donell <carlos@redhat.com> wrote:
>
> On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote:
> > * Andreas Schwab:
> >
> >> I don't think this will work in general.  Wouldn't it be possible that
> >> an operands optimizes to a 64-bit constant in later passes?  What you
> >> really need is a constraint that only matches a 32-bit immediate or a
> >> register.  Like "rWf"?

config/i386/constraints.md in GCC has

(define_constraint "e"
  "32-bit signed integer constant, or a symbolic reference known
   to fit that range (for immediate operands in sign-extending x86-64
   instructions)."
  (match_operand 0 "x86_64_immediate_operand"))

"er" is the correct constraint for movq.

> > Maybe we should to the __seg_fs namespace instead?  Wouldn't that avoid
> > these issues?
>
> It might.
>
> HJ, Does it work to rewrite this using __seg_fs?

I tried it.  GCC generates worse code.

> Otherwise I'll review v5, which I think is better than it was before and
> makes forward progress.

Here is the v6 patch to use the "er" constraint.   OK for master?

Thanks.


-- 
H.J.

Re: [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1257 bytes --]

On Tue, Mar 16, 2021 at 9:50 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Mar 16, 2021 at 9:34 AM Carlos O'Donell <carlos@redhat.com> wrote:
> >
> > On 3/16/21 12:10 PM, Florian Weimer via Libc-alpha wrote:
> > > * Andreas Schwab:
> > >
> > >> I don't think this will work in general.  Wouldn't it be possible that
> > >> an operands optimizes to a 64-bit constant in later passes?  What you
> > >> really need is a constraint that only matches a 32-bit immediate or a
> > >> register.  Like "rWf"?
>
> config/i386/constraints.md in GCC has
>
> (define_constraint "e"
>   "32-bit signed integer constant, or a symbolic reference known
>    to fit that range (for immediate operands in sign-extending x86-64
>    instructions)."
>   (match_operand 0 "x86_64_immediate_operand"))
>
> "er" is the correct constraint for movq.
>
> > > Maybe we should to the __seg_fs namespace instead?  Wouldn't that avoid
> > > these issues?
> >
> > It might.
> >
> > HJ, Does it work to rewrite this using __seg_fs?
>
> I tried it.  GCC generates worse code.
>
> > Otherwise I'll review v5, which I think is better than it was before and
> > makes forward progress.
>
> Here is the v6 patch to use the "er" constraint.   OK for master?
>

Include the patch.

-- 
H.J.

[-- Attachment #2: v6-0001-x86_64-Correct-THREAD_SETMEM-THREAD_SETMEM_NC-for.patch --]
[-- Type: text/x-patch, Size: 5458 bytes --]

From 98ff24c0c7fba29c90ac37edd0057515aacdafa0 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 16 Mar 2021 07:41:46 -0700
Subject: [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq
 [BZ #27591]

config/i386/constraints.md in GCC has

(define_constraint "e"
  "32-bit signed integer constant, or a symbolic reference known
   to fit that range (for immediate operands in sign-extending x86-64
   instructions)."
  (match_operand 0 "x86_64_immediate_operand"))

Since movq takes a signed 32-bit immediate or a register source operand,
use "er", instead of "nr"/"ir", constraint for 32-bit signed integer
constant or register on movq.
---
 sysdeps/x86_64/Makefile           |  2 +
 sysdeps/x86_64/nptl/tls.h         | 10 ++++-
 sysdeps/x86_64/tst-x86-64-tls-1.c | 64 +++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c

diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index d1d7cb9d2e..06a444b6af 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests))
 tests-container += tst-glibc-hwcaps-cache
 endif
 
+tests-internal += tst-x86-64-tls-1
+
 endif # $(subdir) == elf
 
 ifeq ($(subdir),csu)
diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..a78c4f4d01 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,8 +271,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
+	 /* Since movq takes a signed 32-bit immediate or a register source   \
+	    operand, use "er" constraint for 32-bit signed integer constant   \
+	    or register.  */						      \
 	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
+		       : "er" ((uint64_t) cast_to_integer (value)),	      \
 			 "i" (offsetof (struct pthread, member)));	      \
        }})
 
@@ -296,8 +299,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
+	 /* Since movq takes a signed 32-bit immediate or a register source   \
+	    operand, use "er" constraint for 32-bit signed integer constant   \
+	    or register.  */						      \
 	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
+		       : "er" ((uint64_t) cast_to_integer (value)),	      \
 			 "i" (offsetof (struct pthread, member[0])),	      \
 			 "r" (idx));					      \
        }})
diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c
new file mode 100644
index 0000000000..354635884e
--- /dev/null
+++ b/sysdeps/x86_64/tst-x86-64-tls-1.c
@@ -0,0 +1,64 @@
+/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <tls.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  unsigned long long int saved_ssp_base, ssp_base;
+  saved_ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1);
+  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  if (ssp_base != ((1ULL << 57) - 1))
+    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx",
+		ssp_base, (1ULL << 57) - 1);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, -1ULL);
+  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  if (ssp_base != -1ULL)
+    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", ssp_base, -1ULL);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base);
+#ifndef __ILP32__
+  struct pthread_key_data *saved_specific, *specific;
+  saved_specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+
+  uintptr_t value = (1UL << 57) - 1;
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
+		    (struct pthread_key_data *) value);
+  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  if (specific != (struct pthread_key_data *) value)
+    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
+		specific, (struct pthread_key_data *) value);
+
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
+		    (struct pthread_key_data *) -1UL);
+  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  if (specific != (struct pthread_key_data *) -1UL)
+    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
+		specific, (struct pthread_key_data *) -1UL);
+
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific);
+#endif
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

[PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591]

[H.J. Lu]

config/i386/constraints.md in GCC has

(define_constraint "e"
  "32-bit signed integer constant, or a symbolic reference known
   to fit that range (for immediate operands in sign-extending x86-64
   instructions)."
  (match_operand 0 "x86_64_immediate_operand"))

Since movq takes a signed 32-bit immediate or a register source operand,
use "er", instead of "nr"/"ir", constraint for 32-bit signed integer
constant or register on movq.
---
 sysdeps/x86_64/Makefile           |  2 +
 sysdeps/x86_64/nptl/tls.h         | 10 ++++-
 sysdeps/x86_64/tst-x86-64-tls-1.c | 64 +++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c

diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index d1d7cb9d2e..06a444b6af 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests))
 tests-container += tst-glibc-hwcaps-cache
 endif
 
+tests-internal += tst-x86-64-tls-1
+
 endif # $(subdir) == elf
 
 ifeq ($(subdir),csu)
diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
index 20f0958780..a78c4f4d01 100644
--- a/sysdeps/x86_64/nptl/tls.h
+++ b/sysdeps/x86_64/nptl/tls.h
@@ -271,8 +271,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "i" (offsetof (struct pthread, member)));	      \
      else /* 8 */							      \
        {								      \
+	 /* Since movq takes a signed 32-bit immediate or a register source   \
+	    operand, use "er" constraint for 32-bit signed integer constant   \
+	    or register.  */						      \
 	 asm volatile ("movq %q0,%%fs:%P1" :				      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
+		       : "er" ((uint64_t) cast_to_integer (value)),	      \
 			 "i" (offsetof (struct pthread, member)));	      \
        }})
 
@@ -296,8 +299,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
 		       "r" (idx));					      \
      else /* 8 */							      \
        {								      \
+	 /* Since movq takes a signed 32-bit immediate or a register source   \
+	    operand, use "er" constraint for 32-bit signed integer constant   \
+	    or register.  */						      \
 	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
-		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
+		       : "er" ((uint64_t) cast_to_integer (value)),	      \
 			 "i" (offsetof (struct pthread, member[0])),	      \
 			 "r" (idx));					      \
        }})
diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c
new file mode 100644
index 0000000000..354635884e
--- /dev/null
+++ b/sysdeps/x86_64/tst-x86-64-tls-1.c
@@ -0,0 +1,64 @@
+/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64.
+   Copyright (C) 2021 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <tls.h>
+#include <support/check.h>
+
+static int
+do_test (void)
+{
+  unsigned long long int saved_ssp_base, ssp_base;
+  saved_ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1);
+  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  if (ssp_base != ((1ULL << 57) - 1))
+    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx",
+		ssp_base, (1ULL << 57) - 1);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, -1ULL);
+  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
+  if (ssp_base != -1ULL)
+    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", ssp_base, -1ULL);
+
+  THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base);
+#ifndef __ILP32__
+  struct pthread_key_data *saved_specific, *specific;
+  saved_specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+
+  uintptr_t value = (1UL << 57) - 1;
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
+		    (struct pthread_key_data *) value);
+  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  if (specific != (struct pthread_key_data *) value)
+    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
+		specific, (struct pthread_key_data *) value);
+
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
+		    (struct pthread_key_data *) -1UL);
+  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
+  if (specific != (struct pthread_key_data *) -1UL)
+    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
+		specific, (struct pthread_key_data *) -1UL);
+
+  THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific);
+#endif
+  return 0;
+}
+
+#include <support/test-driver.c>
-- 
2.30.2

Re: [PATCH v6] x86_64: Correct THREAD_SETMEM/THREAD_SETMEM_NC for movq [BZ #27591]

[Carlos O'Donell]

On 3/19/21 2:35 PM, H.J. Lu via Libc-alpha wrote:
> config/i386/constraints.md in GCC has
> 
> (define_constraint "e"
>   "32-bit signed integer constant, or a symbolic reference known
>    to fit that range (for immediate operands in sign-extending x86-64
>    instructions)."
>   (match_operand 0 "x86_64_immediate_operand"))
> 
> Since movq takes a signed 32-bit immediate or a register source operand,
> use "er", instead of "nr"/"ir", constraint for 32-bit signed integer
> constant or register on movq.

v6, and this version looks amazing, much better than before and I think
we've arrived a good solution to this problem. Thank you for your efforts
and working through the details.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  sysdeps/x86_64/Makefile           |  2 +
>  sysdeps/x86_64/nptl/tls.h         | 10 ++++-
>  sysdeps/x86_64/tst-x86-64-tls-1.c | 64 +++++++++++++++++++++++++++++++
>  3 files changed, 74 insertions(+), 2 deletions(-)
>  create mode 100644 sysdeps/x86_64/tst-x86-64-tls-1.c
> 
> diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
> index d1d7cb9d2e..06a444b6af 100644
> --- a/sysdeps/x86_64/Makefile
> +++ b/sysdeps/x86_64/Makefile
> @@ -183,6 +183,8 @@ ifeq (no,$(build-hardcoded-path-in-tests))
>  tests-container += tst-glibc-hwcaps-cache
>  endif
>  
> +tests-internal += tst-x86-64-tls-1

OK. Test is internal with use of tls.h header.

> +
>  endif # $(subdir) == elf
>  
>  ifeq ($(subdir),csu)
> diff --git a/sysdeps/x86_64/nptl/tls.h b/sysdeps/x86_64/nptl/tls.h
> index 20f0958780..a78c4f4d01 100644
> --- a/sysdeps/x86_64/nptl/tls.h
> +++ b/sysdeps/x86_64/nptl/tls.h
> @@ -271,8 +271,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "i" (offsetof (struct pthread, member)));	      \
>       else /* 8 */							      \
>         {								      \
> +	 /* Since movq takes a signed 32-bit immediate or a register source   \
> +	    operand, use "er" constraint for 32-bit signed integer constant   \
> +	    or register.  */						      \
>  	 asm volatile ("movq %q0,%%fs:%P1" :				      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> +		       : "er" ((uint64_t) cast_to_integer (value)),	      \

OK. Great change, very clear, and uses immediate or register to get it to work.

>  			 "i" (offsetof (struct pthread, member)));	      \
>         }})
>  
> @@ -296,8 +299,11 @@ _Static_assert (offsetof (tcbhead_t, __glibc_unused2) == 0x80,
>  		       "r" (idx));					      \
>       else /* 8 */							      \
>         {								      \
> +	 /* Since movq takes a signed 32-bit immediate or a register source   \
> +	    operand, use "er" constraint for 32-bit signed integer constant   \
> +	    or register.  */						      \
>  	 asm volatile ("movq %q0,%%fs:%P1(,%q2,8)" :			      \
> -		       : IMM_MODE ((uint64_t) cast_to_integer (value)),	      \
> +		       : "er" ((uint64_t) cast_to_integer (value)),	      \
>  			 "i" (offsetof (struct pthread, member[0])),	      \

OK.

>  			 "r" (idx));					      \
>         }})
> diff --git a/sysdeps/x86_64/tst-x86-64-tls-1.c b/sysdeps/x86_64/tst-x86-64-tls-1.c
> new file mode 100644
> index 0000000000..354635884e
> --- /dev/null
> +++ b/sysdeps/x86_64/tst-x86-64-tls-1.c
> @@ -0,0 +1,64 @@
> +/* Test THREAD_SETMEM and THREAD_SETMEM_NC for IMM64.
> +   Copyright (C) 2021 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <https://www.gnu.org/licenses/>.  */
> +
> +#include <tls.h>
> +#include <support/check.h>
> +
> +static int
> +do_test (void)
> +{
> +  unsigned long long int saved_ssp_base, ssp_base;
> +  saved_ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
> +
> +  THREAD_SETMEM (THREAD_SELF, header.ssp_base, (1ULL << 57) - 1);
> +  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
> +  if (ssp_base != ((1ULL << 57) - 1))
> +    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx",
> +		ssp_base, (1ULL << 57) - 1);
> +
> +  THREAD_SETMEM (THREAD_SELF, header.ssp_base, -1ULL);
> +  ssp_base = THREAD_GETMEM (THREAD_SELF, header.ssp_base);
> +  if (ssp_base != -1ULL)
> +    FAIL_EXIT1 ("THREAD_SETMEM: 0x%llx != 0x%llx", ssp_base, -1ULL);
> +
> +  THREAD_SETMEM (THREAD_SELF, header.ssp_base, saved_ssp_base);
> +#ifndef __ILP32__
> +  struct pthread_key_data *saved_specific, *specific;
> +  saved_specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
> +
> +  uintptr_t value = (1UL << 57) - 1;
> +  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
> +		    (struct pthread_key_data *) value);
> +  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
> +  if (specific != (struct pthread_key_data *) value)
> +    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
> +		specific, (struct pthread_key_data *) value);
> +
> +  THREAD_SETMEM_NC (THREAD_SELF, specific, 1,
> +		    (struct pthread_key_data *) -1UL);
> +  specific = THREAD_GETMEM_NC (THREAD_SELF, specific, 1);
> +  if (specific != (struct pthread_key_data *) -1UL)
> +    FAIL_EXIT1 ("THREAD_GETMEM_NC: %p != %p",
> +		specific, (struct pthread_key_data *) -1UL);
> +
> +  THREAD_SETMEM_NC (THREAD_SELF, specific, 1, saved_specific);
> +#endif
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>

OK.

> 


-- 
Cheers,
Carlos.