From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x534.google.com (mail-pg1-x534.google.com [IPv6:2607:f8b0:4864:20::534]) by sourceware.org (Postfix) with ESMTPS id 767C6385AC2E for ; Thu, 17 Feb 2022 19:16:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 767C6385AC2E Received: by mail-pg1-x534.google.com with SMTP id h125so5838419pgc.3 for ; Thu, 17 Feb 2022 11:16:02 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=DMEMp8+kUTYnbPoGpwHb379mDWIXgk/U+4X4WRkZgoY=; b=vWdZMAah81y2f9KT86/Y5/Pgy7tFP2Pica7Gsu7D/VUYZ/vL1KNDyPMxX3N32AqJri qGCvUH1WcfC5XB6H4Blc3Rmkwd1f0k2fpsi8Ss3IVyO4dR6WK/KUczwJFWLXjZX0DS5+ Kjpf53MlbJ1wGSUkswTa6KCrKWVJzct3oXGzuOsyq+k2m+OTYUOCGAQmghO9CeIDAHte qtjCMeB3ZgQKfviKu2T08uqrF8qRBfNwYl/SdapvtNn0izSYpSRHdGpIi5a1UITnZQdO yQxcVDWTL4/BhBS/U6fmnMHHC/TE0LGwIhKGXU1xuSWwgEjjl6dJ3FFbvmNmAIrgnYPa FBXw== X-Gm-Message-State: AOAM5332rwO7OodI7Zk7YV5G3RBiiHxmrlaZLNtoWQfKcaIee4W3RXFj fKMK2DgevAWACzCn/5uWxlognSII5ZfOhUSXzl4= X-Google-Smtp-Source: ABdhPJxYiDruP3oNiIR4ChGPN6hEIRLSH/wazPS9XP65lrSKz3SoG1HSu90eIAwfpo3SVz4J7XtVwegLmBgCIBHPqKI= X-Received: by 2002:a63:e50c:0:b0:363:96b8:4334 with SMTP id r12-20020a63e50c000000b0036396b84334mr3542134pgh.18.1645125361448; Thu, 17 Feb 2022 11:16:01 -0800 (PST) MIME-Version: 1.0 References: <20220215162751.281955-1-goldstein.w.n@gmail.com> <20220216080935.3284536-1-goldstein.w.n@gmail.com> <867c131c-cec4-d8d6-5f24-cd51de94f1b2@redhat.com> In-Reply-To: From: Noah Goldstein Date: Thu, 17 Feb 2022 13:15:50 -0600 Message-ID: Subject: Re: [PATCH v3] x86: Fallback {str|wcs}cmp RTM in the ncmp overflow case [BZ #28896] To: "H.J. Lu" Cc: "Carlos O'Donell" , GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Feb 2022 19:16:04 -0000 On Wed, Feb 16, 2022 at 8:27 AM H.J. Lu wrote: > > On Wed, Feb 16, 2022 at 6:08 AM Carlos O'Donell wrote: > > > > On 2/16/22 08:17, H.J. Lu via Libc-alpha wrote: > > > On Wed, Feb 16, 2022 at 12:09 AM Noah Goldstein wrote: > > >> > > >> In the overflow fallback strncmp-avx2-rtm and wcsncmp-avx2-rtm would > > >> call strcmp-avx2 and wcsncmp-avx2 respectively. This would have > > >> not checks around vzeroupper and would trigger spurious > > >> aborts. This commit fixes that. > > >> > > >> test-strcmp, test-strncmp, test-wcscmp, and test-wcsncmp all pass on > > >> AVX2 machines with and without RTM. > > >> > > >> Co-authored-by: H.J. Lu > > >> --- > > >> sysdeps/x86/Makefile | 2 +- > > >> sysdeps/x86/tst-strncmp-rtm.c | 14 +++++++++++++- > > >> sysdeps/x86_64/multiarch/strcmp-avx2.S | 8 ++------ > > >> sysdeps/x86_64/multiarch/strncmp-avx2-rtm.S | 1 + > > >> sysdeps/x86_64/multiarch/strncmp-avx2.S | 1 + > > >> sysdeps/x86_64/multiarch/wcsncmp-avx2-rtm.S | 2 +- > > >> sysdeps/x86_64/multiarch/wcsncmp-avx2.S | 2 +- > > >> 7 files changed, 20 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > > >> index 6cf708335c..d110f7b7f2 100644 > > >> --- a/sysdeps/x86/Makefile > > >> +++ b/sysdeps/x86/Makefile > > >> @@ -109,7 +109,7 @@ CFLAGS-tst-memset-rtm.c += -mrtm > > >> CFLAGS-tst-strchr-rtm.c += -mrtm > > >> CFLAGS-tst-strcpy-rtm.c += -mrtm > > >> CFLAGS-tst-strlen-rtm.c += -mrtm > > >> -CFLAGS-tst-strncmp-rtm.c += -mrtm > > >> +CFLAGS-tst-strncmp-rtm.c += -mrtm -Wno-error > > >> CFLAGS-tst-strrchr-rtm.c += -mrtm > > >> endif > > >> > > >> diff --git a/sysdeps/x86/tst-strncmp-rtm.c b/sysdeps/x86/tst-strncmp-rtm.c > > >> index 09ed6fa0d6..ebc94a3a6d 100644 > > >> --- a/sysdeps/x86/tst-strncmp-rtm.c > > >> +++ b/sysdeps/x86/tst-strncmp-rtm.c > > >> @@ -16,6 +16,7 @@ > > >> License along with the GNU C Library; if not, see > > >> . */ > > >> > > >> +#include > > >> #include > > >> > > >> #define LOOP 3000 > > >> @@ -45,8 +46,19 @@ function (void) > > >> return 1; > > >> } > > >> > > >> +__attribute__ ((noinline, noclone)) > > >> +static int > > >> +function_overflow (void) > > >> +{ > > >> + if (strncmp (string1, string2, SIZE_MAX) == 0) > > >> + return 0; > > >> + else > > >> + return 1; > > >> +} > > >> + > > >> static int > > >> do_test (void) > > >> { > > >> - return do_test_1 ("strncmp", LOOP, prepare, function); > > >> + return (do_test_1 ("strncmp", LOOP, prepare, function) > > >> + || do_test_1 ("strncmp", LOOP, prepare, function_overflow)); > > >> } > > >> diff --git a/sysdeps/x86_64/multiarch/strcmp-avx2.S b/sysdeps/x86_64/multiarch/strcmp-avx2.S > > >> index 99e5349be8..6da0e1a248 100644 > > >> --- a/sysdeps/x86_64/multiarch/strcmp-avx2.S > > >> +++ b/sysdeps/x86_64/multiarch/strcmp-avx2.S > > >> @@ -193,10 +193,10 @@ L(ret_zero): > > >> .p2align 4,, 5 > > >> L(one_or_less): > > >> jb L(ret_zero) > > >> -# ifdef USE_AS_WCSCMP > > >> /* 'nbe' covers the case where length is negative (large > > >> unsigned). */ > > >> - jnbe __wcscmp_avx2 > > >> + jnbe OVERFLOW_STRCMP > > >> +# ifdef USE_AS_WCSCMP > > >> movl (%rdi), %edx > > >> xorl %eax, %eax > > >> cmpl (%rsi), %edx > > >> @@ -205,10 +205,6 @@ L(one_or_less): > > >> negl %eax > > >> orl $1, %eax > > >> # else > > >> - /* 'nbe' covers the case where length is negative (large > > >> - unsigned). */ > > >> - > > >> - jnbe __strcmp_avx2 > > >> movzbl (%rdi), %eax > > >> movzbl (%rsi), %ecx > > >> subl %ecx, %eax > > >> diff --git a/sysdeps/x86_64/multiarch/strncmp-avx2-rtm.S b/sysdeps/x86_64/multiarch/strncmp-avx2-rtm.S > > >> index 37d1224bb9..68bad365ba 100644 > > >> --- a/sysdeps/x86_64/multiarch/strncmp-avx2-rtm.S > > >> +++ b/sysdeps/x86_64/multiarch/strncmp-avx2-rtm.S > > >> @@ -1,3 +1,4 @@ > > >> #define STRCMP __strncmp_avx2_rtm > > >> #define USE_AS_STRNCMP 1 > > >> +#define OVERFLOW_STRCMP __strcmp_avx2_rtm > > >> #include "strcmp-avx2-rtm.S" > > >> diff --git a/sysdeps/x86_64/multiarch/strncmp-avx2.S b/sysdeps/x86_64/multiarch/strncmp-avx2.S > > >> index 1678bcc235..f138e9f1fd 100644 > > >> --- a/sysdeps/x86_64/multiarch/strncmp-avx2.S > > >> +++ b/sysdeps/x86_64/multiarch/strncmp-avx2.S > > >> @@ -1,3 +1,4 @@ > > >> #define STRCMP __strncmp_avx2 > > >> #define USE_AS_STRNCMP 1 > > >> +#define OVERFLOW_STRCMP __strcmp_avx2 > > >> #include "strcmp-avx2.S" > > >> diff --git a/sysdeps/x86_64/multiarch/wcsncmp-avx2-rtm.S b/sysdeps/x86_64/multiarch/wcsncmp-avx2-rtm.S > > >> index 4e88c70cc6..f467582cbe 100644 > > >> --- a/sysdeps/x86_64/multiarch/wcsncmp-avx2-rtm.S > > >> +++ b/sysdeps/x86_64/multiarch/wcsncmp-avx2-rtm.S > > >> @@ -1,5 +1,5 @@ > > >> #define STRCMP __wcsncmp_avx2_rtm > > >> #define USE_AS_STRNCMP 1 > > >> #define USE_AS_WCSCMP 1 > > >> - > > >> +#define OVERFLOW_STRCMP __wcscmp_avx2_rtm > > >> #include "strcmp-avx2-rtm.S" > > >> diff --git a/sysdeps/x86_64/multiarch/wcsncmp-avx2.S b/sysdeps/x86_64/multiarch/wcsncmp-avx2.S > > >> index 4fa1de4d3f..e9ede522b8 100644 > > >> --- a/sysdeps/x86_64/multiarch/wcsncmp-avx2.S > > >> +++ b/sysdeps/x86_64/multiarch/wcsncmp-avx2.S > > >> @@ -1,5 +1,5 @@ > > >> #define STRCMP __wcsncmp_avx2 > > >> #define USE_AS_STRNCMP 1 > > >> #define USE_AS_WCSCMP 1 > > >> - > > >> +#define OVERFLOW_STRCMP __wcscmp_avx2 > > >> #include "strcmp-avx2.S" > > >> -- > > >> 2.25.1 > > >> > > > > > > LGTM. > > > > > > Reviewed-by: H.J. Lu > > > > > > Thanks. > > > > > > > This fails CI for i686. > > https://patchwork.sourceware.org/project/glibc/patch/20220216080935.3284536-1-goldstein.w.n@gmail.com/ > > > > -- > > Cheers, > > Carlos. > > > > Hi Noah, > > We need this change on top of your patch: > > diff --git a/sysdeps/x86/tst-strncmp-rtm.c b/sysdeps/x86/tst-strncmp-rtm.c > index ebc94a3a6d..9e20abaacc 100644 > --- a/sysdeps/x86/tst-strncmp-rtm.c > +++ b/sysdeps/x86/tst-strncmp-rtm.c > @@ -59,6 +59,9 @@ function_overflow (void) > static int > do_test (void) > { > - return (do_test_1 ("strncmp", LOOP, prepare, function) > - || do_test_1 ("strncmp", LOOP, prepare, function_overflow)); > + int status = do_test_1 ("strncmp", LOOP, prepare, function); > + if (status != EXIT_SUCCESS) > + return status; > + status = do_test_1 ("strncmp", LOOP, prepare, function_overflow); > + return status; > } Added your fix in V5. > > -- > H.J.