diff mbox series

x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299]

Message ID 20171017154436.GA29035@gmail.com
State New
Headers show
Series x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299] | expand

Commit Message

H.J. Lu Oct. 17, 2017, 3:44 p.m. UTC 
Since ld.so expands $PLATFORM with GLRO(dl_platform), don't set
GLRO(dl_platform) to NULL.

OK for master and 2.26 branch?

H.J.
---
2017-10-17  Valery Reznic <valery_reznic@yahoo.com>
	    H.J. Lu  <hongjiu.lu@intel.com>

	[BZ #22299]
	* sysdeps/x86/cpu-features.c (init_cpu_features): Don't set
	GLRO(dl_platform) to NULL.
	* sysdeps/x86_64/Makefile (tests): Add tst-platform-1.
	(modules-names): Add tst-platformmod-1 and
	x86_64/tst-platformmod-2.
	(CFLAGS-tst-platform-1.c): New.
	(CFLAGS-tst-platformmod-1.c): Likewise.
	(CFLAGS-tst-platformmod-2.c): Likewise.
	(LDFLAGS-tst-platformmod-2.so): Likewise.
	($(objpfx)tst-platform-1): Likewise.
	($(objpfx)tst-platform-1.out): Likewise.
	(tst-platform-1-ENV): Likewise.
	($(objpfx)x86_64/tst-platformmod-2.os): Likewise.
	* sysdeps/x86_64/tst-platform-1.c: New file.
	* sysdeps/x86_64/tst-platformmod-1.c: Likewise.
	* sysdeps/x86_64/tst-platformmod-2.c: Likewise.
---
 sysdeps/x86/cpu-features.c         | 12 ++++++++----
 sysdeps/x86_64/Makefile            | 18 ++++++++++++++++++
 sysdeps/x86_64/tst-platform-1.c    | 29 +++++++++++++++++++++++++++++
 sysdeps/x86_64/tst-platformmod-1.c | 23 +++++++++++++++++++++++
 sysdeps/x86_64/tst-platformmod-2.c | 23 +++++++++++++++++++++++
 5 files changed, 101 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/x86_64/tst-platform-1.c
 create mode 100644 sysdeps/x86_64/tst-platformmod-1.c
 create mode 100644 sysdeps/x86_64/tst-platformmod-2.c

Comments

Valery Reznic Oct. 17, 2017, 4:09 p.m. UTC | #1 
And while we on it - I don't think that dl_hwcap should be reset either:
sysdeps/x86/cpu-features.c:  /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */                 
  GLRO(dl_hwcap) = 0;   
If kernel provide in the AT_HWCAP/AT_HWCAP2 some info, why destroy it?
Or I missed something?
Valery
      From: H.J. Lu <hongjiu.lu@intel.com>
 To: GNU C Library <libc-alpha@sourceware.org> 
Cc: Valery Reznic <valery_reznic@yahoo.com>
 Sent: Tuesday, October 17, 2017 6:44 PM
 Subject: [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299]
   
Since ld.so expands $PLATFORM with GLRO(dl_platform), don't set
GLRO(dl_platform) to NULL.

OK for master and 2.26 branch?

H.J.
---
2017-10-17  Valery Reznic <valery_reznic@yahoo.com>
        H.J. Lu  <hongjiu.lu@intel.com>

    [BZ #22299]
    * sysdeps/x86/cpu-features.c (init_cpu_features): Don't set
    GLRO(dl_platform) to NULL.
    * sysdeps/x86_64/Makefile (tests): Add tst-platform-1.
    (modules-names): Add tst-platformmod-1 and
    x86_64/tst-platformmod-2.
    (CFLAGS-tst-platform-1.c): New.
    (CFLAGS-tst-platformmod-1.c): Likewise.
    (CFLAGS-tst-platformmod-2.c): Likewise.
    (LDFLAGS-tst-platformmod-2.so): Likewise.
    ($(objpfx)tst-platform-1): Likewise.
    ($(objpfx)tst-platform-1.out): Likewise.
    (tst-platform-1-ENV): Likewise.
    ($(objpfx)x86_64/tst-platformmod-2.os): Likewise.
    * sysdeps/x86_64/tst-platform-1.c: New file.
    * sysdeps/x86_64/tst-platformmod-1.c: Likewise.
    * sysdeps/x86_64/tst-platformmod-2.c: Likewise.
---
 sysdeps/x86/cpu-features.c        | 12 ++++++++----
 sysdeps/x86_64/Makefile            | 18 ++++++++++++++++++
 sysdeps/x86_64/tst-platform-1.c    | 29 +++++++++++++++++++++++++++++
 sysdeps/x86_64/tst-platformmod-1.c | 23 +++++++++++++++++++++++
 sysdeps/x86_64/tst-platformmod-2.c | 23 +++++++++++++++++++++++
 5 files changed, 101 insertions(+), 4 deletions(-)
 create mode 100644 sysdeps/x86_64/tst-platform-1.c
 create mode 100644 sysdeps/x86_64/tst-platformmod-1.c
 create mode 100644 sysdeps/x86_64/tst-platformmod-2.c

diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index c267f17b76..1b3cfcc7cd 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -335,7 +335,6 @@ no_cpuid:
 #endif
 
  /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
-  GLRO(dl_platform) = NULL;
 #if !HAVE_TUNABLES && defined SHARED
  /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
      this.  */
@@ -346,13 +345,15 @@ no_cpuid:
  GLRO(dl_hwcap) = HWCAP_X86_64;
  if (cpu_features->kind == arch_kind_intel)
    {
+      const char *platform = NULL;
+
      if (CPU_FEATURES_ARCH_P (cpu_features, AVX512F_Usable)
       && CPU_FEATURES_CPU_P (cpu_features, AVX512CD))
     {
       if (CPU_FEATURES_CPU_P (cpu_features, AVX512ER))
         {
           if (CPU_FEATURES_CPU_P (cpu_features, AVX512PF))
-        GLRO(dl_platform) = "xeon_phi";
+        platform = "xeon_phi";
         }
       else
         {
@@ -363,7 +364,7 @@ no_cpuid:
         }
     }
 
-      if (GLRO(dl_platform) == NULL
+      if (platform == NULL
       && CPU_FEATURES_ARCH_P (cpu_features, AVX2_Usable)
       && CPU_FEATURES_ARCH_P (cpu_features, FMA_Usable)
       && CPU_FEATURES_CPU_P (cpu_features, BMI1)
@@ -371,7 +372,10 @@ no_cpuid:
       && CPU_FEATURES_CPU_P (cpu_features, LZCNT)
       && CPU_FEATURES_CPU_P (cpu_features, MOVBE)
       && CPU_FEATURES_CPU_P (cpu_features, POPCNT))
-    GLRO(dl_platform) = "haswell";
+    platform = "haswell";
+
+      if (platform)
+    GLRO(dl_platform) = platform;
    }
 #else
  GLRO(dl_hwcap) = 0;
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 1514805f4a..9ccb8905b1 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -58,6 +58,19 @@ LDFLAGS-tst-x86_64mod-1.so = -Wl,-soname,tst-x86_64mod-1.so
 
 $(objpfx)tst-x86_64-1: $(objpfx)x86_64/tst-x86_64mod-1.so
 
+tests += tst-platform-1
+modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
+CFLAGS-tst-platform-1.c = -mno-avx
+CFLAGS-tst-platformmod-1.c = -mno-avx
+CFLAGS-tst-platformmod-2.c = -mno-avx
+LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
+$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
+$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
+# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
+# always set to x86_64.
+tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so \
+    GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
+
 tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 \
     tst-audit10 tst-sse tst-avx tst-avx512
 test-extras += tst-audit4-aux tst-audit10-aux \
@@ -141,3 +154,8 @@ do-tests-clean common-mostlyclean: tst-x86_64-1-clean
 .PHONY: tst-x86_64-1-clean
 tst-x86_64-1-clean:
     -rm -rf $(objpfx)x86_64
+
+$(objpfx)x86_64/tst-platformmod-2.os: $(objpfx)tst-platformmod-2.os
+    $(make-target-directory)
+    rm -f $@
+    ln $< $@
diff --git a/sysdeps/x86_64/tst-platform-1.c b/sysdeps/x86_64/tst-platform-1.c
new file mode 100644
index 0000000000..76a02e4b6d
--- /dev/null
+++ b/sysdeps/x86_64/tst-platform-1.c
@@ -0,0 +1,29 @@
+/* Test PRELOAD with $PLATFORM.
+  Copyright (C) 2017 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
+  <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+extern int preload (void);
+
+static int
+do_test (void)
+{
+  return preload () == 0x1234 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/x86_64/tst-platformmod-1.c b/sysdeps/x86_64/tst-platformmod-1.c
new file mode 100644
index 0000000000..9ef5e2b5be
--- /dev/null
+++ b/sysdeps/x86_64/tst-platformmod-1.c
@@ -0,0 +1,23 @@
+/* Test PRELOAD with $PLATFORM.
+  Copyright (C) 2017 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
+  <http://www.gnu.org/licenses/>.  */
+
+int
+preload (void)
+{
+  return 0;
+}
diff --git a/sysdeps/x86_64/tst-platformmod-2.c b/sysdeps/x86_64/tst-platformmod-2.c
new file mode 100644
index 0000000000..d0e5103892
--- /dev/null
+++ b/sysdeps/x86_64/tst-platformmod-2.c
@@ -0,0 +1,23 @@
+/* Test PRELOAD with $PLATFORM.
+  Copyright (C) 2017 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
+  <http://www.gnu.org/licenses/>.  */
+
+int
+preload (void)
+{
+  return 0x1234;
+}
H.J. Lu Oct. 17, 2017, 4:22 p.m. UTC | #2 
On Tue, Oct 17, 2017 at 9:09 AM, Valery Reznic <valery_reznic@yahoo.com> wrote:
> And while we on it - I don't think that dl_hwcap should be reset either:
>
> sysdeps/x86/cpu-features.c:
>   /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
>   GLRO(dl_hwcap) = 0;
>
> If kernel provide in the AT_HWCAP/AT_HWCAP2 some info, why destroy it?
>

Kernel doesn't provide anything useful for x86-64 and we can get everything
we need from CPUID.
H.J. Lu Oct. 18, 2017, 12:36 p.m. UTC | #3 
On Tue, Oct 17, 2017 at 11:28 PM, Valery Reznic <valery_reznic@yahoo.com> wrote:
>  I always thought that it was kernel responsibility to detect platform/hwcap
> and pass them to the application in the auxv vector.
>

X86 glibc doesn't use them.
Valery Reznic Oct. 19, 2017, 1:27 p.m. UTC | #4 
I see. 

So what about [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299] - any chance to have it in glibc 2.27?


      From: H.J. Lu <hjl.tools@gmail.com>
 To: Valery Reznic <valery_reznic@yahoo.com> 
Cc: GNU C Library <libc-alpha@sourceware.org>
 Sent: Wednesday, October 18, 2017 3:36 PM
 Subject: Re: [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299]
   
On Tue, Oct 17, 2017 at 11:28 PM, Valery Reznic <valery_reznic@yahoo.com> wrote:
>  I always thought that it was kernel responsibility to detect platform/hwcap
> and pass them to the application in the auxv vector.
>

X86 glibc doesn't use them.
H.J. Lu Oct. 19, 2017, 2:41 p.m. UTC | #5 
On Thu, Oct 19, 2017 at 6:27 AM, Valery Reznic <valery_reznic@yahoo.com> wrote:
> I see.
>
> So what about [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ
> #22299] - any chance to have it in glibc 2.27?
>
>

Tested on Haswell with and without --disable-tunables.   This is the patch I am
going to check in.
Florian Weimer Oct. 19, 2017, 2:51 p.m. UTC | #6 
On 10/19/2017 04:41 PM, H.J. Lu wrote:
> +      if (platform)
> +	GLRO(dl_platform) = platform;

This should use “if (platform != NULL)”.

> +ifneq (no,$(have-tunables))
> +tests += tst-platform-1
> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
> +CFLAGS-tst-platform-1.c = -mno-avx
> +CFLAGS-tst-platformmod-1.c = -mno-avx
> +CFLAGS-tst-platformmod-2.c = -mno-avx
> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
> +# always set to x86_64.
> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so \
> +	GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
> +endif

Does this build $(objpfx)/tst-platformmod-2.so?  I think this would 
invalidate part of the test.

Thanks,
Florian
H.J. Lu Oct. 19, 2017, 3:27 p.m. UTC | #7 
On Thu, Oct 19, 2017 at 7:51 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 10/19/2017 04:41 PM, H.J. Lu wrote:
>>
>> +      if (platform)
>> +       GLRO(dl_platform) = platform;
>
>
> This should use “if (platform != NULL)”.

Done.

>> +ifneq (no,$(have-tunables))
>> +tests += tst-platform-1
>> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
>> +CFLAGS-tst-platform-1.c = -mno-avx
>> +CFLAGS-tst-platformmod-1.c = -mno-avx
>> +CFLAGS-tst-platformmod-2.c = -mno-avx
>> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
>> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
>> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
>> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
>> +# always set to x86_64.
>> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so
>> \
>> +       GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
>> +endif
>
>
> Does this build $(objpfx)/tst-platformmod-2.so?  I think this would

No.  x86_64/tst-platformmod-2, not tst-platformmod-2, is added to
modules-names.

[hjl@gnu-6 build-x86_64-linux]$ find -name tst-platformmod-2.so
./elf/x86_64/tst-platformmod-2.so
[hjl@gnu-6 build-x86_64-linux]$

> invalidate part of the test.

I am checking it in now.

Thanks.
Valery Reznic Oct. 20, 2017, 7:41 a.m. UTC | #8 
Great, thanks.
Valery


      From: H.J. Lu <hjl.tools@gmail.com>
 To: Valery Reznic <valery_reznic@yahoo.com> 
Cc: GNU C Library <libc-alpha@sourceware.org>
 Sent: Thursday, October 19, 2017 5:41 PM
 Subject: Re: [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ #22299]
   
On Thu, Oct 19, 2017 at 6:27 AM, Valery Reznic <valery_reznic@yahoo.com> wrote:
> I see.
>
> So what about [PATCH] x86-64: Don't set GLRO(dl_platform) to NULL [BZ
> #22299] - any chance to have it in glibc 2.27?
>
>

Tested on Haswell with and without --disable-tunables.  This is the patch I am
going to check in.
H.J. Lu Oct. 22, 2017, 11:52 a.m. UTC | #9 
On Thu, Oct 19, 2017 at 8:27 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Oct 19, 2017 at 7:51 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 10/19/2017 04:41 PM, H.J. Lu wrote:
>>>
>>> +      if (platform)
>>> +       GLRO(dl_platform) = platform;
>>
>>
>> This should use “if (platform != NULL)”.
>
> Done.
>
>>> +ifneq (no,$(have-tunables))
>>> +tests += tst-platform-1
>>> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
>>> +CFLAGS-tst-platform-1.c = -mno-avx
>>> +CFLAGS-tst-platformmod-1.c = -mno-avx
>>> +CFLAGS-tst-platformmod-2.c = -mno-avx
>>> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
>>> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
>>> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
>>> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
>>> +# always set to x86_64.
>>> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so
>>> \
>>> +       GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
>>> +endif
>>
>>
>> Does this build $(objpfx)/tst-platformmod-2.so?  I think this would
>
> No.  x86_64/tst-platformmod-2, not tst-platformmod-2, is added to
> modules-names.
>
> [hjl@gnu-6 build-x86_64-linux]$ find -name tst-platformmod-2.so
> ./elf/x86_64/tst-platformmod-2.so
> [hjl@gnu-6 build-x86_64-linux]$
>
>> invalidate part of the test.
>
> I am checking it in now.
>

I'd like to backport it to 2.26 branch.  Any objections?
H.J. Lu Oct. 26, 2017, 2:34 p.m. UTC | #10 
On Sun, Oct 22, 2017 at 4:52 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Oct 19, 2017 at 8:27 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Oct 19, 2017 at 7:51 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> On 10/19/2017 04:41 PM, H.J. Lu wrote:
>>>>
>>>> +      if (platform)
>>>> +       GLRO(dl_platform) = platform;
>>>
>>>
>>> This should use “if (platform != NULL)”.
>>
>> Done.
>>
>>>> +ifneq (no,$(have-tunables))
>>>> +tests += tst-platform-1
>>>> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
>>>> +CFLAGS-tst-platform-1.c = -mno-avx
>>>> +CFLAGS-tst-platformmod-1.c = -mno-avx
>>>> +CFLAGS-tst-platformmod-2.c = -mno-avx
>>>> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
>>>> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
>>>> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
>>>> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
>>>> +# always set to x86_64.
>>>> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so
>>>> \
>>>> +       GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
>>>> +endif
>>>
>>>
>>> Does this build $(objpfx)/tst-platformmod-2.so?  I think this would
>>
>> No.  x86_64/tst-platformmod-2, not tst-platformmod-2, is added to
>> modules-names.
>>
>> [hjl@gnu-6 build-x86_64-linux]$ find -name tst-platformmod-2.so
>> ./elf/x86_64/tst-platformmod-2.so
>> [hjl@gnu-6 build-x86_64-linux]$
>>
>>> invalidate part of the test.
>>
>> I am checking it in now.
>>
>
> I'd like to backport it to 2.26 branch.  Any objections?
>

I am checking it in.
Aurelien Jarno Dec. 13, 2017, 8:48 a.m. UTC | #11 
[ Sorry to come back so late about this commit ]

On 2017-10-19 08:27, H.J. Lu wrote:
> On Thu, Oct 19, 2017 at 7:51 AM, Florian Weimer <fweimer@redhat.com> wrote:
> 
> >> +ifneq (no,$(have-tunables))
> >> +tests += tst-platform-1
> >> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
> >> +CFLAGS-tst-platform-1.c = -mno-avx
> >> +CFLAGS-tst-platformmod-1.c = -mno-avx
> >> +CFLAGS-tst-platformmod-2.c = -mno-avx
> >> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
> >> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
> >> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
> >> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
> >> +# always set to x86_64.
> >> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so
> >> \
> >> +       GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
> >> +endif
> >
> >
> > Does this build $(objpfx)/tst-platformmod-2.so?  I think this would
> 
> No.  x86_64/tst-platformmod-2, not tst-platformmod-2, is added to
> modules-names.
> 
> [hjl@gnu-6 build-x86_64-linux]$ find -name tst-platformmod-2.so
> ./elf/x86_64/tst-platformmod-2.so
> [hjl@gnu-6 build-x86_64-linux]$

This assumes that the platform for sysdeps/x86_64 is always "x86_64".
This is actually wrong for x32 for where it is set to i686. This causes
the test to fail on x32.

Aurelien
H.J. Lu Dec. 13, 2017, 12:06 p.m. UTC | #12 
On Wed, Dec 13, 2017 at 12:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> [ Sorry to come back so late about this commit ]
>
> On 2017-10-19 08:27, H.J. Lu wrote:
>> On Thu, Oct 19, 2017 at 7:51 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>
>> >> +ifneq (no,$(have-tunables))
>> >> +tests += tst-platform-1
>> >> +modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
>> >> +CFLAGS-tst-platform-1.c = -mno-avx
>> >> +CFLAGS-tst-platformmod-1.c = -mno-avx
>> >> +CFLAGS-tst-platformmod-2.c = -mno-avx
>> >> +LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
>> >> +$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
>> >> +$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
>> >> +# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
>> >> +# always set to x86_64.
>> >> +tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so
>> >> \
>> >> +       GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
>> >> +endif
>> >
>> >
>> > Does this build $(objpfx)/tst-platformmod-2.so?  I think this would
>>
>> No.  x86_64/tst-platformmod-2, not tst-platformmod-2, is added to
>> modules-names.
>>
>> [hjl@gnu-6 build-x86_64-linux]$ find -name tst-platformmod-2.so
>> ./elf/x86_64/tst-platformmod-2.so
>> [hjl@gnu-6 build-x86_64-linux]$
>
> This assumes that the platform for sysdeps/x86_64 is always "x86_64".
> This is actually wrong for x32 for where it is set to i686. This causes
> the test to fail on x32.
>

It is wrong for kernel to set AT_PLATFORM to i686.   Should we fix it
in glibc or in kernel?
diff mbox series

Patch

diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
index c267f17b76..1b3cfcc7cd 100644
--- a/sysdeps/x86/cpu-features.c
+++ b/sysdeps/x86/cpu-features.c
@@ -335,7 +335,6 @@  no_cpuid:
 #endif
 
   /* Reuse dl_platform, dl_hwcap and dl_hwcap_mask for x86.  */
-  GLRO(dl_platform) = NULL;
 #if !HAVE_TUNABLES && defined SHARED
   /* The glibc.tune.hwcap_mask tunable is initialized already, so no need to do
      this.  */
@@ -346,13 +345,15 @@  no_cpuid:
   GLRO(dl_hwcap) = HWCAP_X86_64;
   if (cpu_features->kind == arch_kind_intel)
     {
+      const char *platform = NULL;
+
       if (CPU_FEATURES_ARCH_P (cpu_features, AVX512F_Usable)
 	  && CPU_FEATURES_CPU_P (cpu_features, AVX512CD))
 	{
 	  if (CPU_FEATURES_CPU_P (cpu_features, AVX512ER))
 	    {
 	      if (CPU_FEATURES_CPU_P (cpu_features, AVX512PF))
-		GLRO(dl_platform) = "xeon_phi";
+		platform = "xeon_phi";
 	    }
 	  else
 	    {
@@ -363,7 +364,7 @@  no_cpuid:
 	    }
 	}
 
-      if (GLRO(dl_platform) == NULL
+      if (platform == NULL
 	  && CPU_FEATURES_ARCH_P (cpu_features, AVX2_Usable)
 	  && CPU_FEATURES_ARCH_P (cpu_features, FMA_Usable)
 	  && CPU_FEATURES_CPU_P (cpu_features, BMI1)
@@ -371,7 +372,10 @@  no_cpuid:
 	  && CPU_FEATURES_CPU_P (cpu_features, LZCNT)
 	  && CPU_FEATURES_CPU_P (cpu_features, MOVBE)
 	  && CPU_FEATURES_CPU_P (cpu_features, POPCNT))
-	GLRO(dl_platform) = "haswell";
+	platform = "haswell";
+
+      if (platform)
+	GLRO(dl_platform) = platform;
     }
 #else
   GLRO(dl_hwcap) = 0;
diff --git a/sysdeps/x86_64/Makefile b/sysdeps/x86_64/Makefile
index 1514805f4a..9ccb8905b1 100644
--- a/sysdeps/x86_64/Makefile
+++ b/sysdeps/x86_64/Makefile
@@ -58,6 +58,19 @@  LDFLAGS-tst-x86_64mod-1.so = -Wl,-soname,tst-x86_64mod-1.so
 
 $(objpfx)tst-x86_64-1: $(objpfx)x86_64/tst-x86_64mod-1.so
 
+tests += tst-platform-1
+modules-names += tst-platformmod-1 x86_64/tst-platformmod-2
+CFLAGS-tst-platform-1.c = -mno-avx
+CFLAGS-tst-platformmod-1.c = -mno-avx
+CFLAGS-tst-platformmod-2.c = -mno-avx
+LDFLAGS-tst-platformmod-2.so = -Wl,-soname,tst-platformmod-2.so
+$(objpfx)tst-platform-1: $(objpfx)tst-platformmod-1.so
+$(objpfx)tst-platform-1.out: $(objpfx)x86_64/tst-platformmod-2.so
+# Turn off AVX512F_Usable and AVX2_Usable so that GLRO(dl_platform) is
+# always set to x86_64.
+tst-platform-1-ENV = LD_PRELOAD=$(objpfx)\$$PLATFORM/tst-platformmod-2.so \
+	GLIBC_TUNABLES=glibc.tune.hwcaps=-AVX512F_Usable,-AVX2_Usable
+
 tests += tst-audit3 tst-audit4 tst-audit5 tst-audit6 tst-audit7 \
 	 tst-audit10 tst-sse tst-avx tst-avx512
 test-extras += tst-audit4-aux tst-audit10-aux \
@@ -141,3 +154,8 @@  do-tests-clean common-mostlyclean: tst-x86_64-1-clean
 .PHONY: tst-x86_64-1-clean
 tst-x86_64-1-clean:
 	-rm -rf $(objpfx)x86_64
+
+$(objpfx)x86_64/tst-platformmod-2.os: $(objpfx)tst-platformmod-2.os
+	$(make-target-directory)
+	rm -f $@
+	ln $< $@
diff --git a/sysdeps/x86_64/tst-platform-1.c b/sysdeps/x86_64/tst-platform-1.c
new file mode 100644
index 0000000000..76a02e4b6d
--- /dev/null
+++ b/sysdeps/x86_64/tst-platform-1.c
@@ -0,0 +1,29 @@ 
+/* Test PRELOAD with $PLATFORM.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+
+extern int preload (void);
+
+static int
+do_test (void)
+{
+  return preload () == 0x1234 ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/x86_64/tst-platformmod-1.c b/sysdeps/x86_64/tst-platformmod-1.c
new file mode 100644
index 0000000000..9ef5e2b5be
--- /dev/null
+++ b/sysdeps/x86_64/tst-platformmod-1.c
@@ -0,0 +1,23 @@ 
+/* Test PRELOAD with $PLATFORM.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+int
+preload (void)
+{
+  return 0;
+}
diff --git a/sysdeps/x86_64/tst-platformmod-2.c b/sysdeps/x86_64/tst-platformmod-2.c
new file mode 100644
index 0000000000..d0e5103892
--- /dev/null
+++ b/sysdeps/x86_64/tst-platformmod-2.c
@@ -0,0 +1,23 @@ 
+/* Test PRELOAD with $PLATFORM.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+int
+preload (void)
+{
+  return 0x1234;
+}