From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id E604D3857C45 for ; Wed, 23 Sep 2020 19:01:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org E604D3857C45 Received: from mail-qv1-f69.google.com (mail-qv1-f69.google.com [209.85.219.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-427-iw3AXs9zPMiKjV6KZjnqFA-1; Wed, 23 Sep 2020 15:01:21 -0400 X-MC-Unique: iw3AXs9zPMiKjV6KZjnqFA-1 Received: by mail-qv1-f69.google.com with SMTP id w2so694881qvr.19 for ; Wed, 23 Sep 2020 12:01:21 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=yhBB5H1qNMh1KIVDL6seGufjOjNqA40gCE8w0DxApUY=; b=O212rXLdgaH4VSwkb8qENSHYumZEyQYDQhzBhRYeqZeacKaTnsUZA9489wQYiwcpfE hnbBjAiP0pRC6RLK5SlpydRqjUyyngR+0EcWBJb3ImzW/C6n7yo1rYjKnp7R6oCceixk aYBvP9epLv+mHXO+t5hXGqFBDTmkLyJhp9bO3tIbZnkhP1rFruPQCZUZmrFze3u48bjy fOkGnJ+MCbYMh79IxG09kuVa2pp6Vz8S5UD7ncxiiZxLVol3bFwKUGgZLY+OJz8rbYE9 O4aWlzaZuF0IW1SYWS1iLu0PUK1RBrrNO/ULwFo7hDlYR8Aa0anJHn4+N64Pnvwfw8Vc bglw== X-Gm-Message-State: AOAM532EuKsEWACF2ACELwygrBWHdRWxP94o8Zxpq2oEctOnA/sjkvmD yvGN8QpwD1Z7RTrO7PfsLOg/3KxBKj2BAc4wHAjnBdDfLUyztLtyYAe/KIKWU/dq2F0jfQ1sZSu H/VG1Cw0sYCWM9a6cM9SV X-Received: by 2002:ac8:4784:: with SMTP id k4mr1640906qtq.266.1600887680609; Wed, 23 Sep 2020 12:01:20 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyMkCadSLqROVI+CGcOyHsizFwEzv6vc2ZC88NuFW+fTo+lWeINgjEFUmz/o2ZNFDXbuMWxxg== X-Received: by 2002:ac8:4784:: with SMTP id k4mr1640872qtq.266.1600887680290; Wed, 23 Sep 2020 12:01:20 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id f13sm482211qko.122.2020.09.23.12.01.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 23 Sep 2020 12:01:18 -0700 (PDT) Subject: Re: [PATCH 1/4] strncmp: Add a testcase for page boundary [BZ #25933] To: "H.J. Lu" , libc-alpha@sourceware.org References: <20200612201056.228614-1-hjl.tools@gmail.com> From: Carlos O'Donell Organization: Red Hat Message-ID: <9a90afd4-5777-1c99-a500-aafceb875087@redhat.com> Date: Wed, 23 Sep 2020 15:01:17 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20200612201056.228614-1-hjl.tools@gmail.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H5, RCVD_IN_MSPIKE_WL, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Sep 2020 19:01:25 -0000 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 > --- > 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) > -- Cheers, Carlos.