PR libgcc/60790: Avoid IFUNC resolver access to uninitialized data

This patch performs lazy initialization of the relevant CPUID feature 
register value.  It will needlessly invoke the CPUID determination code 
on architectures which lack CPUID support or support for the feature 
register, but I think it's not worth to avoid the complexity for that.

I verified manually that the CMPXCHG16B implementation is still selected 
for a 128-bit load after the change.  I don't know how to write an 
automated test for that.

Thanks,
Florian

On 03/29/2018 08:00 AM, Florian Weimer wrote:
> This patch performs lazy initialization of the relevant CPUID feature
> register value.  It will needlessly invoke the CPUID determination code
> on architectures which lack CPUID support or support for the feature
> register, but I think it's not worth to avoid the complexity for that.
> 
> I verified manually that the CMPXCHG16B implementation is still selected
> for a 128-bit load after the change.  I don't know how to write an
> automated test for that.
> 
> Thanks,
> Florian
> 
> pr60790.patch
> 
> 
> Index: libatomic/ChangeLog
> ===================================================================
> --- libatomic/ChangeLog	(revision 258952)
> +++ libatomic/ChangeLog	(working copy)
> @@ -1,3 +1,18 @@
> +2018-03-29  Florian Weimer  <fweimer@tor.usersys.redhat.com>
> +
> +	PR libgcc/60790
> +	x86: Do not assume ELF constructors run before IFUNC resolvers.
> +	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
> +	Remove declarations.
> +	(__libat_feat1, __libat_feat1_init): Declare.
> +	(FEAT1_REGISTER): Define.
> +	(load_feat1): New function.
> +	(IFUNC_COND_1): Adjust.
> +	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
> +	(init_cpuid): Remove definitions.
> +	(__libat_feat1): New variable.
> +	(__libat_feat1_init): New function.
OK.

Do you have write access to the GCC repository?  If not I can commit for
you.

jeff

On 05/23/2018 12:48 AM, Jeff Law wrote:
> OK.

Thanks.  I will let it sit in trunk for a while and then consider 
backporting it to release branches.

> Do you have write access to the GCC repository?  If not I can commit for
> you.

I have write access.  I take this as a hint to contribute more 
regularly. 8-)

Florian

* Jeff Law:

> On 03/29/2018 08:00 AM, Florian Weimer wrote:
>> This patch performs lazy initialization of the relevant CPUID feature
>> register value.  It will needlessly invoke the CPUID determination code
>> on architectures which lack CPUID support or support for the feature
>> register, but I think it's not worth to avoid the complexity for that.
>> 
>> I verified manually that the CMPXCHG16B implementation is still selected
>> for a 128-bit load after the change.  I don't know how to write an
>> automated test for that.
>> 
>> Thanks,
>> Florian
>> 
>> pr60790.patch
>> 
>> 
>> Index: libatomic/ChangeLog
>> ===================================================================
>> --- libatomic/ChangeLog	(revision 258952)
>> +++ libatomic/ChangeLog	(working copy)
>> @@ -1,3 +1,18 @@
>> +2018-03-29  Florian Weimer  <fweimer@tor.usersys.redhat.com>
>> +
>> +	PR libgcc/60790
>> +	x86: Do not assume ELF constructors run before IFUNC resolvers.
>> +	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
>> +	Remove declarations.
>> +	(__libat_feat1, __libat_feat1_init): Declare.
>> +	(FEAT1_REGISTER): Define.
>> +	(load_feat1): New function.
>> +	(IFUNC_COND_1): Adjust.
>> +	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
>> +	(init_cpuid): Remove definitions.
>> +	(__libat_feat1): New variable.
>> +	(__libat_feat1_init): New function.
> OK.

Here is the backport to gcc-8-branch.

libatomic/ChangeLog

	PR libgcc/60790
	x86: Do not assume ELF constructors run before IFUNC resolvers.
	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
	Remove declarations.
	(__libat_feat1, __libat_feat1_init): Declare.
	(FEAT1_REGISTER): Define.
	(load_feat1): New function.
	(IFUNC_COND_1): Adjust.
	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
	(init_cpuid): Remove definitions.
	(__libat_feat1): New variable.
	(__libat_feat1_init): New function.

Index: libatomic/config/x86/host-config.h
===================================================================
--- libatomic/config/x86/host-config.h	(revision 264990)
+++ libatomic/config/x86/host-config.h	(working copy)
@@ -25,13 +25,39 @@
 #if HAVE_IFUNC
 #include <cpuid.h>
 
-extern unsigned int libat_feat1_ecx HIDDEN;
-extern unsigned int libat_feat1_edx HIDDEN;
+#ifdef __x86_64__
+# define FEAT1_REGISTER ecx
+#else
+# define FEAT1_REGISTER edx
+#endif
 
+/* Value of the CPUID feature register FEAT1_REGISTER for the cmpxchg
+   bit for IFUNC_COND1 below.  */
+extern unsigned int __libat_feat1 HIDDEN;
+
+/* Initialize libat_feat1 and return its value.  */
+unsigned int __libat_feat1_init (void) HIDDEN;
+
+/* Return the value of the relevant feature register for the relevant
+   cmpxchg bit, or 0 if there is no CPUID support.  */
+static inline unsigned int
+__attribute__ ((const))
+load_feat1 (void)
+{
+  /* See the store in __libat_feat1_init.  */
+  unsigned int feat1 = __atomic_load_n (&__libat_feat1, __ATOMIC_RELAXED);
+  if (feat1 == 0)
+    /* Assume that initialization has not happened yet.  This may get
+       called repeatedly if the CPU does not have any feature bits at
+       all.  */
+    feat1 = __libat_feat1_init ();
+  return feat1;
+}
+
 #ifdef __x86_64__
