From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x1036.google.com (mail-pj1-x1036.google.com [IPv6:2607:f8b0:4864:20::1036]) by sourceware.org (Postfix) with ESMTPS id D33D33858C83 for ; Tue, 15 Feb 2022 17:41:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D33D33858C83 Received: by mail-pj1-x1036.google.com with SMTP id ki18-20020a17090ae91200b001b8be87e9abso2281367pjb.1 for ; Tue, 15 Feb 2022 09:41:16 -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=H+BrfoWGTnL3ICX/Ra0T3OlKqFKRvZnoDX4TZHPQNYY=; b=NG2pLRAdO7xE5mTZhe7EiyEdk/PtAqG3rcmEYryf6Ae4+8joUYlu7nlBVne3AgJ2Ol qEKTDboNh0fVAJhJVrJ5q68JV/ya1SHHp/inRKrSEEy7OYoYMPheJ2dvFCMXg1lgRsBj y/aD5X4Zka6G+BunGEzv5fqE8mrxkjP+Ynh25uBgrVITf2gqTcLLzNiwWiAkcc7cexuV AeayTXFO/4i1rrlw5qU+FSWnC/gjuTIPTLFOEAwOYXkO7t4CFQX41PRCkUxkVY+94wvh 7Nw/CJfI8TDRQ936T2t1kKRQkZaMOPLhsuahf/mg2ZOp7NPoFwIvk2n98aTj+xAtMHW+ SXIQ== X-Gm-Message-State: AOAM5315gpyDWTCj4IJkSpF4qj40wOEaxoFG/lRb+QSL2ALHIP7MafL0 idyNvRZrNUyak4c9yYlSn5WPWYSFXIig7e5srQc= X-Google-Smtp-Source: ABdhPJzt7XZyd7EBRNvUzchOx5CmR/txuB8LknccrPwm4D1Q4TWHzt+jF90PkfmAXY/1BWBhBkruMiI6jZFHcMM7XeA= X-Received: by 2002:a17:902:d48b:: with SMTP id c11mr5068502plg.109.1644946875917; Tue, 15 Feb 2022 09:41:15 -0800 (PST) MIME-Version: 1.0 References: <20220214014037.2422450-1-goldstein.w.n@gmail.com> <068fb737-d440-4d73-a1c2-a8daab41c25d@gotplt.org> <6040a978-abaa-777f-fc57-9284e456701c@gotplt.org> In-Reply-To: <6040a978-abaa-777f-fc57-9284e456701c@gotplt.org> From: Noah Goldstein Date: Tue, 15 Feb 2022 11:41:05 -0600 Message-ID: Subject: Re: [PATCH v1] String: Strength memset tests in test-memset.c To: Siddhesh Poyarekar Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.7 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, 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: Tue, 15 Feb 2022 17:41:20 -0000 On Tue, Feb 15, 2022 at 8:01 AM Siddhesh Poyarekar wrote: > > On 15/02/2022 19:25, Noah Goldstein wrote: > > On Tue, Feb 15, 2022 at 7:52 AM Siddhesh Poyarekar wrote: > >> > >> On 14/02/2022 07:10, Noah Goldstein via Libc-alpha wrote: > >>> The prior sentinel logic was broken and was checking the SIMPLE_MEMSET > >>> as opposed to the tested implementation. As well `s` (the test buffer) > >>> was not reset between implementation tests so it was possible for a > >>> buggy implementation to be hidden by a previously executed correct > >>> one. > >>> --- > >>> string/test-memset.c | 36 +++++++++++++++++++++--------------- > >>> 1 file changed, 21 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/string/test-memset.c b/string/test-memset.c > >>> index 8498b1bc97..ee548f6924 100644 > >>> --- a/string/test-memset.c > >>> +++ b/string/test-memset.c > >>> @@ -106,26 +106,28 @@ SIMPLE_MEMSET (CHAR *s, int c, size_t n) > >>> } > >>> > >>> static void > >>> -do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n) > >>> +do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n, int space_below, int space_above) > >>> { > >>> - CHAR buf[n + 2]; > >>> - CHAR *tstbuf = buf + 1; > >>> - CHAR sentinel = c - 1; > >>> - buf[0] = sentinel; > >>> - buf[n + 1] = sentinel; > >>> + CHAR buf[n]; > >>> + CHAR sentinel = ~c; > >>> + if (space_below) > >>> + s[-1] = sentinel; > >>> + if (space_above) > >>> + s[n] = sentinel; > >>> + SIMPLE_MEMSET(s, ~c, n); > >> > >> Setting s with ~c... > >> > >>> #ifdef TEST_BZERO > >>> - simple_bzero (tstbuf, n); > >>> + simple_bzero (buf, n); > >>> CALL (impl, s, n); > >>> - if (memcmp (s, tstbuf, n) != 0 > >>> - || buf[0] != sentinel > >>> - || buf[n + 1] != sentinel) > >>> + if (memcmp (s, buf, n) != 0 > >>> + || (space_below && s[-1] != sentinel) > >>> + || (space_above && s[n] != sentinel)) > >>> #else > >>> CHAR *res = CALL (impl, s, c, n); > >> > >> ... and then overwriting it with c... > >> > >>> if (res != s > >>> - || SIMPLE_MEMSET (tstbuf, c, n) != tstbuf > >>> - || MEMCMP (s, tstbuf, n) != 0 > >>> - || buf[0] != sentinel > >>> - || buf[n + 1] != sentinel) > >>> + || SIMPLE_MEMSET (buf, c, n) != buf > >>> + || MEMCMP (s, buf, n) != 0 > >> > >> ... which should then equate to buf since it is also set to c. OK. > >> > >>> + || (space_below && s[-1] != sentinel) > >>> + || (space_above && s[n] != sentinel)) > >>> #endif /* !TEST_BZERO */ > >>> { > >>> error (0, 0, "Wrong result in function %s", impl->name); > >>> @@ -137,12 +139,16 @@ do_one_test (impl_t *impl, CHAR *s, int c __attribute ((unused)), size_t n) > >>> static void > >>> do_test (size_t align, int c, size_t len) > >>> { > >>> + int space_below, space_above; > >>> align &= 4095; > >>> if ((align + len) * sizeof (CHAR) > page_size) > >>> return; > >>> > >>> + space_below = !!align; > >>> + space_above = !((align + len + 1) * sizeof (CHAR) > page_size); > >>> + > >>> FOR_EACH_IMPL (impl, 0) > >>> - do_one_test (impl, (CHAR *) (buf1) + align, c, len); > >>> + do_one_test (impl, (CHAR *) (buf1) + align, c, len, space_below, space_above); > >> > >> This would reduce test coverage, testing underflow only in cases where > >> align is non-zero. Couldn't this be fixed without this coverage reduction? > > > > It doesn't reduce test coverage. As the commit message says the previous > > sentinel logic was working on `tstbuf` so for all intents and purposes we were > > entirely missing it before. > > > > I think also if align == 0 we are at the beginning of a page and will segfault > > if we have underflow. > > Of course, I had a memory of buf1 and buf2 being sub-buffers of a larger > buffer, but I clearly misremembered. > > LGTM. > > Reviewed-by: Siddhesh Poyarekar Thanks pushed. >