[PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Florian Weimer]

Some targets fail to apply dead store elimination to the
memset call in setup_ordinary_clear.  Before this commit,
this causes the test case to fail.  Instead, the test case
now logs lack of memset elimination as an informational
message.

2016-12-20  Florian Weimer  <fweimer@redhat.com>

	Do not require memset elimination in explicit_bzero test.
	* string/tst-xbzero-opt.c (prepare_test_buffer): Force inlining.
	(enum test_expectation): Add NO_EXPECTATIONS.
	(subtests): NO_EXPECTATIONS for ordinary clear.
	(check_test_buffer): Handle NO_EXPECTATIONS.
	* string/Makefile (CFLAGS-tst-xbzero-opt.c): Compile with -O3.

diff --git a/string/Makefile b/string/Makefile
index 9a8b46d..0816277 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -71,6 +71,7 @@ CFLAGS-tst-strlen.c = -fno-builtin
 CFLAGS-stratcliff.c = -fno-builtin
 CFLAGS-test-ffs.c = -fno-builtin
 CFLAGS-tst-inlcall.c = -fno-builtin
+CFLAGS-tst-xbzero-opt.c = -O3
 
 ifeq ($(run-built-tests),yes)
 $(objpfx)tst-svc-cmp.out: tst-svc.expect $(objpfx)tst-svc.out
diff --git a/string/tst-xbzero-opt.c b/string/tst-xbzero-opt.c
index 312857e..3fcaf28 100644
--- a/string/tst-xbzero-opt.c
+++ b/string/tst-xbzero-opt.c
@@ -88,19 +88,19 @@ static const unsigned char test_pattern[16] =
    between preparing it and letting it go out of scope, and we expect
    to find it.  This confirms that the test buffer does get filled in
    and we can find it from the stack buffer.  In the "ordinary_clear"
-   case, we clear it using memset, and we expect to find it.  This
-   confirms that the compiler can optimize out block clears in this
-   context; if it can't, the real test might be succeeding for the
-   wrong reason.  Finally, the "explicit_clear" case uses
-   explicit_bzero and expects _not_ to find the test buffer, which is
-   the real test.  */
+   case, we clear it using memset.  Depending on the target, the
+   compiler may not be able to apply dead store elimination to the
+   memset call, so the test does not fail if the memset is not
+   eliminated.  Finally, the "explicit_clear" case uses explicit_bzero
+   and expects _not_ to find the test buffer, which is the real
+   test.  */
 
 static ucontext_t uc_main, uc_co;
 
 /* 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.  */
-static void
+static inline __attribute__  ((always_inline)) void
 prepare_test_buffer (unsigned char *buf)
 {
   for (unsigned int i = 0; i < PATTERN_REPS; i++)
@@ -133,7 +133,10 @@ setup_explicit_clear (void)
   explicit_bzero (buf, TEST_BUFFER_SIZE);
 }
 
-enum test_expectation { EXPECT_NONE, EXPECT_SOME, EXPECT_ALL };
+enum test_expectation
+  {
+    EXPECT_NONE, EXPECT_SOME, EXPECT_ALL, NO_EXPECTATIONS
+  };
 struct subtest
 {
   void (*setup_subtest) (void);
@@ -145,7 +148,9 @@ static const struct subtest *cur_subtest;
 static const struct subtest subtests[] =
 {
   { setup_no_clear,       "no clear",       EXPECT_SOME },
-  { setup_ordinary_clear, "ordinary clear", EXPECT_SOME },
+  /* The memset may happen or not, depending on compiler
+     optimizations.  */
+  { setup_ordinary_clear, "ordinary clear", NO_EXPECTATIONS },
   { setup_explicit_clear, "explicit clear", EXPECT_NONE },
   { 0,                    0,                -1          }
 };
@@ -225,6 +230,11 @@ check_test_buffer (enum test_expectation expected,
 	}
       break;
 
+    case NO_EXPECTATIONS:
+      printf ("INFO: %s/%s: found %d patterns%s\n", label, stage, cnt,
+	      cnt == 0 ? " (memset not eliminated)" : "");
+      break;
+
     default:
       printf ("ERROR: %s/%s: invalid value for 'expected' = %d\n",
 	      label, stage, (int)expected);

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Florian Weimer]

On 12/20/2016 11:09 AM, Florian Weimer wrote:
> Some targets fail to apply dead store elimination to the
> memset call in setup_ordinary_clear.  Before this commit,
> this causes the test case to fail.  Instead, the test case
> now logs lack of memset elimination as an informational
> message.
>
> 2016-12-20  Florian Weimer  <fweimer@redhat.com>
>
> 	Do not require memset elimination in explicit_bzero test.
> 	* string/tst-xbzero-opt.c (prepare_test_buffer): Force inlining.
> 	(enum test_expectation): Add NO_EXPECTATIONS.
> 	(subtests): NO_EXPECTATIONS for ordinary clear.
> 	(check_test_buffer): Handle NO_EXPECTATIONS.
> 	* string/Makefile (CFLAGS-tst-xbzero-opt.c): Compile with -O3.

Stefan, this test still fails for me on s390x:

PASS: no clear/prepare: expected 32 got 32
PASS: no clear/test: expected some got 32
PASS: ordinary clear/prepare: expected 32 got 32
INFO: ordinary clear/test: found 0 patterns (memset not eliminated)
PASS: explicit clear/prepare: expected 32 got 32
FAIL: explicit clear/test: expected 0 got 1

Do you have an idea what's going on there?

Thanks,
Florian

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Florian Weimer]

On 12/21/2016 07:04 PM, Florian Weimer wrote:
> On 12/20/2016 11:09 AM, Florian Weimer wrote:
>> Some targets fail to apply dead store elimination to the
>> memset call in setup_ordinary_clear.  Before this commit,
>> this causes the test case to fail.  Instead, the test case
>> now logs lack of memset elimination as an informational
>> message.
>>
>> 2016-12-20  Florian Weimer  <fweimer@redhat.com>
>>
>>     Do not require memset elimination in explicit_bzero test.
>>     * string/tst-xbzero-opt.c (prepare_test_buffer): Force inlining.
>>     (enum test_expectation): Add NO_EXPECTATIONS.
>>     (subtests): NO_EXPECTATIONS for ordinary clear.
>>     (check_test_buffer): Handle NO_EXPECTATIONS.
>>     * string/Makefile (CFLAGS-tst-xbzero-opt.c): Compile with -O3.
>
> Stefan, this test still fails for me on s390x:
>
> PASS: no clear/prepare: expected 32 got 32
> PASS: no clear/test: expected some got 32
> PASS: ordinary clear/prepare: expected 32 got 32
> INFO: ordinary clear/test: found 0 patterns (memset not eliminated)
> PASS: explicit clear/prepare: expected 32 got 32
> FAIL: explicit clear/test: expected 0 got 1
>
> Do you have an idea what's going on there?

I filed bug 21006 and will add it as a release blocker.

Thanks,
Florian

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Stefan Liebler]

[-- Attachment #1: Type: text/plain, Size: 2225 bytes --]

On 12/30/2016 01:16 PM, Florian Weimer wrote:
> On 12/21/2016 07:04 PM, Florian Weimer wrote:
>> On 12/20/2016 11:09 AM, Florian Weimer wrote:
>>> Some targets fail to apply dead store elimination to the
>>> memset call in setup_ordinary_clear.  Before this commit,
>>> this causes the test case to fail.  Instead, the test case
>>> now logs lack of memset elimination as an informational
>>> message.
>>>
>>> 2016-12-20  Florian Weimer  <fweimer@redhat.com>
>>>
>>>     Do not require memset elimination in explicit_bzero test.
>>>     * string/tst-xbzero-opt.c (prepare_test_buffer): Force inlining.
>>>     (enum test_expectation): Add NO_EXPECTATIONS.
>>>     (subtests): NO_EXPECTATIONS for ordinary clear.
>>>     (check_test_buffer): Handle NO_EXPECTATIONS.
>>>     * string/Makefile (CFLAGS-tst-xbzero-opt.c): Compile with -O3.
>>
>> Stefan, this test still fails for me on s390x:
>>
>> PASS: no clear/prepare: expected 32 got 32
>> PASS: no clear/test: expected some got 32
>> PASS: ordinary clear/prepare: expected 32 got 32
>> INFO: ordinary clear/test: found 0 patterns (memset not eliminated)
>> PASS: explicit clear/prepare: expected 32 got 32
>> FAIL: explicit clear/test: expected 0 got 1
>>
>> Do you have an idea what's going on there?
>
> I filed bug 21006 and will add it as a release blocker.
>
> Thanks,
> Florian
>
Hi Florian,

the test is also failing on my system and I've had a look into it.

In setup_explicit_clear, the buffer is filled with the test_pattern.
On s390x the memcpy in prepare_test_buffer is done by loading
r4 / r5 with the test_pattern and using store multiple instruction
to store r4 / r5 to buf.
If explicit_bzero is resolved in setup_explicit_clear, r4 / r5 is
stored to stack by _dl_runtime_resolve and the call to memmem in
count_test_patterns finds a hit of the test_pattern on the stack.

The attached patch resolves all symbols at program startup by linking
with -z now.  This omits the call of _dl_runtime_resolve within
setup_explicit_clear and the test passes.

If this is okay, I'll commit this patch and clear this bug in the 
release blockers list in the release-wiki.

Bye
Stefan

ChangeLog:

	[BZ #21006]
	* string/Makefile (LDFLAGS-tst-xbzero-opt): New variable.


[-- Attachment #2: 20170110_string_tst_xbzero_opt.patch --]
[-- Type: text/x-patch, Size: 1910 bytes --]

commit a07f447fd235d1898a56af7317a4ff6a11a603b9
Author: Stefan Liebler <stli@linux.vnet.ibm.com>
Date:   Tue Jan 10 08:47:13 2017 +0100

    S390: Fix FAIL in test string/tst-xbzero-opt [BZ #21006]
    
    On s390x this test failed with:
    FAIL: explicit clear/test: expected 0 got 1
    
    In setup_explicit_clear, the buffer is filled with the test_pattern.
    On s390x the memcpy in prepare_test_buffer is done by loading
    r4 / r5 with the test_pattern and using store multiple instruction
    to store r4 / r5 to buf.
    If explicit_bzero is resolved in setup_explicit_clear, r4 / r5 is
    stored to stack by _dl_runtime_resolve and the call to memmem in
    count_test_patterns finds a hit of the test_pattern on the stack.
    
    This patch resolves all symbols at program startup by linking with
    -z now.  This omits the call of _dl_runtime_resolve within
    setup_explicit_clear and the test passes.
    
    ChangeLog:
    
    	[BZ #21006]
    	* string/Makefile (LDFLAGS-tst-xbzero-opt): New variable.

diff --git a/string/Makefile b/string/Makefile
index 04e9da9..87e0d1d 100644
--- a/string/Makefile
+++ b/string/Makefile
@@ -73,6 +73,14 @@ CFLAGS-stratcliff.c = -fno-builtin
 CFLAGS-test-ffs.c = -fno-builtin
 CFLAGS-tst-inlcall.c = -fno-builtin
 CFLAGS-tst-xbzero-opt.c = -O3
+# BZ 21006: Resolve all functions but at least explicit_bzero at startup.
+# Otherwise the test fails on s390x as the memcpy in prepare_test_buffer is
+# done by loading r4 / r5 with the test_pattern and using store multiple
+# instruction to store r4 / r5 to buf.  If explicit_bzero would be resolved in
+# setup_explicit_clear, r4 / r5 would be stored to stack by _dl_runtime_resolve
+# and the call to memmem in count_test_patterns will find a hit of the
+# test_pattern on the stack.
+LDFLAGS-tst-xbzero-opt = -z now
 
 # Called during TLS initialization.
 CFLAGS-memcpy.c = $(no-stack-protector)

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Stefan Liebler]

On 01/10/2017 09:22 AM, Stefan Liebler wrote:
> On 12/30/2016 01:16 PM, Florian Weimer wrote:
>> On 12/21/2016 07:04 PM, Florian Weimer wrote:
>>> On 12/20/2016 11:09 AM, Florian Weimer wrote:
>>>> Some targets fail to apply dead store elimination to the
>>>> memset call in setup_ordinary_clear.  Before this commit,
>>>> this causes the test case to fail.  Instead, the test case
>>>> now logs lack of memset elimination as an informational
>>>> message.
>>>>
>>>> 2016-12-20  Florian Weimer  <fweimer@redhat.com>
>>>>
>>>>     Do not require memset elimination in explicit_bzero test.
>>>>     * string/tst-xbzero-opt.c (prepare_test_buffer): Force inlining.
>>>>     (enum test_expectation): Add NO_EXPECTATIONS.
>>>>     (subtests): NO_EXPECTATIONS for ordinary clear.
>>>>     (check_test_buffer): Handle NO_EXPECTATIONS.
>>>>     * string/Makefile (CFLAGS-tst-xbzero-opt.c): Compile with -O3.
>>>
>>> Stefan, this test still fails for me on s390x:
>>>
>>> PASS: no clear/prepare: expected 32 got 32
>>> PASS: no clear/test: expected some got 32
>>> PASS: ordinary clear/prepare: expected 32 got 32
>>> INFO: ordinary clear/test: found 0 patterns (memset not eliminated)
>>> PASS: explicit clear/prepare: expected 32 got 32
>>> FAIL: explicit clear/test: expected 0 got 1
>>>
>>> Do you have an idea what's going on there?
>>
>> I filed bug 21006 and will add it as a release blocker.
>>
>> Thanks,
>> Florian
>>
> Hi Florian,
>
> the test is also failing on my system and I've had a look into it.
>
> In setup_explicit_clear, the buffer is filled with the test_pattern.
> On s390x the memcpy in prepare_test_buffer is done by loading
> r4 / r5 with the test_pattern and using store multiple instruction
> to store r4 / r5 to buf.
> If explicit_bzero is resolved in setup_explicit_clear, r4 / r5 is
> stored to stack by _dl_runtime_resolve and the call to memmem in
> count_test_patterns finds a hit of the test_pattern on the stack.
>
> The attached patch resolves all symbols at program startup by linking
> with -z now.  This omits the call of _dl_runtime_resolve within
> setup_explicit_clear and the test passes.
>
> If this is okay, I'll commit this patch and clear this bug in the
> release blockers list in the release-wiki.
>
> Bye
> Stefan
>
> ChangeLog:
>
>     [BZ #21006]
>     * string/Makefile (LDFLAGS-tst-xbzero-opt): New variable.
>
PING

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Zack Weinberg]

On Mon, Jan 16, 2017 at 3:24 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
> On 01/10/2017 09:22 AM, Stefan Liebler wrote:
>>
>> In setup_explicit_clear, the buffer is filled with the test_pattern.
>> On s390x the memcpy in prepare_test_buffer is done by loading
>> r4 / r5 with the test_pattern and using store multiple instruction
>> to store r4 / r5 to buf.
>> If explicit_bzero is resolved in setup_explicit_clear, r4 / r5 is
>> stored to stack by _dl_runtime_resolve and the call to memmem in
>> count_test_patterns finds a hit of the test_pattern on the stack.
>>
>> The attached patch resolves all symbols at program startup by linking
>> with -z now.  This omits the call of _dl_runtime_resolve within
>> setup_explicit_clear and the test passes.
>>
>> If this is okay, I'll commit this patch and clear this bug in the
>> release blockers list in the release-wiki.

This seems like a reasonable workaround to me.  Please commit.

(Guess we better add "spill slots for callee-save registers, including
registers saved only by dynamic linker stubs" to the list of things to
worry about when adding explicit_bzero to the compiler...)

zw

Re: [PATCH COMMITTED] Do not require memset elimination in explicit_bzero test

[Stefan Liebler]

On 01/16/2017 04:28 PM, Zack Weinberg wrote:
> On Mon, Jan 16, 2017 at 3:24 AM, Stefan Liebler <stli@linux.vnet.ibm.com> wrote:
>> On 01/10/2017 09:22 AM, Stefan Liebler wrote:
>>>
>>> In setup_explicit_clear, the buffer is filled with the test_pattern.
>>> On s390x the memcpy in prepare_test_buffer is done by loading
>>> r4 / r5 with the test_pattern and using store multiple instruction
>>> to store r4 / r5 to buf.
>>> If explicit_bzero is resolved in setup_explicit_clear, r4 / r5 is
>>> stored to stack by _dl_runtime_resolve and the call to memmem in
>>> count_test_patterns finds a hit of the test_pattern on the stack.
>>>
>>> The attached patch resolves all symbols at program startup by linking
>>> with -z now.  This omits the call of _dl_runtime_resolve within
>>> setup_explicit_clear and the test passes.
>>>
>>> If this is okay, I'll commit this patch and clear this bug in the
>>> release blockers list in the release-wiki.
>
> This seems like a reasonable workaround to me.  Please commit.
>
> (Guess we better add "spill slots for callee-save registers, including
> registers saved only by dynamic linker stubs" to the list of things to
> worry about when adding explicit_bzero to the compiler...)
>
> zw
>
Thanks.
Committed, closed bug and cleared entry in the release blockers list in 
the release-wiki.