From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2d.google.com (mail-oa1-x2d.google.com [IPv6:2001:4860:4864:20::2d]) by sourceware.org (Postfix) with ESMTPS id 07CD13858D28 for ; Mon, 19 Dec 2022 18:05:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 07CD13858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oa1-x2d.google.com with SMTP id 586e51a60fabf-1322d768ba7so12494452fac.5 for ; Mon, 19 Dec 2022 10:05:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=Qh1J2oXqTRMTrfJBaFFUX7HBEZd3OwURpUaD4tMTThE=; b=YscvZRTUEdK7fxpjirJUrjGqbIh48azlv/0eBUJ5ZWvvkXHnNW5pNcjZK0kzLxKLTz WM01rA1KpFUZYTl+O5YDDv/dagZlg0ZizPUXpRlBpQSYRERxgGkmhdMb3D0LVe/iMhdB TbS/9K07A93mYLDXkTd70EwuVLo3WaM3JGLgOG1Mmxr8qdXDQticE7iYVF0Z60DFOPPJ YM8nkxzD4zuRq9/GuoeE1+mbdX1a4FQ+k76KYklhKA7mCr+jrc5pXCCTxwnYgQ6NGQlk zfmPMHhs/3clVtpRalYInZrcYYQjfb5P88EijCHAQOkfgQz5B1BzK0cZybJkB1EO8/L4 Kprw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Qh1J2oXqTRMTrfJBaFFUX7HBEZd3OwURpUaD4tMTThE=; b=jsZ8Lyt6h2/BpdUneVkjcRjZt9Z76jOTLzQGnmbeMnlzdogvaeX/E6yjR92O4ckgwQ lc9ZzCcqlfWcAKb4wsTOld28JD/GCjDjEJ46YbxpiUaDxsVWJjlKRjyBYKT2gdWg+8qw tk3g/6SOt/CtE1+4Rdl51dhj0o0cn/a6JPed/3vtjDGfXgXn6bVDlhgKLcQXUn7+HFgL OIcFdgR991nLiB8kr6MHKcLURRoacwZ1prWNq/uB6d2xIBtm1b4003GlgHNEK8q681OZ NyNwz7i32RirorlXHW44TtCarNw0JMh46cUB5FjkV2Ek5SaXAYV43ieANw/7eBeR883o smWg== X-Gm-Message-State: AFqh2kqeBSVkHdyMbFoYi2tLNEkH9SWWQsbNFnusjkOYR9qn1Hbw9KPF hADyG2/47Kr6y3Wn4NXcM34oZI6r3/JRCe91LnY2ho87 X-Google-Smtp-Source: AMrXdXvlqgtwdbL2H8njJMgPZPggLs24UGldNtuSg47sGhx6eYlVax4WmPoaF+INdXv/1h0RyPnnIC91NLHPjPVfFUU= X-Received: by 2002:a05:6870:8087:b0:148:2d85:b411 with SMTP id q7-20020a056870808700b001482d85b411mr1924787oab.266.1671473102215; Mon, 19 Dec 2022 10:05:02 -0800 (PST) MIME-Version: 1.0 References: <20221214213053.4174989-1-hjl.tools@gmail.com> In-Reply-To: From: "H.J. Lu" Date: Mon, 19 Dec 2022 10:04:26 -0800 Message-ID: Subject: Re: [PATCH v5] x86: Add a testcase for BZ #29863 To: Noah Goldstein Cc: libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3021.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_NUMSUBJECT,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Mon, Dec 19, 2022 at 9:50 AM Noah Goldstein wrote: > > On Mon, Dec 19, 2022 at 8:56 AM H.J. Lu wrote: > > > > On Mon, Dec 19, 2022 at 6:39 AM Noah Goldstein wrote: > > > > > > On Wed, Dec 14, 2022 at 1:30 PM H.J. Lu wrote: > > > > > > > > When a thread is updating the memory area of memcmp and another thread > > > > is doing memcmp, the memcmp return value is undefined. Add a testcase > > > > to verify that memcmp won't segfault in this case. > > > > --- > > > > sysdeps/x86/Makefile | 8 +++ > > > > sysdeps/x86/tst-memcmp-race.c | 106 ++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 114 insertions(+) > > > > create mode 100644 sysdeps/x86/tst-memcmp-race.c > > > > > > > > diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile > > > > index 56fd5fc805..c3f05a06ab 100644 > > > > --- a/sysdeps/x86/Makefile > > > > +++ b/sysdeps/x86/Makefile > > > > @@ -257,3 +257,11 @@ tests += \ > > > > tests-static += \ > > > > tst-sysconf-cache-linesize-static > > > > endif > > > > + > > > > +ifeq ($(subdir),nptl) > > > > +tests += \ > > > > + tst-memcmp-race \ > > > > +# tests > > > > + > > > > +CFLAGS-tst-memcmp-race.c += -O0 > > > > +endif > > > > diff --git a/sysdeps/x86/tst-memcmp-race.c b/sysdeps/x86/tst-memcmp-race.c > > > > new file mode 100644 > > > > index 0000000000..eefa9e43c4 > > > > --- /dev/null > > > > +++ b/sysdeps/x86/tst-memcmp-race.c > > > > @@ -0,0 +1,106 @@ > > > > +/* Test case for memcmp with race condition. > > > > + Copyright (C) 2022 Free Software Foundation, Inc. > > > > + This file is part of the GNU C Library. > > > > + > > > > + The GNU C Library is free software; you can redistribute it and/or > > > > + modify it under the terms of the GNU Lesser General Public > > > > + License as published by the Free Software Foundation; either > > > > + version 2.1 of the License, or (at your option) any later version. > > > > + > > > > + The GNU C Library is distributed in the hope that it will be useful, > > > > + but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > > > > + Lesser General Public License for more details. > > > > + > > > > + You should have received a copy of the GNU Lesser General Public > > > > + License along with the GNU C Library; if not, see > > > > + . */ > > > > + > > > > +/* Verify that there is no segfault when one thread is updating the > > > > + memory block of memcmp and the other thread is doing memcmp. */ > > > > + > > > > +#define TEST_MAIN > > > > +#define TEST_NAME "memcmp" > > > > + > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#define NUM_THREADS 2 > > > > +#define LOOP1 10000 > > > > +#define LOOP2 1000000 > > > > + > > > > +typedef int (*proto_t) (const void *, const void *, size_t); > > > > + > > > > +IMPL (memcmp, 1) > > > > + > > > > +struct arg > > > > +{ > > > > + proto_t func; > > > > + size_t len; > > > > + int i; > > > > +}; > > > > + > > > > +static void * > > > > +childThread (void *tArgs) > > > > +{ > > > > + struct arg *a = (struct arg *) tArgs; > > > > + int i; > > > > + > > > > + if (0 == a->i % 2) > > > > + { > > > > + for (i = 0; i < LOOP1; i++) > > > > + { > > > > + int result = a->func (buf1, buf2, a->len); > > > > > > So the test can catch more subtle errors can we test > > > with memcmp args at begining and end of a page. > > > > > > i.e buf1/buf2 and (buf1 + page_size - a->len)/(buf2 + page_size - a->len). > > > > I'd like to keep it as simple as possible to cover this > > particular race condition. Make it more complex may > > not provide additional coverage for race conditions. > > > > Think the idea is since we want to make this test generic, > it should provide actual coverage, not just test for the exact > bug we had in memcmp-sse2. > > For example one of the toy impls brough up: > > for (i = 0; i + 4; i < n; i += 4) { > u32 a32 = *(u32 *)(a + i); > u32 b32 = *(u32 *)(b + i); > if(a32 != b32) { > for(; a[i] - b[i] == 0; ++i); > return a[i] - b[i]; > } > } > > might overread just a few bytes and we probably > want to at least be aware of that. We need to verify that it triggers the segfault without the fix. > > > > > > > > + printf ("i = %d : result = %d\n", i, result); > > > > + } > > > > + } > > > > + else > > > > + { > > > > + size_t offset = a->len > 16 ? a->len - 16 : 1; > > > > + for (i = 0; i < LOOP2; i++) > > > > + buf1[offset] = i & 1; > > > > + } > > > > + > > > > + return NULL; > > > > +} > > > > + > > > > +static void > > > > +do_one_test (proto_t func, size_t len) > > > > +{ > > > > + int i; > > > > + pthread_t threads[NUM_THREADS]; > > > > + struct arg a[NUM_THREADS]; > > > > + > > > > + for (i = 0; i < NUM_THREADS; ++i) > > > > + { > > > > + a[i].func = func; > > > > + a[i].len = len; > > > > + a[i].i = i; > > > > + threads[i] = xpthread_create (NULL, childThread, (void *)&a[i]); > > > > + } > > > > + > > > > + for (i = 0; i < NUM_THREADS; ++i) > > > > + xpthread_join (threads[i]); > > > > +} > > > > + > > > > +int > > > > +test_main (void) > > > > +{ > > > > + test_init (); > > > > + > > > > + memset (buf1, 1, page_size); > > > > + memset (buf2, 1, page_size); > > > > + > > > > + for (size_t i = 0; i <= 11; i++) > > > > + { > > > > + FOR_EACH_IMPL (impl, 0) > > > > + do_one_test ((proto_t) impl->fn, (1 << i) + 1); > > > can you also test 2^i - 1. > > > > Will add it. > > > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +#include > > > > -- > > > > 2.38.1 > > > > > > > > > > Can you move this to the string directory (so we run this on all > > > arch) and add corresponding tests for memcmpeq/wmemcmp. > > > > I can move it to nptl since it is a threaded test. I will take a look > > at memcmpeq and wmemcmp. > > > > -- > > H.J. -- H.J.