Message ID | 20200612201056.228614-1-hjl.tools@gmail.com |
---|---|
State | Committed |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A8F9B395B82A; Fri, 12 Jun 2020 20:11:04 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A8F9B395B82A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1591992664; bh=z43lccmwObyJKJGKkf6g7+LZfljeXuT6R1oge5GIb8A=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=otvkOleihEW+Nfn0JFO3KyT38jUBEMMvLveI3HWz8xcRyGfG4YmJ5owEeNdw5zooI cE/UQqrtvLvQPFiajd3WsG4JWC9gJzSz7XuB8lUpgt34I+GxVzxD9CzEFULXwcwu1/ JgU1RUkzXXTUZqXL1cKvwl9ps6XAkgNq0kkF48iQ= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-pj1-x1043.google.com (mail-pj1-x1043.google.com [IPv6:2607:f8b0:4864:20::1043]) by sourceware.org (Postfix) with ESMTPS id DF701395B442 for <libc-alpha@sourceware.org>; Fri, 12 Jun 2020 20:10:59 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org DF701395B442 Received: by mail-pj1-x1043.google.com with SMTP id i4so4317987pjd.0 for <libc-alpha@sourceware.org>; Fri, 12 Jun 2020 13:10:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:subject:date:message-id:mime-version :content-transfer-encoding; bh=z43lccmwObyJKJGKkf6g7+LZfljeXuT6R1oge5GIb8A=; b=YlEqh9vhieEDq93CkkVVRwKn2OqznuRu1IHINVwSp/pU4Og+o17ivu2vq7QCey+DTW SpT/MZ0wshN29c4uIm1UmjfUX6wCmrjsLXGbcCI0BIDpUW8gdhNFHbvXZPJKKHDeHHmG XBbM5nuWWES7VHcaWYn/iWjytu7Nl1YnooITeEqQdYHQGJjYVXDcZ6D/nOU8Ea2k/pn2 NLcTVUqKWi/AzIbSGtyFfr6MGsESCYy0F5Klw9/aZtUWBYRQPHRDATFT7tb1Zy43egzJ EK3qvPst6M6nrQNVFF7br55EmWwmVHtSIayk2Z4TtcknJyS7D9lJLeKVlkgeJUNfQU1+ LqxA== X-Gm-Message-State: AOAM531qpPsE4QHZxExqv+pYc4OOSkqVDT7zp60qC7H5rnxHjKRi+QPd qAJanajG6sjvrdTmU+caweP85+cB X-Google-Smtp-Source: ABdhPJxxu65CULMA8cxt5P6p7PFsTmg+BbuoOMD0sMBDeMHlws8zoNvSEU8jb8KeFMP0QIEy+wM1RA== X-Received: by 2002:a17:90b:19c4:: with SMTP id nm4mr621526pjb.66.1591992658801; Fri, 12 Jun 2020 13:10:58 -0700 (PDT) Received: from gnu-cfl-2.localdomain (c-69-181-90-243.hsd1.ca.comcast.net. [69.181.90.243]) by smtp.gmail.com with ESMTPSA id i12sm7038958pfk.180.2020.06.12.13.10.57 for <libc-alpha@sourceware.org> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Jun 2020 13:10:57 -0700 (PDT) Received: from gnu-cfl-2.localdomain (localhost [IPv6:::1]) by gnu-cfl-2.localdomain (Postfix) with ESMTP id 4B6B41A00D7 for <libc-alpha@sourceware.org>; Fri, 12 Jun 2020 13:10:56 -0700 (PDT) To: libc-alpha@sourceware.org Subject: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933] Date: Fri, 12 Jun 2020 13:10:53 -0700 Message-Id: <20200612201056.228614-1-hjl.tools@gmail.com> X-Mailer: git-send-email 2.26.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <http://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <http://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: "H.J. Lu via Libc-alpha" <libc-alpha@sourceware.org> Reply-To: "H.J. Lu" <hjl.tools@gmail.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
[1/4] strncmp: Add a testcase for page boundary [BZ #25933]
|
|
Commit Message
H.J. Lu
June 12, 2020, 8:10 p.m. UTC
Add a strncmp testcase to cover cases where one of strings ends on the page boundary. --- string/test-strncmp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
Comments
On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote: > Add a strncmp testcase to cover cases where one of strings ends on the > page boundary. > --- > string/test-strncmp.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c > index d961ac4493..d0928a2864 100644 > --- a/string/test-strncmp.c > +++ b/string/test-strncmp.c > @@ -403,6 +403,30 @@ check2 (void) > free (s2); > } > > +static void > +check3 (void) > +{ > + size_t size = 32 * 4; > + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); > + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); > + int exp_result; > + > + memset (s1, 'a', page_size); > + memset (s2, 'a', page_size); > + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; > + > + for (size_t s = 99; s <= size; s++) > + for (size_t s1a = 31; s1a < 32; s1a++) > + for (size_t s2a = 30; s2a < 32; s2a++) > + { > + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; > + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; > + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); > + FOR_EACH_IMPL (impl, 0) > + check_result (impl, s1p, s2p, s, exp_result); > + } > +} There are lots of magic numbers here. Could you add some context around those numbers? If they are meant to reflect a cache line length, then it's appropriate to support different cache line sizes. Rafael Zinsly just did this with strncasecmp in the last week or so. > + > int > test_main (void) > { > @@ -412,6 +436,7 @@ test_main (void) > > check1 (); > check2 (); > + check3 (); > > printf ("%23s", ""); > FOR_EACH_IMPL (impl, 0) > -- > 2.26.2 >
On Mon, Jun 15, 2020 at 1:29 PM Paul A. Clarke <pc@us.ibm.com> wrote: > > On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote: > > Add a strncmp testcase to cover cases where one of strings ends on the > > page boundary. > > --- > > string/test-strncmp.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c > > index d961ac4493..d0928a2864 100644 > > --- a/string/test-strncmp.c > > +++ b/string/test-strncmp.c > > @@ -403,6 +403,30 @@ check2 (void) > > free (s2); > > } > > > > +static void > > +check3 (void) > > +{ > > + size_t size = 32 * 4; > > + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); > > + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); > > + int exp_result; > > + > > + memset (s1, 'a', page_size); > > + memset (s2, 'a', page_size); > > + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; > > + > > + for (size_t s = 99; s <= size; s++) > > + for (size_t s1a = 31; s1a < 32; s1a++) > > + for (size_t s2a = 30; s2a < 32; s2a++) > > + { > > + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; > > + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; > > + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); > > + FOR_EACH_IMPL (impl, 0) > > + check_result (impl, s1p, s2p, s, exp_result); > > + } > > +} > > There are lots of magic numbers here. > > Could you add some context around those number My commit log says --- Add a strncmp testcase to cover cases where one of strings ends on the page boundary. --- > If they are meant to reflect a cache line length, then it's No. My testcase is for correctness, not for performance. > appropriate to support different cache line sizes. > Rafael Zinsly just did this with strncasecmp in the > last week or so. > > > + > > int > > test_main (void) > > { > > @@ -412,6 +436,7 @@ test_main (void) > > > > check1 (); > > check2 (); > > + check3 (); > > > > printf ("%23s", ""); > > FOR_EACH_IMPL (impl, 0) > > -- > > 2.26.2 > >
On Mon, Jun 15, 2020 at 02:34:13PM -0700, H.J. Lu via Libc-alpha wrote: > On Mon, Jun 15, 2020 at 1:29 PM Paul A. Clarke <pc@us.ibm.com> wrote: > > > > On Fri, Jun 12, 2020 at 01:10:53PM -0700, H.J. Lu via Libc-alpha wrote: > > > Add a strncmp testcase to cover cases where one of strings ends on the > > > page boundary. > > > --- > > > string/test-strncmp.c | 25 +++++++++++++++++++++++++ > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c > > > index d961ac4493..d0928a2864 100644 > > > --- a/string/test-strncmp.c > > > +++ b/string/test-strncmp.c > > > @@ -403,6 +403,30 @@ check2 (void) > > > free (s2); > > > } > > > > > > +static void > > > +check3 (void) > > > +{ > > > + size_t size = 32 * 4; > > > + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); > > > + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); > > > + int exp_result; > > > + > > > + memset (s1, 'a', page_size); > > > + memset (s2, 'a', page_size); > > > + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; > > > + > > > + for (size_t s = 99; s <= size; s++) > > > + for (size_t s1a = 31; s1a < 32; s1a++) > > > + for (size_t s2a = 30; s2a < 32; s2a++) > > > + { > > > + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; > > > + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; > > > + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); > > > + FOR_EACH_IMPL (impl, 0) > > > + check_result (impl, s1p, s2p, s, exp_result); > > > + } > > > +} > > > > There are lots of magic numbers here. > > > > Could you add some context around those number > > My commit log says > > --- > Add a strncmp testcase to cover cases where one of strings ends on the > page boundary. > --- Which says nothing about why you need to test over 90000 different cases of a string ending on a page boundary, nor what any of the magic numbers represent. > > If they are meant to reflect a cache line length, then it's > > No. My testcase is for correctness, not for performance. I made no reference to performance. > > appropriate to support different cache line sizes. > > Rafael Zinsly just did this with strncasecmp in the > > last week or so. PC
On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote: > Add a strncmp testcase to cover cases where one of strings ends on the > page boundary. I would like to see this sequence of 4 patches committed because they *do* correctly regression test swbz#25933, and I'd like to see this not regress again. However, I share Paul's concerns over the magic numbers, so I have made some concrete suggestions for comments. OK with the following changes: - Add all suggested comments. - s1a iterates over [30,32). Reviewed-by: Carlos O'Donell <carlos@redhat.com> > --- > string/test-strncmp.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c > index d961ac4493..d0928a2864 100644 > --- a/string/test-strncmp.c > +++ b/string/test-strncmp.c > @@ -403,6 +403,30 @@ check2 (void) > free (s2); > } > > +static void > +check3 (void) > +{ > + size_t size = 32 * 4; Add a comment: /* To trigger bug 25933 we need a size that is equal to the vector length times 4. In the case of AVX2 for Intel we need 32 * 4. We make this test generic and run it for all architectures as additional boundary testing for such related algorithms. */ This is my understanding, that we need 32*4 to trigger the bug. > + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); > + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); We set s1 and s2 to point into the buffers at a point 1 page before the end. We expect the test to add 1 page PROT_NONE at the end. Thus s1 and s2 by default point to two pages, the first page PROT_READ|PROT_WRITE and the second page PROT_NONE. So far so good. No comment required. You have to understand how these test cases work to read this. > + int exp_result; > + > + memset (s1, 'a', page_size); > + memset (s2, 'a', page_size); OK. Fill both full of 'a'. > + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; OK. Null terminate s1. Note that s2 is not null terminated. > + Add comment: /* Iterate over a size that is just below where we expect the bug to trigger up to the size we expect will trigger the bug e.g. [99-128]. Likewise iterate the start of two strings between 30 and 31 bytes away from the boundary to simulate alignment changes. */ > + for (size_t s = 99; s <= size; s++) OK. A bit of belt-and-suspenders here we make s range from 99-128, so we iterate just before the size we care about up to the size we expect to trigger the bug. > + for (size_t s1a = 31; s1a < 32; s1a++) s1a iterates over [31,32) e.g. 31 > + for (size_t s2a = 30; s2a < 32; s2a++) s2a iterates over [30,32) e.g. 30, 31 > + { > + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; Set the pointer back from the PROT_NONE page by "s+s1a" bytes. > + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; Set the pointer back from the PROT_NONE page by "s+s2a" bytes. > + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); Then compare. This code comes from Adhemerval's testing in comment #2, where the test catches a second loop that has similar problems. At most we test [30 sizes]x[1 offset for s1a]x[2 offets for s2a] = 60 tests. Suggest: s1a iterate over [30,32) like s2a for the sake of simplicity. > + FOR_EACH_IMPL (impl, 0) > + check_result (impl, s1p, s2p, s, exp_result); > + } > +} > + > int > test_main (void) > { > @@ -412,6 +436,7 @@ test_main (void) > > check1 (); > check2 (); > + check3 (); OK. > > printf ("%23s", ""); > FOR_EACH_IMPL (impl, 0) >
On Wed, Sep 23, 2020 at 12:01 PM Carlos O'Donell <carlos@redhat.com> wrote: > > On 6/12/20 4:10 PM, H.J. Lu via Libc-alpha wrote: > > Add a strncmp testcase to cover cases where one of strings ends on the > > page boundary. > > I would like to see this sequence of 4 patches committed because they *do* > correctly regression test swbz#25933, and I'd like to see this not regress > again. > > However, I share Paul's concerns over the magic numbers, so I have made > some concrete suggestions for comments. > > OK with the following changes: > - Add all suggested comments. > - s1a iterates over [30,32). > > Reviewed-by: Carlos O'Donell <carlos@redhat.com> > > > --- > > string/test-strncmp.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/string/test-strncmp.c b/string/test-strncmp.c > > index d961ac4493..d0928a2864 100644 > > --- a/string/test-strncmp.c > > +++ b/string/test-strncmp.c > > @@ -403,6 +403,30 @@ check2 (void) > > free (s2); > > } > > > > +static void > > +check3 (void) > > +{ > > + size_t size = 32 * 4; > > Add a comment: > > /* To trigger bug 25933 we need a size that is equal to the > vector length times 4. In the case of AVX2 for Intel we > need 32 * 4. We make this test generic and run it for all > architectures as additional boundary testing for such > related algorithms. */ > > This is my understanding, that we need 32*4 to trigger the bug. > > > + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); > > + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); > > We set s1 and s2 to point into the buffers at a point 1 page > before the end. We expect the test to add 1 page PROT_NONE at > the end. Thus s1 and s2 by default point to two pages, the > first page PROT_READ|PROT_WRITE and the second page PROT_NONE. > So far so good. No comment required. You have to understand > how these test cases work to read this. > > > + int exp_result; > > + > > + memset (s1, 'a', page_size); > > + memset (s2, 'a', page_size); > > OK. Fill both full of 'a'. > > > + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; > > OK. Null terminate s1. > > Note that s2 is not null terminated. > > > + > > Add comment: > > /* Iterate over a size that is just below where we expect > the bug to trigger up to the size we expect will trigger > the bug e.g. [99-128]. Likewise iterate the start of > two strings between 30 and 31 bytes away from the > boundary to simulate alignment changes. */ > > > + for (size_t s = 99; s <= size; s++) > > OK. A bit of belt-and-suspenders here we make s range > from 99-128, so we iterate just before the size we care > about up to the size we expect to trigger the bug. > > > + for (size_t s1a = 31; s1a < 32; s1a++) > > s1a iterates over [31,32) e.g. 31 > > > + for (size_t s2a = 30; s2a < 32; s2a++) > > s2a iterates over [30,32) e.g. 30, 31 > > > + { > > + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; > > Set the pointer back from the PROT_NONE page by "s+s1a" bytes. > > > + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; > > Set the pointer back from the PROT_NONE page by "s+s2a" bytes. > > > + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); > > Then compare. > > This code comes from Adhemerval's testing in comment #2, > where the test catches a second loop that has similar problems. > > At most we test [30 sizes]x[1 offset for s1a]x[2 offets for s2a] = 60 tests. > > Suggest: > > s1a iterate over [30,32) like s2a for the sake of simplicity. > > > + FOR_EACH_IMPL (impl, 0) > > + check_result (impl, s1p, s2p, s, exp_result); > > + } > > +} > > + > > int > > test_main (void) > > { > > @@ -412,6 +436,7 @@ test_main (void) > > > > check1 (); > > check2 (); > > + check3 (); > > OK. > > > > > printf ("%23s", ""); > > FOR_EACH_IMPL (impl, 0) > > > Here is the updated patch I am checking in. Thanks.
diff --git a/string/test-strncmp.c b/string/test-strncmp.c index d961ac4493..d0928a2864 100644 --- a/string/test-strncmp.c +++ b/string/test-strncmp.c @@ -403,6 +403,30 @@ check2 (void) free (s2); } +static void +check3 (void) +{ + size_t size = 32 * 4; + CHAR *s1 = (CHAR *) (buf1 + (BUF1PAGES - 1) * page_size); + CHAR *s2 = (CHAR *) (buf2 + (BUF1PAGES - 1) * page_size); + int exp_result; + + memset (s1, 'a', page_size); + memset (s2, 'a', page_size); + s1[(page_size / CHARBYTES) - 1] = (CHAR) 0; + + for (size_t s = 99; s <= size; s++) + for (size_t s1a = 31; s1a < 32; s1a++) + for (size_t s2a = 30; s2a < 32; s2a++) + { + CHAR *s1p = s1 + (page_size / CHARBYTES - s) - s1a; + CHAR *s2p = s2 + (page_size / CHARBYTES - s) - s2a; + exp_result = SIMPLE_STRNCMP (s1p, s2p, s); + FOR_EACH_IMPL (impl, 0) + check_result (impl, s1p, s2p, s, exp_result); + } +} + int test_main (void) { @@ -412,6 +436,7 @@ test_main (void) check1 (); check2 (); + check3 (); printf ("%23s", ""); FOR_EACH_IMPL (impl, 0)