From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 87390 invoked by alias); 26 Jul 2018 15:13:59 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 87351 invoked by uid 89); 26 Jul 2018 15:13:58 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: [COMMITTED] Fix string/tst-xbzero-opt if build with gcc head. To: "Carlos O'Donell" , Zack Weinberg Cc: GNU C Library References: <23901f64-d483-f587-6a1e-2af2f5bdecdb@linux.ibm.com> <44c07854-9080-45d5-0a90-398f0b611cc5@redhat.com> From: Stefan Liebler Date: Thu, 26 Jul 2018 15:13:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <44c07854-9080-45d5-0a90-398f0b611cc5@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit x-cbid: 18072615-4275-0000-0000-0000029E612D X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 18072615-4276-0000-0000-000037A6652F Message-Id: <90ca7308-8cb2-0f9c-d6c5-1366aab5dd6b@linux.ibm.com> X-SW-Source: 2018-07/txt/msg00906.txt.bz2 On 07/26/2018 03:33 PM, Carlos O'Donell wrote: > On 07/16/2018 07:05 AM, Stefan Liebler wrote: >> On 07/12/2018 06:42 PM, Zack Weinberg wrote: >>> On Thu, Jul 12, 2018 at 10:22 AM, Stefan Liebler  wrote: >>>> Fix string/tst-xbzero-opt is build with gcc head. >>> ... >>>> In setup_no_clear / setup_ordinary_clear, GCC is omitting the memcpy loop in >>>> prepare_test_buffer. Thus count_test_patterns does not find any of the >>>> test_pattern. >>>> >>>> This patch introduces a compiler barrier just after filling the buffer. >>> >>> I think I understand why the call to swapcontext in >>> prepare_test_buffer is not a sufficient compiler barrier, but I am not >>> a fan of asm volatile ("" ::: "memory"), because I fully expect some >>> future compiler to decide that there are no actual assembly >>> instructions being inserted so the statement can be completely >>> ignored.  I would prefer us to find some kind of construct that >>> actually does make externally-visible side effects depend on the >>> contents of 'buf' in terms of the C abstract machine. >>> >>> zw >>> >> >> Okay. Then here is a new version of the patch without the empty asm. >> If build with GCC head on s390x, setup_no_clear is now filling the >> buffer and setup_ordinary_clear is just a tail call to setup_no_clear >> (same behaviour as before). > > Thanks for fixing this. > >> commit a42ce495317df04f1abdc3670c1fcbb68f329c3d >> Author: Stefan Liebler >> Date: Fri Jul 13 15:58:48 2018 +0200 >> >> Fix string/tst-xbzero-opt if build with gcc head. >> >> On s390x, the test string/tst-xbzero-opt is failing if build with gcc head: >> FAIL: no clear/prepare: expected 32 got 0 >> FAIL: no clear/test: expected some got 0 >> FAIL: ordinary clear/prepare: expected 32 got 0 >> INFO: ordinary clear/test: found 0 patterns (memset not eliminated) >> PASS: explicit clear/prepare: expected 32 got 32 >> PASS: explicit clear/test: expected 0 got 0 >> >> In setup_no_clear / setup_ordinary_clear, GCC is omitting the memcpy loop >> in prepare_test_buffer. Thus count_test_patterns does not find any of the >> test_pattern. >> >> This patch calls use_test_buffer in order to force the compiler to really copy >> the pattern to buf. > > OK. > >> >> ChangeLog: >> >> * string/tst-xbzero-opt.c (use_test_buffer): New function. >> (prepare_test_buffer): Call use_test_buffer as compiler barrier. > > OK for 2.28. Consider noclone also. > > Reviewed-by: Carlos O'Donell > Okay. I've just committed this patch with noclone. Thanks Stefan >> >> diff --git a/string/tst-xbzero-opt.c b/string/tst-xbzero-opt.c >> index cf7041f37a..b00fafd237 100644 >> --- a/string/tst-xbzero-opt.c >> +++ b/string/tst-xbzero-opt.c >> @@ -97,6 +97,17 @@ static const unsigned char test_pattern[16] = >> >> static ucontext_t uc_main, uc_co; >> >> +static __attribute__ ((noinline)) int > > Suggest: Add noclone. > > As optimization barriers we have also used "noclone" here > e.g. malloc/tst-mallocstate.c (my_free). > > I can't see a case where a specialization of this function avoids > the copy, but it might be possible? Just for safety's sake use noclone > too. > >> +use_test_buffer (unsigned char *buf) >> +{ >> + unsigned int sum = 0; >> + >> + for (unsigned int i = 0; i < PATTERN_REPS; i++) >> + sum += buf[i * PATTERN_SIZE]; >> + >> + return (sum == 2 * PATTERN_REPS) ? 0 : 1; >> +} > > OK. > >> + >> /* Always check the test buffer immediately after filling it; this >> makes externally visible side effects depend on the buffer existing >> and having been filled in. */ >> @@ -108,6 +119,10 @@ prepare_test_buffer (unsigned char *buf) >> >> if (swapcontext (&uc_co, &uc_main)) >> abort (); >> + >> + /* Force the compiler to really copy the pattern to buf. */ >> + if (use_test_buffer (buf)) >> + abort (); > > OK. > >> } >> >> static void >