Message ID | 20221214172908.2433968-1-hjl.tools@gmail.com |
---|---|
State | New |
Headers | show |
Series | x86: Add a testcase for BZ #29863 | expand |
On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > When a thread is updating the memory area of memcmp and another thread > is doing memcmp, the memcmp return value is undefined. Add a testcase > to verify that memcmp won't segfault in this case. > --- > sysdeps/x86/Makefile | 11 +++++ > sysdeps/x86/tst-memcmp-race-sse2.c | 1 + > sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++ > 3 files changed, 87 insertions(+) > create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c > create mode 100644 sysdeps/x86/tst-memcmp-race.c > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > index 56fd5fc805..3e07e18c7d 100644 > --- a/sysdeps/x86/Makefile > +++ b/sysdeps/x86/Makefile > @@ -257,3 +257,14 @@ tests += \ > tests-static += \ > tst-sysconf-cache-linesize-static > endif > + > +ifeq ($(subdir),nptl) > +tests += \ > + tst-memcmp-race-sse2 \ > + tst-memcmp-race \ > +# tests > + > +CFLAGS-tst-memcmp-race-sse2.c += -O0 > +CFLAGS-tst-memcmp-race.c += -O0 > +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2 > +endif > diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c > new file mode 100644 > index 0000000000..2986e418f3 > --- /dev/null > +++ b/sysdeps/x86/tst-memcmp-race-sse2.c > @@ -0,0 +1 @@ > +#include "tst-memcmp-race.c" > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c > new file mode 100644 > index 0000000000..f8f8e47f6e > --- /dev/null > +++ b/sysdeps/x86/tst-memcmp-race.c > @@ -0,0 +1,75 @@ > +/* Test case for memcmp with race condition. > + Copyright (C) 2022 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 <stdio.h> > +#include <stdint.h> > +#include <string.h> > +#include <support/xthread.h> > + > +#define NUM_THREADS 2 > +#define LEN 17 Imo if we test this `LEN` should be a variable and we should test all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are other cases. > +#define STR "abcdefghijklmnopq" > +#define LOOP1 100 > +#define LOOP2 1000000 > + > +char buf1[LEN], buf2[LEN]; I had to make this volatile to reproduce the failure with GCC 12, does this properly segfault w.o the fix patch? > + > +static void * > +childThread (void *tArgs) > +{ > + int i; > + > + if (0 == (size_t)tArgs % 2) > + { > + for (i = 0; i < LOOP1; i++) > + { > + int result; > + > + result = memcmp (buf1, buf2, LEN); If we are testing this I think it should test each impl. > + printf ("i = %d : result = %d\n", i, result); IMO remove the print and make `result` volatile. > + } > + } > + else > + { > + for (i = 0; i < LOOP2; i++) > + buf1[1] = (0 == (i % 2)) ? 'b' : 'c'; > + } > + > + return NULL; > +} > + > +static int > +do_test (void) > +{ > + int i; > + pthread_t threads[NUM_THREADS]; > + > + memcpy (buf1, STR, LEN); > + memcpy (buf2, STR, LEN); > + > + for (i = 0; i < NUM_THREADS; ++i) > + threads[i] = xpthread_create (NULL, childThread, > + (void *)(uintptr_t) i); > + > + for (i = 0; i < NUM_THREADS; ++i) > + xpthread_join (threads[i]); > + > + return 0; > +} > + > +#include <support/test-driver.c> > -- > 2.38.1 > Not sure we want this, given its not behavior we have decided to actively support its failures could be misleading.
On Wed, Dec 14, 2022 at 9:45 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > When a thread is updating the memory area of memcmp and another thread > > is doing memcmp, the memcmp return value is undefined. Add a testcase > > to verify that memcmp won't segfault in this case. > > --- > > sysdeps/x86/Makefile | 11 +++++ > > sysdeps/x86/tst-memcmp-race-sse2.c | 1 + > > sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++ > > 3 files changed, 87 insertions(+) > > create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c > > create mode 100644 sysdeps/x86/tst-memcmp-race.c > > > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > > index 56fd5fc805..3e07e18c7d 100644 > > --- a/sysdeps/x86/Makefile > > +++ b/sysdeps/x86/Makefile > > @@ -257,3 +257,14 @@ tests += \ > > tests-static += \ > > tst-sysconf-cache-linesize-static > > endif > > + > > +ifeq ($(subdir),nptl) > > +tests += \ > > + tst-memcmp-race-sse2 \ > > + tst-memcmp-race \ > > +# tests > > + > > +CFLAGS-tst-memcmp-race-sse2.c += -O0 > > +CFLAGS-tst-memcmp-race.c += -O0 > > +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2 > > +endif > > diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c > > new file mode 100644 > > index 0000000000..2986e418f3 > > --- /dev/null > > +++ b/sysdeps/x86/tst-memcmp-race-sse2.c > > @@ -0,0 +1 @@ > > +#include "tst-memcmp-race.c" > > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c > > new file mode 100644 > > index 0000000000..f8f8e47f6e > > --- /dev/null > > +++ b/sysdeps/x86/tst-memcmp-race.c > > @@ -0,0 +1,75 @@ > > +/* Test case for memcmp with race condition. > > + Copyright (C) 2022 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 <stdio.h> > > +#include <stdint.h> > > +#include <string.h> > > +#include <support/xthread.h> > > + > > +#define NUM_THREADS 2 > > +#define LEN 17 > > Imo if we test this `LEN` should be a variable and we should test > all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are > other cases. Fixd in v2. > > +#define STR "abcdefghijklmnopq" > > +#define LOOP1 100 > > +#define LOOP2 1000000 > > + > > +char buf1[LEN], buf2[LEN]; > > I had to make this volatile to reproduce the failure > with GCC 12, does this properly segfault w.o the > fix patch? Yes, with -O0. > > + > > +static void * > > +childThread (void *tArgs) > > +{ > > + int i; > > + > > + if (0 == (size_t)tArgs % 2) > > + { > > + for (i = 0; i < LOOP1; i++) > > + { > > + int result; > > + > > + result = memcmp (buf1, buf2, LEN); > > If we are testing this I think it should test each impl. Fixed in v2. > > + printf ("i = %d : result = %d\n", i, result); > > IMO remove the print and make `result` volatile. There is a race condition. -O0 can reproduce it with GCC 12. > > + } > > + } > > + else > > + { > > + for (i = 0; i < LOOP2; i++) > > + buf1[1] = (0 == (i % 2)) ? 'b' : 'c'; > > + } > > + > > + return NULL; > > +} > > + > > +static int > > +do_test (void) > > +{ > > + int i; > > + pthread_t threads[NUM_THREADS]; > > + > > + memcpy (buf1, STR, LEN); > > + memcpy (buf2, STR, LEN); > > + > > + for (i = 0; i < NUM_THREADS; ++i) > > + threads[i] = xpthread_create (NULL, childThread, > > + (void *)(uintptr_t) i); > > + > > + for (i = 0; i < NUM_THREADS; ++i) > > + xpthread_join (threads[i]); > > + > > + return 0; > > +} > > + > > +#include <support/test-driver.c> > > -- > > 2.38.1 > > > > Not sure we want this, given its not behavior we have decided > to actively support its failures could be misleading. This testcase only checks segfault, not memcmp return values.
On Wed, Dec 14, 2022 at 11:49 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > On Wed, Dec 14, 2022 at 9:45 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote: > > > > On Wed, Dec 14, 2022 at 9:29 AM H.J. Lu <hjl.tools@gmail.com> wrote: > > > > > > When a thread is updating the memory area of memcmp and another thread > > > is doing memcmp, the memcmp return value is undefined. Add a testcase > > > to verify that memcmp won't segfault in this case. > > > --- > > > sysdeps/x86/Makefile | 11 +++++ > > > sysdeps/x86/tst-memcmp-race-sse2.c | 1 + > > > sysdeps/x86/tst-memcmp-race.c | 75 ++++++++++++++++++++++++++++++ > > > 3 files changed, 87 insertions(+) > > > create mode 100644 sysdeps/x86/tst-memcmp-race-sse2.c > > > create mode 100644 sysdeps/x86/tst-memcmp-race.c > > > > > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > > > index 56fd5fc805..3e07e18c7d 100644 > > > --- a/sysdeps/x86/Makefile > > > +++ b/sysdeps/x86/Makefile > > > @@ -257,3 +257,14 @@ tests += \ > > > tests-static += \ > > > tst-sysconf-cache-linesize-static > > > endif > > > + > > > +ifeq ($(subdir),nptl) > > > +tests += \ > > > + tst-memcmp-race-sse2 \ > > > + tst-memcmp-race \ > > > +# tests > > > + > > > +CFLAGS-tst-memcmp-race-sse2.c += -O0 > > > +CFLAGS-tst-memcmp-race.c += -O0 > > > +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2 > > > +endif > > > diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c > > > new file mode 100644 > > > index 0000000000..2986e418f3 > > > --- /dev/null > > > +++ b/sysdeps/x86/tst-memcmp-race-sse2.c > > > @@ -0,0 +1 @@ > > > +#include "tst-memcmp-race.c" > > > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c > > > new file mode 100644 > > > index 0000000000..f8f8e47f6e > > > --- /dev/null > > > +++ b/sysdeps/x86/tst-memcmp-race.c > > > @@ -0,0 +1,75 @@ > > > +/* Test case for memcmp with race condition. > > > + Copyright (C) 2022 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 <stdio.h> > > > +#include <stdint.h> > > > +#include <string.h> > > > +#include <support/xthread.h> > > > + > > > +#define NUM_THREADS 2 > > > +#define LEN 17 > > > > Imo if we test this `LEN` should be a variable and we should test > > all 2^N + 1 for N in [0, 12]. I wouldn't be suprised if there are > > other cases. > > Fixd in v2. > > > > +#define STR "abcdefghijklmnopq" > > > +#define LOOP1 100 > > > +#define LOOP2 1000000 > > > + > > > +char buf1[LEN], buf2[LEN]; > > > > I had to make this volatile to reproduce the failure > > with GCC 12, does this properly segfault w.o the > > fix patch? > > Yes, with -O0. > > > > + > > > +static void * > > > +childThread (void *tArgs) > > > +{ > > > + int i; > > > + > > > + if (0 == (size_t)tArgs % 2) > > > + { > > > + for (i = 0; i < LOOP1; i++) > > > + { > > > + int result; > > > + > > > + result = memcmp (buf1, buf2, LEN); > > > > If we are testing this I think it should test each impl. > > Fixed in v2. > > > > + printf ("i = %d : result = %d\n", i, result); > > > > IMO remove the print and make `result` volatile. > > There is a race condition. -O0 can reproduce it with GCC 12. > > > > + } > > > + } > > > + else > > > + { > > > + for (i = 0; i < LOOP2; i++) > > > + buf1[1] = (0 == (i % 2)) ? 'b' : 'c'; > > > + } > > > + > > > + return NULL; > > > +} > > > + > > > +static int > > > +do_test (void) > > > +{ > > > + int i; > > > + pthread_t threads[NUM_THREADS]; > > > + > > > + memcpy (buf1, STR, LEN); > > > + memcpy (buf2, STR, LEN); > > > + > > > + for (i = 0; i < NUM_THREADS; ++i) > > > + threads[i] = xpthread_create (NULL, childThread, > > > + (void *)(uintptr_t) i); > > > + > > > + for (i = 0; i < NUM_THREADS; ++i) > > > + xpthread_join (threads[i]); > > > + > > > + return 0; > > > +} > > > + > > > +#include <support/test-driver.c> > > > -- > > > 2.38.1 > > > > > > > Not sure we want this, given its not behavior we have decided > > to actively support its failures could be misleading. > > This testcase only checks segfault, not memcmp return values. At this point I don't think we have committed to not segfaulting in the data-race case. > > -- > H.J.
diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile index 56fd5fc805..3e07e18c7d 100644 --- a/sysdeps/x86/Makefile +++ b/sysdeps/x86/Makefile @@ -257,3 +257,14 @@ tests += \ tests-static += \ tst-sysconf-cache-linesize-static endif + +ifeq ($(subdir),nptl) +tests += \ + tst-memcmp-race-sse2 \ + tst-memcmp-race \ +# tests + +CFLAGS-tst-memcmp-race-sse2.c += -O0 +CFLAGS-tst-memcmp-race.c += -O0 +tst-memcmp-race-sse2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-AVX2 +endif diff --git a/sysdeps/x86/tst-memcmp-race-sse2.c b/sysdeps/x86/tst-memcmp-race-sse2.c new file mode 100644 index 0000000000..2986e418f3 --- /dev/null +++ b/sysdeps/x86/tst-memcmp-race-sse2.c @@ -0,0 +1 @@ +#include "tst-memcmp-race.c" diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c new file mode 100644 index 0000000000..f8f8e47f6e --- /dev/null +++ b/sysdeps/x86/tst-memcmp-race.c @@ -0,0 +1,75 @@ +/* Test case for memcmp with race condition. + Copyright (C) 2022 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 <stdio.h> +#include <stdint.h> +#include <string.h> +#include <support/xthread.h> + +#define NUM_THREADS 2 +#define LEN 17 +#define STR "abcdefghijklmnopq" +#define LOOP1 100 +#define LOOP2 1000000 + +char buf1[LEN], buf2[LEN]; + +static void * +childThread (void *tArgs) +{ + int i; + + if (0 == (size_t)tArgs % 2) + { + for (i = 0; i < LOOP1; i++) + { + int result; + + result = memcmp (buf1, buf2, LEN); + printf ("i = %d : result = %d\n", i, result); + } + } + else + { + for (i = 0; i < LOOP2; i++) + buf1[1] = (0 == (i % 2)) ? 'b' : 'c'; + } + + return NULL; +} + +static int +do_test (void) +{ + int i; + pthread_t threads[NUM_THREADS]; + + memcpy (buf1, STR, LEN); + memcpy (buf2, STR, LEN); + + for (i = 0; i < NUM_THREADS; ++i) + threads[i] = xpthread_create (NULL, childThread, + (void *)(uintptr_t) i); + + for (i = 0; i < NUM_THREADS; ++i) + xpthread_join (threads[i]); + + return 0; +} + +#include <support/test-driver.c>