From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x630.google.com (mail-pl1-x630.google.com [IPv6:2607:f8b0:4864:20::630]) by sourceware.org (Postfix) with ESMTPS id 86A013858C20 for ; Wed, 16 Feb 2022 14:27:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86A013858C20 Received: by mail-pl1-x630.google.com with SMTP id u12so2101858plf.13 for ; Wed, 16 Feb 2022 06:27:29 -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=aKSnYsYWLtuugddRijuJKqhJAUuygWaJN+eIyeSKufQ=; b=cAw35lUBST8o9HaT8jWRg7ErYCd2s4dqAa0VpeVzT19/d0kvxehmup3p8vLEN/5hGN imbOBpMKyCmSuh0IrHrmXhs5VIYAUNEt8TSrYSBV3064b1O8YQ2y0wjkRG1oxha1VAiN t3MecSJJw2vDBTcEXWeTKMZSrZECVpcHuXD++SjOuovwxREdlhRVxvJtGXQePze7PT6n Gy3ixAfAEEwYQC3FcJ+JepwA76CFi7MME02M4n1jB9RNFQ4r4aLTkSgE79eWYHFiG8dF FMTs+ZSoNRU0o4Vm0Xu1/y2ZbAbYo6NSyt9KCjbpJdL9Y1xZl87lg2Z7S03dnxd9rVcy WEIQ== X-Gm-Message-State: AOAM532RO7fUnUY+7wetY2Q6Y6BXWRsGXvHumDFyiZPXYExS1TIttWoQ TPzDDqX2lxOvzmOi+9DGFSDT777/32nhYBnQUDd4hZzgSPI= X-Google-Smtp-Source: ABdhPJwyVUqdwRAt9QR8KlMP8mxwbo8wdd6XPJTZg7bSgyCqUNnD0zuTr/fMV5/Rapj/pCnY9q6WeYDE/UVTBCd/XWA= X-Received: by 2002:a17:90b:4f91:b0:1b9:7dd3:ba70 with SMTP id qe17-20020a17090b4f9100b001b97dd3ba70mr1993381pjb.143.1645021648548; Wed, 16 Feb 2022 06:27:28 -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: <867c131c-cec4-d8d6-5f24-cd51de94f1b2@redhat.com> From: "H.J. Lu" Date: Wed, 16 Feb 2022 06:26:52 -0800 Message-ID: Subject: Re: [PATCH v3] x86: Fallback {str|wcs}cmp RTM in the ncmp overflow case [BZ #28896] To: "Carlos O'Donell" Cc: Noah Goldstein , GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3027.4 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: Wed, 16 Feb 2022 14:27:31 -0000 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; } -- H.J.