-# define IFUNC_COND_1	(libat_feat1_ecx & bit_CMPXCHG16B)
+# define IFUNC_COND_1	(load_feat1 () & bit_CMPXCHG16B)
 #else
-# define IFUNC_COND_1	(libat_feat1_edx & bit_CMPXCHG8B)
+# define IFUNC_COND_1	(load_feat1 () & bit_CMPXCHG8B)
 #endif
 
 #ifdef __x86_64__
Index: libatomic/config/x86/init.c
===================================================================
--- libatomic/config/x86/init.c	(revision 264990)
+++ libatomic/config/x86/init.c	(working copy)
@@ -26,13 +26,17 @@
 
 #if HAVE_IFUNC
 
-unsigned int libat_feat1_ecx, libat_feat1_edx;
+unsigned int __libat_feat1;
 
-static void __attribute__((constructor))
-init_cpuid (void)
+unsigned int
+__libat_feat1_init (void)
 {
-  unsigned int eax, ebx;
-  __get_cpuid (1, &eax, &ebx, &libat_feat1_ecx, &libat_feat1_edx);
+  unsigned int eax, ebx, ecx, edx;
+  FEAT1_REGISTER = 0;
+  __get_cpuid (1, &eax, &ebx, &ecx, &edx);
+  /* See the load in load_feat1.  */
+  __atomic_store_n (&__libat_feat1, FEAT1_REGISTER, __ATOMIC_RELAXED);
+  return FEAT1_REGISTER;
 }
 
 #endif /* HAVE_IFUNC */
Index: .
===================================================================
--- .	(revision 264990)
+++ .	(working copy)

Property changes on: .

* Florian Weimer:

> * Jeff Law:
>
>> On 03/29/2018 08:00 AM, Florian Weimer wrote:
>>> This patch performs lazy initialization of the relevant CPUID feature
>>> register value.  It will needlessly invoke the CPUID determination code
>>> on architectures which lack CPUID support or support for the feature
>>> register, but I think it's not worth to avoid the complexity for that.
>>> 
>>> I verified manually that the CMPXCHG16B implementation is still selected
>>> for a 128-bit load after the change.  I don't know how to write an
>>> automated test for that.
>>> 
>>> Thanks,
>>> Florian
>>> 
>>> pr60790.patch
>>> 
>>> 
>>> Index: libatomic/ChangeLog
>>> ===================================================================
>>> --- libatomic/ChangeLog	(revision 258952)
>>> +++ libatomic/ChangeLog	(working copy)
>>> @@ -1,3 +1,18 @@
>>> +2018-03-29  Florian Weimer  <fweimer@tor.usersys.redhat.com>
>>> +
>>> +	PR libgcc/60790
>>> +	x86: Do not assume ELF constructors run before IFUNC resolvers.
>>> +	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
>>> +	Remove declarations.
>>> +	(__libat_feat1, __libat_feat1_init): Declare.
>>> +	(FEAT1_REGISTER): Define.
>>> +	(load_feat1): New function.
>>> +	(IFUNC_COND_1): Adjust.
>>> +	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
>>> +	(init_cpuid): Remove definitions.
>>> +	(__libat_feat1): New variable.
>>> +	(__libat_feat1_init): New function.
>> OK.
>
> Here is the backport to gcc-8-branch.

Ping?  <https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00524.html>

On 3/19/19 9:00 AM, Florian Weimer wrote:
> * Florian Weimer:
> 
>> * Jeff Law:
>>
>>> On 03/29/2018 08:00 AM, Florian Weimer wrote:
>>>> This patch performs lazy initialization of the relevant CPUID feature
>>>> register value.  It will needlessly invoke the CPUID determination code
>>>> on architectures which lack CPUID support or support for the feature
>>>> register, but I think it's not worth to avoid the complexity for that.
>>>>
>>>> I verified manually that the CMPXCHG16B implementation is still selected
>>>> for a 128-bit load after the change.  I don't know how to write an
>>>> automated test for that.
>>>>
>>>> Thanks,
>>>> Florian
>>>>
>>>> pr60790.patch
>>>>
>>>>
>>>> Index: libatomic/ChangeLog
>>>> ===================================================================
>>>> --- libatomic/ChangeLog	(revision 258952)
>>>> +++ libatomic/ChangeLog	(working copy)
>>>> @@ -1,3 +1,18 @@
>>>> +2018-03-29  Florian Weimer  <fweimer@tor.usersys.redhat.com>
>>>> +
>>>> +	PR libgcc/60790
>>>> +	x86: Do not assume ELF constructors run before IFUNC resolvers.
>>>> +	* config/x86/host-config.h (libat_feat1_ecx, libat_feat1_edx):
>>>> +	Remove declarations.
>>>> +	(__libat_feat1, __libat_feat1_init): Declare.
>>>> +	(FEAT1_REGISTER): Define.
>>>> +	(load_feat1): New function.
>>>> +	(IFUNC_COND_1): Adjust.
>>>> +	* config/x86/init.c (libat_feat1_ecx, libat_feat1_edx)
>>>> +	(init_cpuid): Remove definitions.
>>>> +	(__libat_feat1): New variable.
>>>> +	(__libat_feat1_init): New function.
>>> OK.
>>
>> Here is the backport to gcc-8-branch.
> 
> Ping?  <https://gcc.gnu.org/ml/gcc-patches/2018-10/msg00524.html>
OK for the branch as well.  I didn't know you were waiting on me :-)
jeff