[PATCH v3 for-next 0/3] Improvements to string.h

[PATCH v3 for-next 0/3] Improvements to string.h

[Andrew Cooper]

This series has expanded a bit from its first version.  It has been
compile-checked on all architectures (locally and in Travis;
https://travis-ci.org/andyhhp/xen/builds/231640889), and functionally tested
on x86.

Andrew Cooper (3):
  xen/string: Clean up {xen,arm}/string.h
  xen/string: Use compiler __builtin_*() where possible
  x86/string: Clean up x86/string.h

 xen/arch/x86/string.c        |  11 ++---
 xen/common/string.c          |  24 +++++-----
 xen/include/asm-arm/string.h |  27 ++++++------
 xen/include/asm-x86/string.h |  18 ++++++--
 xen/include/xen/string.h     | 103 ++++++++++++++++++++++++++++---------------
 5 files changed, 111 insertions(+), 72 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 1/3] xen/string: Clean up {xen, arm}/string.h

[Andrew Cooper]

 * Drop __kernel_size_t entirely.  It isn't useful distinction, especially as
   it means the the prototypes don't appear to match their common definitions.
 * Introduce __HAVE_ARCH_* guards for strpbrk(), strsep() and strspn(), which
   match their implementation in common/string.c
 * Apply consistent Xen style throughout.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v3:
 * New
---
 xen/include/asm-arm/string.h | 27 +++++++------
 xen/include/xen/string.h     | 91 +++++++++++++++++++++++++++-----------------
 2 files changed, 69 insertions(+), 49 deletions(-)

diff --git a/xen/include/asm-arm/string.h b/xen/include/asm-arm/string.h
index 005959f..1804c3c 100644
--- a/xen/include/asm-arm/string.h
+++ b/xen/include/asm-arm/string.h
@@ -8,48 +8,47 @@
  */
 
 #define __HAVE_ARCH_STRRCHR
-extern char * strrchr(const char * s, int c);
+char *strrchr(const char *s, int c);
 
 #define __HAVE_ARCH_STRCHR
-extern char * strchr(const char * s, int c);
+char *strchr(const char *s, int c);
 
 #if defined(CONFIG_ARM_64)
 #define __HAVE_ARCH_STRCMP
-extern int strcmp(const char *, const char *);
+int strcmp(const char *, const char *);
 
 #define __HAVE_ARCH_STRNCMP
-extern int strncmp(const char *, const char *, __kernel_size_t);
+int strncmp(const char *, const char *, size_t);
 
 #define __HAVE_ARCH_STRLEN
-extern __kernel_size_t strlen(const char *);
+size_t strlen(const char *);
 
 #define __HAVE_ARCH_STRNLEN
-extern __kernel_size_t strnlen(const char *, __kernel_size_t);
+size_t strnlen(const char *, size_t);
 #endif
 
 #define __HAVE_ARCH_MEMCPY
-extern void * memcpy(void *, const void *, __kernel_size_t);
+void *memcpy(void *, const void *, size_t);
 
 #if defined(CONFIG_ARM_64)
 #define __HAVE_ARCH_MEMCMP
-extern int memcmp(const void *, const void *, __kernel_size_t);
+int memcmp(const void *, const void *, size_t);
 #endif
 
-/* Some versions of gcc don't have this builtin. It's non-critical anyway. */
 #define __HAVE_ARCH_MEMMOVE
