From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 74BF33858D33 for ; Mon, 19 Dec 2022 17:50:19 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 74BF33858D33 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-ed1-x52f.google.com with SMTP id m21so3511521edc.3 for ; Mon, 19 Dec 2022 09:50:19 -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=KdDtZ/0uHHd6YPFnRsVFi79o2bo75LWSN8RwYtfLhKU=; b=InissHO1ib35zf2KgkiARi1nazlb3t2YsYatjwVjqr+R+GsgCQHyLI5pFaFj7D3Mzd 5M4bb+UIVZwV3jpMI+FUUOA5Bw/UR+hCVlaCg51oQ5LiCbLVchyUKdKiYYgyGCZcvfv4 10G8a0yXfxBRAZ/lWv7BZqc0tyxRqFIHkT+RnLjiR8hx0+C+n9V7NHZjM4XsakD0OAx2 1NGOxmNM2sx7LwaQurUCoLw0SyuNtTnuHp6t0hnNsQKKWQBiMeyv4tJ2ZTtMya0okUUT UKC8I8W+wOmOrWT9QLrb8JxbyLmytCzt7AKY0kjcE+N58Q5JQRNzAc1ej0tBCwy0ZmJk mmJQ== 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=KdDtZ/0uHHd6YPFnRsVFi79o2bo75LWSN8RwYtfLhKU=; b=efTLMWkMgZyk4Q5I+WzHaom2O4eLZ8gTltIUW9JYnP+doDfIh0O9BjUtjuS3udW6Gb e+Wti7P0g9fKReGCgemGZIb8V7VW7jP3wIACigCi1MeHEFbv99+XovllexkrZrztxGam AYDVdzhdPK8VqpIa46GBiGvSl6xzp8wE0BRvEFQhPDuoJ5/2RrgzXB4PmWFH3JlsEkj9 NAuZ3blE4240+skySpCO0xpqD0g7GuKdqDlskDqpWmS8kHF7Z/bp5BaiQl6zui7asIth 4eEla0+o17bdi0YpBa/5mq99OFAZhieDJhAyXVTR38lGrlkiFL/Ul/lBbMeIYZq7oYYh XXkw== X-Gm-Message-State: ANoB5pkqMMLjXCv30RKCgE5pDKZtKWPri1UeAvocgoZ45Ud3kGmyebfU OEue2BbK0knUnqBFc7NuGhZ3GFbt4Ewyt289aYqO+QPv X-Google-Smtp-Source: AA0mqf7Q1TQjmuzzptk90unyckRboMl/K+CPH5QwR5jZeGwfGlUF1UMVeDDwMYV9qr7T6krHZrBOLrZrz86Gnn+3ccA= X-Received: by 2002:a05:6402:3711:b0:461:b6a9:c5cb with SMTP id ek17-20020a056402371100b00461b6a9c5cbmr75547203edb.148.1671472218086; Mon, 19 Dec 2022 09:50:18 -0800 (PST) MIME-Version: 1.0 References: <20221214213053.4174989-1-hjl.tools@gmail.com> In-Reply-To: From: Noah Goldstein Date: Mon, 19 Dec 2022 09:50:06 -0800 Message-ID: Subject: Re: [PATCH v5] x86: Add a testcase for BZ #29863 To: "H.J. Lu" Cc: libc-alpha@sourceware.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-9.4 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 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. > > > > > + 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.