| Message ID | 20180927194327.7683-3-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | V5 [PATCH 2/2] x86: Add a LD_PRELOAD IFUNC resolver test for CPU_FEATURE_USABLE | expand |
* H. J. Lu: > +static bool > +ifunc_check_sse2 (void) > +{ > + return CPU_FEATURE_USABLE (SSE2); > +} > + > +static bool > +(*get_check_sse2 (void)) (void) > +{ > + return ifunc_check_sse2; > +} > + > +bool check_sse2 (void) __attribute__((ifunc("get_check_sse2"))); The IFUNC resolver does not call CPU_FEATURE_USABLE, so this is not the test I had in mind. The patch below fixes this, and adds something that actually violates the ordering constraints. With these changes, I get this when building glibc with --enable-bind-now: elf/tst-x86-platform-4: Relink `./libc.so.6' with `elf/tst-x86-platform-preload-4.so' for IFUNC symbol `free' Segmentation fault (core dumped) I think using CPU_FEATURE_USABLE in a malloc implementation is something that could be quite natural, so I think this really should be fixed in some way. Thanks, Florian diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile index 1e759d3efc..773b9137b3 100644 --- a/sysdeps/x86/Makefile +++ b/sysdeps/x86/Makefile @@ -22,6 +22,9 @@ $(objpfx)tst-x86-platform-mod-4.so: $(libsupport) $(objpfx)tst-x86-platform-4: $(objpfx)tst-x86-platform-mod-4.so $(objpfx)tst-x86-platform-4.out: $(objpfx)tst-x86-platform-preload-4.so tst-x86-platform-4-ENV = LD_PRELOAD=$(objpfx)tst-x86-platform-preload-4.so +LDFLAGS-tst-x86-platform-preload-4.so = -Wl,-z,now +LDFLAGS-tst-x86-platform-4.so = -Wl,-z,now +LDFLAGS-tst-x86-platform-4 = -Wl,-z,now endif ifeq ($(subdir),misc) diff --git a/sysdeps/x86/tst-x86-platform-4.c b/sysdeps/x86/tst-x86-platform-4.c index fe7bd59b82..fb4e86b566 100644 --- a/sysdeps/x86/tst-x86-platform-4.c +++ b/sysdeps/x86/tst-x86-platform-4.c @@ -16,14 +16,18 @@ License along with the GNU C Library; if not, see <http://www.gnu.org/licenses/>. */ -#include <stdlib.h> -#include <stdio.h> #include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <support/check.h> +#include <support/xstdio.h> #include <sys/platform/x86.h> extern bool check_sse2 (void); extern bool check_avx (void); extern bool check_avx2 (void); +extern int get_expected_free_variant (void); +extern volatile int free_variant_called; static int do_test (void) @@ -48,6 +52,22 @@ do_test (void) failed = true; } + /* Determine if the correct free function was selected. */ + int expected_variant = get_expected_free_variant (); + free (NULL); + TEST_COMPARE (expected_variant, free_variant_called); + + /* Same for a free within libc.so.6. */ + { + FILE * fp = xfopen ("/dev/null", "r"); + free_variant_called = 0; + xfclose (fp); + if (free_variant_called == 0) + puts ("warning: fclose did not call free"); + else + TEST_COMPARE (expected_variant, free_variant_called); + } + return failed ? EXIT_FAILURE : EXIT_SUCCESS; } diff --git a/sysdeps/x86/tst-x86-platform-mod-4.c b/sysdeps/x86/tst-x86-platform-mod-4.c index 789389ce3e..031634c615 100644 --- a/sysdeps/x86/tst-x86-platform-mod-4.c +++ b/sysdeps/x86/tst-x86-platform-mod-4.c @@ -35,3 +35,11 @@ check_avx2 (void) { return false; } + +int +get_expected_free_variant (void) +{ + return -1; +} + +volatile int free_variant_called; diff --git a/sysdeps/x86/tst-x86-platform-preload-4.c b/sysdeps/x86/tst-x86-platform-preload-4.c index d1216b90e9..6cb5b82377 100644 --- a/sysdeps/x86/tst-x86-platform-preload-4.c +++ b/sysdeps/x86/tst-x86-platform-preload-4.c @@ -18,45 +18,118 @@ #include <stdbool.h> #include <sys/platform/x86.h> +#include <stdlib.h> static bool -ifunc_check_sse2 (void) +true_function (void) { - return CPU_FEATURE_USABLE (SSE2); + return true; } static bool -(*get_check_sse2 (void)) (void) +false_function (void) { - return ifunc_check_sse2; + return false; } -bool check_sse2 (void) __attribute__((ifunc("get_check_sse2"))); - static bool -ifunc_check_avx (void) +(*get_check_sse2 (void)) (void) { - return CPU_FEATURE_USABLE (AVX); + if (CPU_FEATURE_USABLE (SSE2)) + return true_function; + else + return false_function; } +bool check_sse2 (void) __attribute__ ((ifunc ("get_check_sse2"))); + static bool (*get_check_avx (void)) (void) { - return ifunc_check_avx; + if (CPU_FEATURE_USABLE (AVX)) + return true_function; + else + return false_function; } -bool check_avx (void) __attribute__((ifunc("get_check_avx"))); +bool check_avx (void) __attribute__ ((ifunc ("get_check_avx"))); static bool -ifunc_check_avx2 (void) +(*get_check_avx2 (void)) (void) { - return CPU_FEATURE_USABLE (AVX2); + if (CPU_FEATURE_USABLE (AVX2)) + return true_function; + else + return false_function; } -static bool -(*get_check_avx2 (void)) (void) +bool check_avx2 (void) __attribute__ ((ifunc ("get_check_avx2"))); + +/* The following is used to check what happens when the free function + in libc.so.6 is interposed. This implementation simply does not + free any memory, to avoid the need for a complete malloc. It + records the variant called in free_variant_called, so that it is + possible to check the selected implementation by calling the free + function. */ + +volatile int free_variant_called; + +void +free_fallback (void *ignored) +{ + free_variant_called = 1; +} + +void +free_with_see (void *ignored) +{ + free_variant_called = 2; +} + +void +free_with_avx (void *ignored) +{ + free_variant_called = 3; +} + +void +free_with_avx2 (void *ignored) +{ + free_variant_called = 4; +} + +void +free_with_rtm (void *ignored) +{ + free_variant_called = 5; +} + +int +get_expected_free_variant (void) +{ + if (CPU_FEATURE_USABLE (RTM)) + return 5; + if (CPU_FEATURE_USABLE (AVX2)) + return 4; + if (CPU_FEATURE_USABLE (AVX)) + return 3; + if (CPU_FEATURE_USABLE (SSE)) + return 2; + return 1; +} + +static __typeof__ (free) * +get_free (void) { - return ifunc_check_avx2; + if (CPU_FEATURE_USABLE (RTM)) + return free_with_rtm; + if (CPU_FEATURE_USABLE (AVX2)) + return free_with_avx2; + if (CPU_FEATURE_USABLE (AVX)) + return free_with_avx; + if (CPU_FEATURE_USABLE (SSE)) + return free_with_see; + return free_fallback; } -bool check_avx2 (void) __attribute__((ifunc("get_check_avx2"))); +void free (void *) __attribute__ ((ifunc ("get_free")));
On 10/24/18, Florian Weimer <fweimer@redhat.com> wrote: > * H. J. Lu: > >> +static bool >> +ifunc_check_sse2 (void) >> +{ >> + return CPU_FEATURE_USABLE (SSE2); >> +} >> + >> +static bool >> +(*get_check_sse2 (void)) (void) >> +{ >> + return ifunc_check_sse2; >> +} >> + >> +bool check_sse2 (void) __attribute__((ifunc("get_check_sse2"))); > > The IFUNC resolver does not call CPU_FEATURE_USABLE, so this is not the > test I had in mind. > > The patch below fixes this, and adds something that actually violates > the ordering constraints. With these changes, I get this when building > glibc with --enable-bind-now: > > elf/tst-x86-platform-4: Relink `./libc.so.6' with > `elf/tst-x86-platform-preload-4.so' for IFUNC symbol `free' > Segmentation fault (core dumped) > > I think using CPU_FEATURE_USABLE in a malloc implementation is something > that could be quite natural, so I think this really should be fixed in > some way. > > ... > + > +static __typeof__ (free) * > +get_free (void) > { > - return ifunc_check_avx2; > + if (CPU_FEATURE_USABLE (RTM)) > + return free_with_rtm; > + if (CPU_FEATURE_USABLE (AVX2)) > + return free_with_avx2; > + if (CPU_FEATURE_USABLE (AVX)) > + return free_with_avx; > + if (CPU_FEATURE_USABLE (SSE)) > + return free_with_see; > + return free_fallback; > } > > -bool check_avx2 (void) __attribute__((ifunc("get_check_avx2"))); > +void free (void *) __attribute__ ((ifunc ("get_free"))); > I guess you knew that this issue was independent of my new functions. You will get the same error regardless of what the get_free body has.
* H. J. Lu: > I guess you knew that this issue was independent of my new functions. > You will get the same error regardless of what the get_free body has. Yes, the check is certainly overly conservative. I thought we want to remove it. Don't we trigger it in glibc in a few places? If the check is gone, then I think we will see incorrect results from the new interface. I think we are very consistent right now when it comes to relocations in IFUNC handlers. I want to see this settled before adding something that requires a relocation which is (among other things) targeted at IFUNC resolvers. Thanks, Florian
On 10/24/18, Florian Weimer <fweimer@redhat.com> wrote: > * H. J. Lu: > >> I guess you knew that this issue was independent of my new functions. >> You will get the same error regardless of what the get_free body has. > > Yes, the check is certainly overly conservative. I thought we want to > remove it. Don't we trigger it in glibc in a few places? If the check > is gone, then I think we will see incorrect results from the new > interface. > > I think we are very consistent right now when it comes to relocations in > IFUNC handlers. I want to see this settled before adding something that > requires a relocation which is (among other things) targeted at IFUNC > resolvers. > <sys/platform/x86.h> isn't targeted for IFUNC. My first use is to add x86_tsc_to_ns and x86_ns_to_tsc. I am enclosing 2 patches here.
* H. J. Lu: > On 10/24/18, Florian Weimer <fweimer@redhat.com> wrote: >> * H. J. Lu: >> >>> I guess you knew that this issue was independent of my new functions. >>> You will get the same error regardless of what the get_free body has. >> >> Yes, the check is certainly overly conservative. I thought we want to >> remove it. Don't we trigger it in glibc in a few places? If the check >> is gone, then I think we will see incorrect results from the new >> interface. >> >> I think we are very consistent right now when it comes to relocations in >> IFUNC handlers. I want to see this settled before adding something that >> requires a relocation which is (among other things) targeted at IFUNC >> resolvers. >> > > <sys/platform/x86.h> isn't targeted for IFUNC. My first use is to add > x86_tsc_to_ns and x86_ns_to_tsc. I am enclosing 2 patches here. That's a generic interface which should rely on internal CPU flags. What's worse, the cached flag isn't updated by the kernel TSC watchdog, so applications will use a known-broken time source. What's wrong with clock_gettime? It handles all these details. Thanks, Florian
On 10/24/18, Florian Weimer <fweimer@redhat.com> wrote: > * H. J. Lu: > >> On 10/24/18, Florian Weimer <fweimer@redhat.com> wrote: >>> * H. J. Lu: >>> >>>> I guess you knew that this issue was independent of my new functions. >>>> You will get the same error regardless of what the get_free body has. >>> >>> Yes, the check is certainly overly conservative. I thought we want to >>> remove it. Don't we trigger it in glibc in a few places? If the check >>> is gone, then I think we will see incorrect results from the new >>> interface. >>> >>> I think we are very consistent right now when it comes to relocations in >>> IFUNC handlers. I want to see this settled before adding something that >>> requires a relocation which is (among other things) targeted at IFUNC >>> resolvers. >>> >> >> <sys/platform/x86.h> isn't targeted for IFUNC. My first use is to add >> x86_tsc_to_ns and x86_ns_to_tsc. I am enclosing 2 patches here. > > That's a generic interface which should rely on internal CPU flags. > What's worse, the cached flag isn't updated by the kernel TSC watchdog, > so applications will use a known-broken time source. > > What's wrong with clock_gettime? It handles all these details. > These are for cases where RDTSC/RDTSCP are preferred over clock_gettime.
diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile index 272d8f0b89..1e759d3efc 100644 --- a/sysdeps/x86/Makefile +++ b/sysdeps/x86/Makefile @@ -7,8 +7,9 @@ sysdep-dl-routines += dl-get-cpu-features tests += tst-get-cpu-features tst-get-cpu-features-static \ tst-x86-platform-1 tst-x86-platform-1-static \ - tst-x86-platform-2 tst-x86-platform-3 -modules-names += tst-x86-platform-mod-2 tst-x86-platform-mod-3 + tst-x86-platform-2 tst-x86-platform-3 tst-x86-platform-4 +modules-names += tst-x86-platform-mod-2 tst-x86-platform-mod-3 \ + tst-x86-platform-mod-4 tst-x86-platform-preload-4 tests-static += tst-get-cpu-features-static tst-x86-platform-1-static $(objpfx)tst-x86-platform-mod-2.so: $(libsupport) @@ -16,6 +17,11 @@ $(objpfx)tst-x86-platform-2: $(objpfx)tst-x86-platform-mod-2.so LDFLAGS-tst-x86-platform-mod-3.so = -Wl,-z,now $(objpfx)tst-x86-platform-mod-3.so: $(libsupport) $(objpfx)tst-x86-platform-3: $(objpfx)tst-x86-platform-mod-3.so +LDFLAGS-tst-x86-platform-mod-4.so = -Wl,-z,now +$(objpfx)tst-x86-platform-mod-4.so: $(libsupport) +$(objpfx)tst-x86-platform-4: $(objpfx)tst-x86-platform-mod-4.so +$(objpfx)tst-x86-platform-4.out: $(objpfx)tst-x86-platform-preload-4.so +tst-x86-platform-4-ENV = LD_PRELOAD=$(objpfx)tst-x86-platform-preload-4.so endif ifeq ($(subdir),misc) diff --git a/sysdeps/x86/tst-x86-platform-4.c b/sysdeps/x86/tst-x86-platform-4.c new file mode 100644 index 0000000000..fe7bd59b82 --- /dev/null +++ b/sysdeps/x86/tst-x86-platform-4.c @@ -0,0 +1,54 @@ +/* Test case for x86 <sys/platform/x86.h>. + Copyright (C) 2018 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> +#include <stdio.h> +#include <stdbool.h> +#include <sys/platform/x86.h> + +extern bool check_sse2 (void); +extern bool check_avx (void); +extern bool check_avx2 (void); + +static int +do_test (void) +{ + bool failed = false; + + if (CPU_FEATURE_USABLE (SSE2) != check_sse2 ()) + { + printf ("LD_PRELOAD SSE2 check failed\n"); + failed = true; + } + + if (CPU_FEATURE_USABLE (AVX) != check_avx ()) + { + printf ("LD_PRELOAD AVX check failed\n"); + failed = true; + } + + if (CPU_FEATURE_USABLE (AVX2) != check_avx2 ()) + { + printf ("LD_PRELOAD AVX2 check failed\n"); + failed = true; + } + + return failed ? EXIT_FAILURE : EXIT_SUCCESS; +} + +#include <support/test-driver.c> diff --git a/sysdeps/x86/tst-x86-platform-mod-4.c b/sysdeps/x86/tst-x86-platform-mod-4.c new file mode 100644 index 0000000000..789389ce3e --- /dev/null +++ b/sysdeps/x86/tst-x86-platform-mod-4.c @@ -0,0 +1,37 @@ +/* Test case for x86 <sys/platform/x86.h>. + Copyright (C) 2018 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 <stdbool.h> + +bool +check_sse2 (void) +{ + return false; +} + +bool +check_avx (void) +{ + return false; +} + +bool +check_avx2 (void) +{ + return false; +} diff --git a/sysdeps/x86/tst-x86-platform-preload-4.c b/sysdeps/x86/tst-x86-platform-preload-4.c new file mode 100644 index 0000000000..d1216b90e9 --- /dev/null +++ b/sysdeps/x86/tst-x86-platform-preload-4.c @@ -0,0 +1,62 @@ +/* Test case for x86 <sys/platform/x86.h>. + Copyright (C) 2018 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 <stdbool.h> +#include <sys/platform/x86.h> + +static bool +ifunc_check_sse2 (void) +{ + return CPU_FEATURE_USABLE (SSE2); +} + +static bool +(*get_check_sse2 (void)) (void) +{ + return ifunc_check_sse2; +} + +bool check_sse2 (void) __attribute__((ifunc("get_check_sse2"))); + +static bool +ifunc_check_avx (void) +{ + return CPU_FEATURE_USABLE (AVX); +} + +static bool +(*get_check_avx (void)) (void) +{ + return ifunc_check_avx; +} + +bool check_avx (void) __attribute__((ifunc("get_check_avx"))); + +static bool +ifunc_check_avx2 (void) +{ + return CPU_FEATURE_USABLE (AVX2); +} + +static bool +(*get_check_avx2 (void)) (void) +{ + return ifunc_check_avx2; +} + +bool check_avx2 (void) __attribute__((ifunc("get_check_avx2")));