-extern void *memmove(void *dest, const void *src, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
 
 #define __HAVE_ARCH_MEMSET
-extern void * memset(void *, int, __kernel_size_t);
+void *memset(void *, int, size_t);
 
 #define __HAVE_ARCH_MEMCHR
-extern void * memchr(const void *, int, __kernel_size_t);
+void *memchr(const void *, int, size_t);
 
 #if defined(CONFIG_ARM_32)
 
-extern void __memzero(void *ptr, __kernel_size_t n);
+void __memzero(void *ptr, size_t n);
 
-#define memset(p,v,n)                                                   \
+#define memset(p, v, n)                                                 \
         ({                                                              \
                 void *__p = (p); size_t __n = n;                        \
                 if ((__n) != 0) {                                       \
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h
index f35934e..eb7aeaa 100644
--- a/xen/include/xen/string.h
+++ b/xen/include/xen/string.h
@@ -1,19 +1,8 @@
-#ifndef _LINUX_STRING_H_
-#define _LINUX_STRING_H_
+#ifndef __XEN_STRING_H__
+#define __XEN_STRING_H__
 
 #include <xen/types.h>	/* for size_t */
 
-#ifdef __cplusplus
-extern "C" {
-#endif
-
-#define __kernel_size_t size_t
-
-extern char * strpbrk(const char *,const char *);
-extern char * strsep(char **,const char *);
-extern __kernel_size_t strspn(const char *,const char *);
-
-
 /*
  * Include machine specific inline routines
  */
@@ -29,60 +18,84 @@ extern __kernel_size_t strspn(const char *,const char *);
 #define strncat __xen_has_no_strncat__
 
 #ifndef __HAVE_ARCH_STRLCPY
-extern size_t strlcpy(char *,const char *, __kernel_size_t);
+size_t strlcpy(char *, const char *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_STRLCAT
-extern size_t strlcat(char *,const char *, __kernel_size_t);
+size_t strlcat(char *, const char *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_STRCMP
-extern int strcmp(const char *,const char *);
+int strcmp(const char *, const char *);
 #endif
+
 #ifndef __HAVE_ARCH_STRNCMP
-extern int strncmp(const char *,const char *,__kernel_size_t);
+int strncmp(const char *, const char *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_STRNICMP
-extern int strnicmp(const char *, const char *, __kernel_size_t);
+int strnicmp(const char *, const char *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_STRCASECMP
-extern int strcasecmp(const char *, const char *);
+int strcasecmp(const char *, const char *);
 #endif
+
 #ifndef __HAVE_ARCH_STRCHR
-extern char * strchr(const char *,int);
+char *strchr(const char *, int);
 #endif
+
 #ifndef __HAVE_ARCH_STRRCHR
-extern char * strrchr(const char *,int);
+char *strrchr(const char *, int);
 #endif
+
 #ifndef __HAVE_ARCH_STRSTR
-extern char * strstr(const char *,const char *);
+char *strstr(const char *, const char *);
 #endif
+
 #ifndef __HAVE_ARCH_STRLEN
-extern __kernel_size_t strlen(const char *);
+size_t strlen(const char *);
 #endif
+
 #ifndef __HAVE_ARCH_STRNLEN
-extern __kernel_size_t strnlen(const char *,__kernel_size_t);
+size_t strnlen(const char *, size_t);
 #endif
 
+#ifndef __HAVE_ARCH_STRPBRK
+char *strpbrk(const char *, const char *);
+#endif
+
+#ifndef __HAVE_ARCH_STRSEP
+char *strsep(char **, const char *);
+#endif
+
+#ifndef __HAVE_ARCH_STRSPN
+size_t strspn(const char *, const char *);
+#endif
+
+
 #ifndef __HAVE_ARCH_MEMSET
-extern void * memset(void *,int,__kernel_size_t);
+void *memset(void *, int, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_MEMCPY
-extern void * memcpy(void *,const void *,__kernel_size_t);
+void *memcpy(void *, const void *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_MEMMOVE
-extern void * memmove(void *,const void *,__kernel_size_t);
+void *memmove(void *, const void *, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_MEMSCAN
-extern void * memscan(void *,int,__kernel_size_t);
+void *memscan(void *, int, size_t);
 #endif
+
 #ifndef __HAVE_ARCH_MEMCMP
-extern int memcmp(const void *,const void *,__kernel_size_t);
-#endif
-#ifndef __HAVE_ARCH_MEMCHR
-extern void * memchr(const void *,int,__kernel_size_t);
+int memcmp(const void *, const void *, size_t);
 #endif
 
-#ifdef __cplusplus
-}
+#ifndef __HAVE_ARCH_MEMCHR
+void *memchr(const void *, int, size_t);
 #endif
 
 #define is_char_array(x) __builtin_types_compatible_p(typeof(x), char[])
@@ -97,4 +110,12 @@ extern void * memchr(const void *,int,__kernel_size_t);
     (strlcat(d, s, sizeof(d)) >= sizeof(d));    \
 })
 
-#endif /* _LINUX_STRING_H_ */
+#endif /* __XEN_STRING_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 2/3] xen/string: Use compiler __builtin_*() where possible

[Andrew Cooper]

The use of -fno-builtin inhibits these automatic transformations.  This causes
constructs such as strlen("literal") to be evaluated at compile time, and
certain simple operations to be replaced with repeated string operations.

To avoid the macro altering the function names, use the method recommended by
the C specification by enclosing the function name in brackets to avoid the
macro being expanded.  This means that optimisation opportunities continue to
work in the rest of the translation unit.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Fix the build with Clang, which objects when the define renames the
   underlying implementation.
v3:
 * Move into common code.
 * Retain symbol definition for function pointer use.
---
 xen/common/string.c      | 24 ++++++++++++------------
 xen/include/xen/string.h | 12 ++++++++++++
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/xen/common/string.c b/xen/common/string.c
index 9a5a4ba..1e122ab 100644
--- a/xen/common/string.c
+++ b/xen/common/string.c
@@ -42,7 +42,7 @@ int strnicmp(const char *s1, const char *s2, size_t len)
 #endif
 
 #ifndef __HAVE_ARCH_STRCASECMP
-int strcasecmp(const char *s1, const char *s2)
+int (strcasecmp)(const char *s1, const char *s2)
 {
     int c1, c2;
 
@@ -117,7 +117,7 @@ EXPORT_SYMBOL(strlcat);
  * @cs: One string
  * @ct: Another string
  */
-int strcmp(const char * cs,const char * ct)
+int (strcmp)(const char *cs, const char *ct)
 {
 	register signed char __res;
 
@@ -137,7 +137,7 @@ int strcmp(const char * cs,const char * ct)
  * @ct: Another string
  * @count: The maximum number of bytes to compare
  */
-int strncmp(const char * cs,const char * ct,size_t count)
+int (strncmp)(const char *cs, const char *ct, size_t count)
 {
 	register signed char __res = 0;
 
@@ -157,7 +157,7 @@ int strncmp(const char * cs,const char * ct,size_t count)
  * @s: The string to be searched
  * @c: The character to search for
  */
-char * strchr(const char * s, int c)
+char *(strchr)(const char *s, int c)
 {
 	for(; *s != (char) c; ++s)
 		if (*s == '\0')
@@ -172,7 +172,7 @@ char * strchr(const char * s, int c)
  * @s: The string to be searched
  * @c: The character to search for
  */
-char * strrchr(const char * s, int c)
+char *(strrchr)(const char *s, int c)
 {
        const char *p = s + strlen(s);
        do {
@@ -188,7 +188,7 @@ char * strrchr(const char * s, int c)
  * strlen - Find the length of a string
  * @s: The string to be sized
  */
-size_t strlen(const char * s)
+size_t (strlen)(const char * s)
 {
 	const char *sc;
 
@@ -298,7 +298,7 @@ char * strsep(char **s, const char *ct)
  *
  * Do not use memset() to access IO space, use memset_io() instead.
  */
-void * memset(void * s,int c,size_t count)
+void *(memset)(void *s, int c, size_t count)
 {
 	char *xs = (char *) s;
 
@@ -319,7 +319,7 @@ void * memset(void * s,int c,size_t count)
  * You should not use this function to access IO space, use memcpy_toio()
  * or memcpy_fromio() instead.
  */
-void * memcpy(void * dest,const void *src,size_t count)
+void *(memcpy)(void *dest, const void *src, size_t count)
 {
 	char *tmp = (char *) dest, *s = (char *) src;
 
@@ -339,7 +339,7 @@ void * memcpy(void * dest,const void *src,size_t count)
  *
  * Unlike memcpy(), memmove() copes with overlapping areas.
  */
-void * memmove(void * dest,const void *src,size_t count)
+void *(memmove)(void *dest, const void *src, size_t count)
 {
 	char *tmp, *s;
 
@@ -367,7 +367,7 @@ void * memmove(void * dest,const void *src,size_t count)
  * @ct: Another area of memory
  * @count: The size of the area.
  */
-int memcmp(const void * cs,const void * ct,size_t count)
+int (memcmp)(const void *cs, const void *ct, size_t count)
 {
 	const unsigned char *su1, *su2;
 	int res = 0;
@@ -409,7 +409,7 @@ void * memscan(void * addr, int c, size_t size)
  * @s1: The string to be searched
  * @s2: The string to search for
  */
-char * strstr(const char * s1,const char * s2)
+char *(strstr)(const char *s1, const char *s2)
 {
 	int l1, l2;
 
@@ -437,7 +437,7 @@ char * strstr(const char * s1,const char * s2)
  * returns the address of the first occurrence of @c, or %NULL
  * if @c is not found
  */
-void *memchr(const void *s, int c, size_t n)
+void *(memchr)(const void *s, int c, size_t n)
 {
 	const unsigned char *p = s;
 	while (n-- != 0) {
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h
index eb7aeaa..853db2f 100644
--- a/xen/include/xen/string.h
+++ b/xen/include/xen/string.h
@@ -27,10 +27,12 @@ size_t strlcat(char *, const char *, size_t);
 
 #ifndef __HAVE_ARCH_STRCMP
 int strcmp(const char *, const char *);
+#define strcmp(s1, s2) __builtin_strcmp(s1, s2)
 #endif
 
 #ifndef __HAVE_ARCH_STRNCMP
 int strncmp(const char *, const char *, size_t);
+#define strncmp(s1, s2, n) __builtin_strncmp(s1, s2, n)
 #endif
 
 #ifndef __HAVE_ARCH_STRNICMP
@@ -39,22 +41,27 @@ int strnicmp(const char *, const char *, size_t);
 
 #ifndef __HAVE_ARCH_STRCASECMP
 int strcasecmp(const char *, const char *);
+#define strcasecmp(s1, s2) __builtin_strcasecmp(s1, s2)
 #endif
 
 #ifndef __HAVE_ARCH_STRCHR
 char *strchr(const char *, int);
+#define strchr(s1, c) __builtin_strchr(s1, c)
 #endif
 
 #ifndef __HAVE_ARCH_STRRCHR
 char *strrchr(const char *, int);
+#define strrchr(s1, c) __builtin_strrchr(s1, c)
 #endif
 
 #ifndef __HAVE_ARCH_STRSTR
 char *strstr(const char *, const char *);
+#define strstr(s1, s2) __builtin_strstr(s1, s2)
 #endif
 
 #ifndef __HAVE_ARCH_STRLEN
 size_t strlen(const char *);
+#define strlen(s1) __builtin_strlen(s1)
 #endif
 
 #ifndef __HAVE_ARCH_STRNLEN
@@ -76,14 +83,17 @@ size_t strspn(const char *, const char *);
 
 #ifndef __HAVE_ARCH_MEMSET
 void *memset(void *, int, size_t);
+#define memset(s, c, n) __builtin_memset(s, c, n)
 #endif
 
 #ifndef __HAVE_ARCH_MEMCPY
 void *memcpy(void *, const void *, size_t);
+#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
 #endif
 
 #ifndef __HAVE_ARCH_MEMMOVE
 void *memmove(void *, const void *, size_t);
+#define memmove(d, s, n) __builtin_memmove(d, s, n)
 #endif
 
 #ifndef __HAVE_ARCH_MEMSCAN
@@ -92,10 +102,12 @@ void *memscan(void *, int, size_t);
 
 #ifndef __HAVE_ARCH_MEMCMP
 int memcmp(const void *, const void *, size_t);
+#define memcmp(s1, s2, n) __builtin_memcmp(s1, s2, n)
 #endif
 
 #ifndef __HAVE_ARCH_MEMCHR
 void *memchr(const void *, int, size_t);
+#define memchr(s, c, n) __builtin_memchr(s, c, n)
 #endif
 
 #define is_char_array(x) __builtin_types_compatible_p(typeof(x), char[])
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v3 3/3] x86/string: Clean up x86/string.h

[Andrew Cooper]

 * None of the GCC docs mention memmove() in its list of builtins even today,
   but 4.1 does have the builtin, meaning that all currently supported
   compilers have it.
 * Consistently use Xen style, matching the common code, and introduce symbol
   definitions for function pointer use.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21602 confirms that the builtin
was present in 4.1.0

v3:
 * Rebase around the rework in the rest of the series
---
 xen/arch/x86/string.c        | 11 ++++-------
 xen/include/asm-x86/string.h | 18 ++++++++++++++----
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/string.c b/xen/arch/x86/string.c
index 1387dfb..cd85a38 100644
--- a/xen/arch/x86/string.c
+++ b/xen/arch/x86/string.c
@@ -1,14 +1,13 @@
 /******************************************************************************
  * string.c
- * 
+ *
  * These provide something for compiler-emitted string operations to link
  * against.
  */
 
 #include <xen/lib.h>
 
-#undef memcpy
-void *memcpy(void *dest, const void *src, size_t n)
+void *(memcpy)(void *dest, const void *src, size_t n)
 {
     long d0, d1, d2;
 
@@ -23,8 +22,7 @@ void *memcpy(void *dest, const void *src, size_t n)
     return dest;
 }
 
-#undef memset
-void *memset(void *s, int c, size_t n)
+void *(memset)(void *s, int c, size_t n)
 {
     long d0, d1;
 
@@ -37,8 +35,7 @@ void *memset(void *s, int c, size_t n)
     return s;
 }
 
-#undef memmove
-void *memmove(void *dest, const void *src, size_t n)
+void *(memmove)(void *dest, const void *src, size_t n)
 {
     long d0, d1, d2;
 
diff --git a/xen/include/asm-x86/string.h b/xen/include/asm-x86/string.h
index c48d9c3..6f3c960 100644
--- a/xen/include/asm-x86/string.h
+++ b/xen/include/asm-x86/string.h
@@ -2,13 +2,23 @@
 #define __X86_STRING_H__
 
 #define __HAVE_ARCH_MEMCPY
-#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
+void *memcpy(void *dest, const void *src, size_t n);
+#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
 
-/* Some versions of gcc don't have this builtin. It's non-critical anyway. */
 #define __HAVE_ARCH_MEMMOVE
-extern void *memmove(void *dest, const void *src, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+#define memmove(d, s, n) __builtin_memmove(d, s, n)
 
 #define __HAVE_ARCH_MEMSET
-#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
+void *memset(void *dest, int c, size_t n);
+#define memset(s, c, n) __builtin_memset(s, c, n)
 
 #endif /* __X86_STRING_H__ */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 1/3] xen/string: Clean up {xen, arm}/string.h

[Jan Beulich]

 >>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
> * Drop __kernel_size_t entirely.  It isn't useful distinction, especially as
>    it means the the prototypes don't appear to match their common definitions.
>  * Introduce __HAVE_ARCH_* guards for strpbrk(), strsep() and strspn(), which
>    match their implementation in common/string.c
>  * Apply consistent Xen style throughout.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 2/3] xen/string: Use compiler __builtin_*() where possible

[Jan Beulich]

>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
> The use of -fno-builtin inhibits these automatic transformations.  This causes
> constructs such as strlen("literal") to be evaluated at compile time, and
> certain simple operations to be replaced with repeated string operations.
> 
> To avoid the macro altering the function names, use the method recommended by
> the C specification by enclosing the function name in brackets to avoid the
> macro being expanded.  This means that optimisation opportunities continue to
> work in the rest of the translation unit.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Jan Beulich]

 >>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/asm-x86/string.h
> +++ b/xen/include/asm-x86/string.h
> @@ -2,13 +2,23 @@
>  #define __X86_STRING_H__
>  
>  #define __HAVE_ARCH_MEMCPY
> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
> +void *memcpy(void *dest, const void *src, size_t n);
> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>  
> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. */
>  #define __HAVE_ARCH_MEMMOVE
> -extern void *memmove(void *dest, const void *src, size_t n);
> +void *memmove(void *dest, const void *src, size_t n);
> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>  
>  #define __HAVE_ARCH_MEMSET
> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
> +void *memset(void *dest, int c, size_t n);
> +#define memset(s, c, n) __builtin_memset(s, c, n)

Now that xen/string.h has the exact same declarations and
definitions already, why don't you simply delete the overrides
from here?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Jan Beulich]

 >>> On 15.05.17 at 12:08, <JBeulich@suse.com> wrote:
>>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/asm-x86/string.h
>> +++ b/xen/include/asm-x86/string.h
>> @@ -2,13 +2,23 @@
>>  #define __X86_STRING_H__
>>  
>>  #define __HAVE_ARCH_MEMCPY
>> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
>> +void *memcpy(void *dest, const void *src, size_t n);
>> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>>  
>> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. 
> */
>>  #define __HAVE_ARCH_MEMMOVE
>> -extern void *memmove(void *dest, const void *src, size_t n);
>> +void *memmove(void *dest, const void *src, size_t n);
>> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>>  
>>  #define __HAVE_ARCH_MEMSET
>> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
>> +void *memset(void *dest, int c, size_t n);
>> +#define memset(s, c, n) __builtin_memset(s, c, n)
> 
> Now that xen/string.h has the exact same declarations and
> definitions already, why don't you simply delete the overrides
> from here?

Hmm, wait - I guess you need to keep them because of the custom
implementation. That's awkward, there shouldn't be a need to have
redundant declarations just because there are custom
implementations. How about making __HAVE_ARCH_* serve both
purposes, by allowing it to have different values (besides being
defined or undefined)?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Andrew Cooper]

On 15/05/17 11:19, Jan Beulich wrote:
>  >>> On 15.05.17 at 12:08, <JBeulich@suse.com> wrote:
>>>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/include/asm-x86/string.h
>>> +++ b/xen/include/asm-x86/string.h
>>> @@ -2,13 +2,23 @@
>>>  #define __X86_STRING_H__
>>>  
>>>  #define __HAVE_ARCH_MEMCPY
>>> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
>>> +void *memcpy(void *dest, const void *src, size_t n);
>>> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>>>  
>>> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. 
>> */
>>>  #define __HAVE_ARCH_MEMMOVE
>>> -extern void *memmove(void *dest, const void *src, size_t n);
>>> +void *memmove(void *dest, const void *src, size_t n);
>>> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>>>  
>>>  #define __HAVE_ARCH_MEMSET
>>> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
>>> +void *memset(void *dest, int c, size_t n);
>>> +#define memset(s, c, n) __builtin_memset(s, c, n)
>> Now that xen/string.h has the exact same declarations and
>> definitions already, why don't you simply delete the overrides
>> from here?
> Hmm, wait - I guess you need to keep them because of the custom
> implementation. That's awkward, there shouldn't be a need to have
> redundant declarations just because there are custom
> implementations. How about making __HAVE_ARCH_* serve both
> purposes, by allowing it to have different values (besides being
> defined or undefined)?

I don't understand how you would intend this new __HAVE_ARCH_* to work.

ARM uses the current behaviour to implement memset() as a local macro.

Given that these files aren't likely to change much at all, I'd
recommend keeping the same behaviour as Linux.  It is only 3 lines after
all.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Jan Beulich]

>>> On 15.05.17 at 15:08, <andrew.cooper3@citrix.com> wrote:
> On 15/05/17 11:19, Jan Beulich wrote:
>>  >>> On 15.05.17 at 12:08, <JBeulich@suse.com> wrote:
>>>>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/include/asm-x86/string.h
>>>> +++ b/xen/include/asm-x86/string.h
>>>> @@ -2,13 +2,23 @@
>>>>  #define __X86_STRING_H__
>>>>  
>>>>  #define __HAVE_ARCH_MEMCPY
>>>> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
>>>> +void *memcpy(void *dest, const void *src, size_t n);
>>>> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>>>>  
>>>> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. 
>>> */
>>>>  #define __HAVE_ARCH_MEMMOVE
>>>> -extern void *memmove(void *dest, const void *src, size_t n);
>>>> +void *memmove(void *dest, const void *src, size_t n);
>>>> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>>>>  
>>>>  #define __HAVE_ARCH_MEMSET
>>>> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
>>>> +void *memset(void *dest, int c, size_t n);
>>>> +#define memset(s, c, n) __builtin_memset(s, c, n)
>>> Now that xen/string.h has the exact same declarations and
>>> definitions already, why don't you simply delete the overrides
>>> from here?
>> Hmm, wait - I guess you need to keep them because of the custom
>> implementation. That's awkward, there shouldn't be a need to have
>> redundant declarations just because there are custom
>> implementations. How about making __HAVE_ARCH_* serve both
>> purposes, by allowing it to have different values (besides being
>> defined or undefined)?
> 
> I don't understand how you would intend this new __HAVE_ARCH_* to work.

E.g. __HAVE_ARCH_* = 2 meaning arch provides declaration and
definition (i.e. generic header and source skip theirs), while
__HAVE_ARCH_* = 1 meaning arch provides just a definition, but
the generic declaration and macro (where applicable) are fine (i.e.
only the generic source skips its piece of code).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 1/3] xen/string: Clean up {xen, arm}/string.h

[Julien Grall]

Hi Andrew,

On 05/12/2017 06:35 PM, Andrew Cooper wrote:
>  * Drop __kernel_size_t entirely.  It isn't useful distinction, especially as
>    it means the the prototypes don't appear to match their common definitions.
>  * Introduce __HAVE_ARCH_* guards for strpbrk(), strsep() and strspn(), which
>    match their implementation in common/string.c
>  * Apply consistent Xen style throughout.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

For ARM bits:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Andrew Cooper]

On 15/05/17 15:22, Jan Beulich wrote:
>>>> On 15.05.17 at 15:08, <andrew.cooper3@citrix.com> wrote:
>> On 15/05/17 11:19, Jan Beulich wrote:
>>>  >>> On 15.05.17 at 12:08, <JBeulich@suse.com> wrote:
>>>>>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/include/asm-x86/string.h
>>>>> +++ b/xen/include/asm-x86/string.h
>>>>> @@ -2,13 +2,23 @@
>>>>>  #define __X86_STRING_H__
>>>>>  
>>>>>  #define __HAVE_ARCH_MEMCPY
>>>>> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
>>>>> +void *memcpy(void *dest, const void *src, size_t n);
>>>>> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>>>>>  
>>>>> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. 
>>>> */
>>>>>  #define __HAVE_ARCH_MEMMOVE
>>>>> -extern void *memmove(void *dest, const void *src, size_t n);
>>>>> +void *memmove(void *dest, const void *src, size_t n);
>>>>> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>>>>>  
>>>>>  #define __HAVE_ARCH_MEMSET
>>>>> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
>>>>> +void *memset(void *dest, int c, size_t n);
>>>>> +#define memset(s, c, n) __builtin_memset(s, c, n)
>>>> Now that xen/string.h has the exact same declarations and
>>>> definitions already, why don't you simply delete the overrides
>>>> from here?
>>> Hmm, wait - I guess you need to keep them because of the custom
>>> implementation. That's awkward, there shouldn't be a need to have
>>> redundant declarations just because there are custom
>>> implementations. How about making __HAVE_ARCH_* serve both
>>> purposes, by allowing it to have different values (besides being
>>> defined or undefined)?
>> I don't understand how you would intend this new __HAVE_ARCH_* to work.
> E.g. __HAVE_ARCH_* = 2 meaning arch provides declaration and
> definition (i.e. generic header and source skip theirs), while
> __HAVE_ARCH_* = 1 meaning arch provides just a definition, but
> the generic declaration and macro (where applicable) are fine (i.e.
> only the generic source skips its piece of code).

I really don't thing its worth effort to make this change and diverge
from the Linux meaning.

It is not like these declarations are going to change often.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v3 3/3] x86/string: Clean up x86/string.h

[Jan Beulich]

>>> On 31.05.17 at 14:42, <andrew.cooper3@citrix.com> wrote:
> On 15/05/17 15:22, Jan Beulich wrote:
>>>>> On 15.05.17 at 15:08, <andrew.cooper3@citrix.com> wrote:
>>> On 15/05/17 11:19, Jan Beulich wrote:
>>>>  >>> On 15.05.17 at 12:08, <JBeulich@suse.com> wrote:
>>>>>>>> On 12.05.17 at 19:35, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/xen/include/asm-x86/string.h
>>>>>> +++ b/xen/include/asm-x86/string.h
>>>>>> @@ -2,13 +2,23 @@
>>>>>>  #define __X86_STRING_H__
>>>>>>  
>>>>>>  #define __HAVE_ARCH_MEMCPY
>>>>>> -#define memcpy(t,f,n) (__builtin_memcpy((t),(f),(n)))
>>>>>> +void *memcpy(void *dest, const void *src, size_t n);
>>>>>> +#define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>>>>>>  
>>>>>> -/* Some versions of gcc don't have this builtin. It's non-critical anyway. 
>>>>> */
>>>>>>  #define __HAVE_ARCH_MEMMOVE
>>>>>> -extern void *memmove(void *dest, const void *src, size_t n);
>>>>>> +void *memmove(void *dest, const void *src, size_t n);
>>>>>> +#define memmove(d, s, n) __builtin_memmove(d, s, n)
>>>>>>  
>>>>>>  #define __HAVE_ARCH_MEMSET
>>>>>> -#define memset(s,c,n) (__builtin_memset((s),(c),(n)))
>>>>>> +void *memset(void *dest, int c, size_t n);
>>>>>> +#define memset(s, c, n) __builtin_memset(s, c, n)
>>>>> Now that xen/string.h has the exact same declarations and
>>>>> definitions already, why don't you simply delete the overrides
>>>>> from here?
>>>> Hmm, wait - I guess you need to keep them because of the custom
>>>> implementation. That's awkward, there shouldn't be a need to have
>>>> redundant declarations just because there are custom
>>>> implementations. How about making __HAVE_ARCH_* serve both
>>>> purposes, by allowing it to have different values (besides being
>>>> defined or undefined)?
>>> I don't understand how you would intend this new __HAVE_ARCH_* to work.
>> E.g. __HAVE_ARCH_* = 2 meaning arch provides declaration and
>> definition (i.e. generic header and source skip theirs), while
>> __HAVE_ARCH_* = 1 meaning arch provides just a definition, but
>> the generic declaration and macro (where applicable) are fine (i.e.
>> only the generic source skips its piece of code).
> 
> I really don't thing its worth effort to make this change and diverge
> from the Linux meaning.
> 
> It is not like these declarations are going to change often.

Well, okay then. Feel free to add my ack.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel