[PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[Renlin Li]

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

Hi all,

After the change r249278. bcopy is folded into memmove. And in newlib aarch64
memmove implementation, it will call memcpy in certain conditions.
The memcpy defined in memops-asm-lib.c will abort when the test is running.

In this case, I defined a user memmove function which by pass the library one.
So that memcpy won't be called accidentally.

Okay to commit?

gcc/testsuite/ChangeLog:

2017-06-22  Renlin Li  <renlin.li@arm.com>
	    Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* gcc.c-torture/execute/builtins/memops-asm-lib.c (my_memmove): New.
	* gcc.c-torture/execute/builtins/memops-asm.c (memmove): Declare memmove.

[-- Attachment #2: tmp.diff --]
[-- Type: text/x-patch, Size: 1523 bytes --]

diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
index 0000529..25d4a40 100644
--- a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
@@ -37,6 +37,24 @@ my_bcopy (const void *s, void *d, size_t n)
     }
 }
 
+__attribute__ ((used))
+void
+my_memmove (void *d, const void *s, size_t n)
+{
+  char *dst = (char *) d;
+  const char *src = (const char *) s;
+  if (src >= dst)
+    while (n--)
+      *dst++ = *src++;
+  else
+    {
+      dst += n;
+      src += n;
+      while (n--)
+	*--dst = *--src;
+    }
+}
+
 /* LTO code is at the present to able to track that asm alias my_bcopy on builtin
    actually refers to this function.  See PR47181. */
 __attribute__ ((used))
diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm.c b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm.c
index ed2b06c..44e336c 100644
--- a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm.c
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm.c
@@ -12,6 +12,8 @@ extern void *memcpy (void *, const void *, size_t)
   __asm (ASMNAME ("my_memcpy"));
 extern void bcopy (const void *, void *, size_t)
   __asm (ASMNAME ("my_bcopy"));
+extern void *memmove (void *, const void *, size_t)
+  __asm (ASMNAME ("my_memmove"));
 extern void *memset (void *, int, size_t)
   __asm (ASMNAME ("my_memset"));
 extern void bzero (void *, size_t)

Re: [PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[Martin Sebor]

On 06/23/2017 03:19 AM, Renlin Li wrote:
> Hi all,
>
> After the change r249278. bcopy is folded into memmove. And in newlib
> aarch64
> memmove implementation, it will call memcpy in certain conditions.
> The memcpy defined in memops-asm-lib.c will abort when the test is running.
>
> In this case, I defined a user memmove function which by pass the
> library one.
> So that memcpy won't be called accidentally.
>
> Okay to commit?

Having memmove call memcpy when there is no overlap seems like
a valid transformation.  I don't know which test specifically
fails so the question on my mind is whether it perhaps is overly
restrictive in assuming that this transformation must never take
place.  Other than that, although I can't really approve patches,
this one looks okay to me.  Thanks for getting to the bottom of
the failure and fixing it!

Martin

>
> gcc/testsuite/ChangeLog:
>
> 2017-06-22  Renlin Li  <renlin.li@arm.com>
>         Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
>     * gcc.c-torture/execute/builtins/memops-asm-lib.c (my_memmove): New.
>     * gcc.c-torture/execute/builtins/memops-asm.c (memmove): Declare
> memmove.

Re: [PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[Renlin Li]

Hi Martin,

On 23/06/17 16:27, Martin Sebor wrote:
> On 06/23/2017 03:19 AM, Renlin Li wrote:
>> Hi all,
>>
>> After the change r249278. bcopy is folded into memmove. And in newlib
>> aarch64
>> memmove implementation, it will call memcpy in certain conditions.
>> The memcpy defined in memops-asm-lib.c will abort when the test is running.
>>
>> In this case, I defined a user memmove function which by pass the
>> library one.
>> So that memcpy won't be called accidentally.
>>
>> Okay to commit?
>
> Having memmove call memcpy when there is no overlap seems like
> a valid transformation.  I don't know which test specifically
> fails so the question on my mind is whether it perhaps is overly
> restrictive in assuming that this transformation must never take
> place.  Other than that, although I can't really approve patches,
> this one looks okay to me.  Thanks for getting to the bottom of
> the failure and fixing it!

Sorry I didn't mention the regressions.
It only happens with aarch64 baremetal targets because of the newlib memmove implementation.

FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -O0
FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -O1
FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -O2
FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -O3 -g
FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -Og -g
FAIL: gcc.c-torture/execute/builtins/memops-asm.c execution,  -Os

I think the purpose of the test is to check, the original function is not directly called 
from the main_test function.
Instead, those calls are redirected to "my_" version. It will abort otherwise.
I CCed Richard Sandiford as he is the original contributor of the test case.

Before r249278, bcopy has a corresponding my_bcopy function which is actually got called.

Regards,
Renlin

>
> Martin
>
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-06-22  Renlin Li  <renlin.li@arm.com>
>>         Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>>     * gcc.c-torture/execute/builtins/memops-asm-lib.c (my_memmove): New.
>>     * gcc.c-torture/execute/builtins/memops-asm.c (memmove): Declare
>> memmove.
>

Re: [PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[Jeff Law]

On 06/23/2017 03:19 AM, Renlin Li wrote:
> Hi all,
> 
> After the change r249278. bcopy is folded into memmove. And in newlib
> aarch64
> memmove implementation, it will call memcpy in certain conditions.
> The memcpy defined in memops-asm-lib.c will abort when the test is running.
> 
> In this case, I defined a user memmove function which by pass the
> library one.
> So that memcpy won't be called accidentally.
> 
> Okay to commit?
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-06-22  Renlin Li  <renlin.li@arm.com>
>         Szabolcs Nagy  <szabolcs.nagy@arm.com>
> 
>     * gcc.c-torture/execute/builtins/memops-asm-lib.c (my_memmove): New.
>     * gcc.c-torture/execute/builtins/memops-asm.c (memmove): Declare
> memmove.
OK.
jeff

Re: [PATCH][Testsuite] Use user defined memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

[Tom de Vries]

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

On 06/23/2017 11:19 AM, Renlin Li wrote:
> +__attribute__ ((used))
> +void
> +my_memmove (void *d, const void *s, size_t n)
> +{
> +  char *dst = (char *) d;
> +  const char *src = (const char *) s;
> +  if (src >= dst)
> +    while (n--)
> +      *dst++ = *src++;
> +  else
> +    {
> +      dst += n;
> +      src += n;
> +      while (n--)
> +	*--dst = *--src;
> +    }
> +}
> +

The memops-asm.c testcase fails for nvptx because of a mismatch between 
function definition and function declaration for my_memmove in the 
generated nvptx code.

man memmove show us that memmove returns a 'void *', not 'void':
...
        void *memmove(void *dest, const void *src, size_t n);
...

and that it returns its first argument (well, the formulation could be 
improved):
...
RETURN VALUE
        The memmove() function returns a pointer to dest.
...


Fixed in attached patch.

Committed.

Thanks,
- Tom

[-- Attachment #2: 0001-Fix-my_memmove-in-gcc.c-torture-execute-builtins-memops-asm-lib.c.patch --]
[-- Type: text/x-patch, Size: 1047 bytes --]

Fix my_memmove in gcc.c-torture/execute/builtins/memops-asm-lib.c

2017-08-06  Tom de Vries  <tom@codesourcery.com>

	* gcc.c-torture/execute/builtins/memops-asm-lib.c (my_memmove):  Fix return
	type.  Add missing return.

---
 gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
index 25d4a40..3baf7a6 100644
--- a/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
+++ b/gcc/testsuite/gcc.c-torture/execute/builtins/memops-asm-lib.c
@@ -38,7 +38,7 @@ my_bcopy (const void *s, void *d, size_t n)
 }
 
 __attribute__ ((used))
-void
+void *
 my_memmove (void *d, const void *s, size_t n)
 {
   char *dst = (char *) d;
@@ -53,6 +53,8 @@ my_memmove (void *d, const void *s, size_t n)
       while (n--)
 	*--dst = *--src;
     }
+
+  return d;
 }
 
 /* LTO code is at the present to able to track that asm alias my_bcopy on builtin