This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: Fix string/tst-xbzero-opt if build with gcc head.
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 <stli@linux.ibm.com> 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 <stli@linux.ibm.com>
> 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 <carlos@redhat.com>
>
> 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