[PATCH v2 0/3] sh: fixup strncpy()

[PATCH v2 0/3] sh: fixup strncpy()

[Kuninori Morimoto]

Hi Sato-san, Rich

These are strncpy() fixup patches, but using different solutions.
Karl's patches are updating current strncpy(), but using 2 patterns.
Kuninori's patch is using generic strncpy().

We don't know which is the best, but we can follow
SH maintainer's selection.

Karl Nasrallah (2):
      sh: fixup strncpy() warning and add missing padding
      sh: fixup strncpy() warning and add missing padding

Kuninori Morimoto (4):
      sh: use generic strncpy()

Thank you for your help !!
Best regards
---
Kuninori Morimoto

[PATCH v2 1/3] sh: fixup strncpy() warning and add missing padding

[Kuninori Morimoto]

From: Karl Nasrallah <knnspeed@aol.com>

Current SH will get below warning at strncpy()

In file included from ${LINUX}/arch/sh/include/asm/string.h:3,
                 from ${LINUX}/include/linux/string.h:20,
                 from ${LINUX}/include/linux/bitmap.h:9,
                 from ${LINUX}/include/linux/nodemask.h:95,
                 from ${LINUX}/include/linux/mmzone.h:17,
                 from ${LINUX}/include/linux/gfp.h:6,
                 from ${LINUX}/innclude/linux/slab.h:15,
                 from ${LINUX}/linux/drivers/mmc/host/vub300.c:38:
${LINUX}/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
${LINUX}/arch/sh/include/asm/string_32.h:51:42: warning: array subscript\
  80 is above array bounds of 'char[26]' [-Warray-bounds]
   : "0" (__dest), "1" (__src), "r" (__src+__n)
                                     ~~~~~^~~~

And current implementation is missing padding which strncpy()
should support.

This patch solves these 2 issues.

Signed-off-by: Karl Nasrallah <knnspeed@aol.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

	- tidyup git-log
	- tidyup 80char
	- tidyup Subject

 arch/sh/include/asm/string_32.h | 56 +++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 19 deletions(-)

diff --git a/arch/sh/include/asm/string_32.h b/arch/sh/include/asm/string_32.h
index 3558b1d..4978f6e 100644
--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -31,27 +31,45 @@ static inline char *strcpy(char *__dest, const char *__src)
 #define __HAVE_ARCH_STRNCPY
 static inline char *strncpy(char *__dest, const char *__src, size_t __n)
 {
-	register char *__xdest = __dest;
-	unsigned long __dummy;
+	char * retval = __dest;
+	const char * __dest_end = __dest + __n - 1;
 
+	/* size_t is always unsigned */
 	if (__n == 0)
-		return __xdest;
-
-	__asm__ __volatile__(
-		"1:\n"
-		"mov.b	@%1+, %2\n\t"
-		"mov.b	%2, @%0\n\t"
-		"cmp/eq	#0, %2\n\t"
-		"bt/s	2f\n\t"
-		" cmp/eq	%5,%1\n\t"
-		"bf/s	1b\n\t"
-		" add	#1, %0\n"
-		"2:"
-		: "=r" (__dest), "=r" (__src), "=&z" (__dummy)
-		: "0" (__dest), "1" (__src), "r" (__src+__n)
-		: "memory", "t");
-
-	return __xdest;
+		return retval;
+
+	/*
+	 * Some notes:
+	 * - cmp/eq #imm8,r0 is its own instruction
+	 * - incrementing dest and comparing to dest_end handles the size
+	 *   parameter in only one instruction
+	 * - mov.b R0,@Rn+ is SH2A only, but we can fill a delay slot with
+	 *   "add #1,%[dest]"
+	 */
+
+	__asm__ __volatile__ (
+		"strncpy_start:\n\t"
+		"mov.b @%[src]+,r0\n\t"
+		"cmp/eq #0,r0\n\t"
+		"bt.s strncpy_pad\n\t"
+		"cmp/eq %[dest],%[dest_end]\n\t"
+		"bt.s strncpy_end\n\t"
+		"mov.b r0,@%[dest]\n\t"
+		"bra strncpy_start\n\t"
+		"add #1,%[dest]\n\t"
+		"strncpy_pad:\n\t"
+		"bt.s strncpy_end\n\t"
+		"mov.b r0,@%[dest]\n\t"
+		"add #1,%[dest]\n\t"
+		"bra strncpy_pad\n\t"
+		"cmp/eq %[dest],%[dest_end]\n\t"
+		"strncpy_end:\n\t"
+		: [src] "+r" (__src), [dest] "+r" (__dest)
+		: [dest_end] "r" (__dest_end)
+		: "r0","t","memory"
+		);
+
+	return retval;
 }
 
 #define __HAVE_ARCH_STRCMP
-- 
2.7.4

[PATCH v2 2/3] sh: fixup strncpy() warning and add missing padding

[Kuninori Morimoto]

From: Karl Nasrallah <knnspeed@aol.com>

Current SH will get below warning at strncpy()

In file included from ${LINUX}/arch/sh/include/asm/string.h:3,
                 from ${LINUX}/include/linux/string.h:20,
                 from ${LINUX}/include/linux/bitmap.h:9,
                 from ${LINUX}/include/linux/nodemask.h:95,
                 from ${LINUX}/include/linux/mmzone.h:17,
                 from ${LINUX}/include/linux/gfp.h:6,
                 from ${LINUX}/innclude/linux/slab.h:15,
                 from ${LINUX}/linux/drivers/mmc/host/vub300.c:38:
${LINUX}/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
${LINUX}/arch/sh/include/asm/string_32.h:51:42: warning: array subscript\
  80 is above array bounds of 'char[26]' [-Warray-bounds]
   : "0" (__dest), "1" (__src), "r" (__src+__n)
                                     ~~~~~^~~~

And current implementation is missing padding which strncpy()
should support.

This patch solves these 2 issues.

Signed-off-by: Karl Nasrallah <knnspeed@aol.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

	- tidyup git-log
	- tidyup 80char
	- tidyup Subject

 arch/sh/include/asm/string_32.h | 58 +++++++++++++++++++++++++++--------------
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/arch/sh/include/asm/string_32.h b/arch/sh/include/asm/string_32.h
index 3558b1d..3b4aec0 100644
--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -31,27 +31,47 @@ static inline char *strcpy(char *__dest, const char *__src)
 #define __HAVE_ARCH_STRNCPY
 static inline char *strncpy(char *__dest, const char *__src, size_t __n)
 {
-	register char *__xdest = __dest;
-	unsigned long __dummy;
+	char * retval = __dest;
+	const char * __dest_end = __dest + __n - 1;
+	register unsigned int * r0_register __asm__ ("r0");
 
+	/* size_t is always unsigned */
 	if (__n == 0)
-		return __xdest;
-
-	__asm__ __volatile__(
-		"1:\n"
-		"mov.b	@%1+, %2\n\t"
-		"mov.b	%2, @%0\n\t"
-		"cmp/eq	#0, %2\n\t"
-		"bt/s	2f\n\t"
-		" cmp/eq	%5,%1\n\t"
-		"bf/s	1b\n\t"
-		" add	#1, %0\n"
-		"2:"
-		: "=r" (__dest), "=r" (__src), "=&z" (__dummy)
-		: "0" (__dest), "1" (__src), "r" (__src+__n)
-		: "memory", "t");
-
-	return __xdest;
+		return retval;
+
+	/*
+	 * Some notes:
+	 * - cmp/eq #imm8,r0 is its own instruction
+	 * - incrementing dest and comparing to dest_end handles the size
+	 *   parameter in only one instruction
+	 * - mov.b R0,@Rn+ is SH2A only, but we can fill a delay slot with
+	 *   "add #1,%[dest]"
+	 */
+
+	__asm__ __volatile__ (
+		"strncpy_start:\n\t"
+		"mov.b @%[src]+,%[r0_reg]\n\t"
+		"cmp/eq #0,%[r0_reg]\n\t"
+		"bt.s strncpy_pad\n\t"
+		"cmp/eq %[dest],%[dest_end]\n\t"
+		"bt.s strncpy_end\n\t"
+		"mov.b %[r0_reg],@%[dest]\n\t"
+		"bra strncpy_start\n\t"
+		"add #1,%[dest]\n\t"
+		"strncpy_pad:\n\t"
+		"bt.s strncpy_end\n\t"
+		"mov.b %[r0_reg],@%[dest]\n\t"
+		"add #1,%[dest]\n\t"
+		"bra strncpy_pad\n\t"
+		"cmp/eq %[dest],%[dest_end]\n\t"
+		"strncpy_end:\n\t"
+		: [src] "+r" (__src), [dest] "+r" (__dest),
+		  [r0_reg] "+&z" (r0_register)
+		: [dest_end] "r" (__dest_end)
+		: "t","memory"
+		);
+
+	return retval;
 }
 
 #define __HAVE_ARCH_STRCMP
-- 
2.7.4

[PATCH 3/3] sh: use generic strncpy()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Current SH will get below warning at strncpy()

In file included from ${LINUX}/arch/sh/include/asm/string.h:3,
                 from ${LINUX}/include/linux/string.h:20,
                 from ${LINUX}/include/linux/bitmap.h:9,
                 from ${LINUX}/include/linux/nodemask.h:95,
                 from ${LINUX}/include/linux/mmzone.h:17,
                 from ${LINUX}/include/linux/gfp.h:6,
                 from ${LINUX}/innclude/linux/slab.h:15,
                 from ${LINUX}/linux/drivers/mmc/host/vub300.c:38:
${LINUX}/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
${LINUX}/arch/sh/include/asm/string_32.h:51:42: warning: array subscript\
  80 is above array bounds of 'char[26]' [-Warray-bounds]
   : "0" (__dest), "1" (__src), "r" (__src+__n)
                                     ~~~~~^~~~

And current implementation is missing padding which strncpy()
should support.

It needs big fixup to solve these issues,
then, __asm__ code is difficult to maintenance.

To solve/avoid these issues, this patch uses generic strncpy()
instead of architecture specific one.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

	- tidyup git-log

 arch/sh/include/asm/string_32.h | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/arch/sh/include/asm/string_32.h b/arch/sh/include/asm/string_32.h
index 3558b1d..be3f9a0 100644
--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -28,32 +28,6 @@ static inline char *strcpy(char *__dest, const char *__src)
 	return __xdest;
 }
 
-#define __HAVE_ARCH_STRNCPY
-static inline char *strncpy(char *__dest, const char *__src, size_t __n)
-{
-	register char *__xdest = __dest;
-	unsigned long __dummy;
-
-	if (__n == 0)
-		return __xdest;
-
-	__asm__ __volatile__(
-		"1:\n"
-		"mov.b	@%1+, %2\n\t"
-		"mov.b	%2, @%0\n\t"
-		"cmp/eq	#0, %2\n\t"
-		"bt/s	2f\n\t"
-		" cmp/eq	%5,%1\n\t"
-		"bf/s	1b\n\t"
-		" add	#1, %0\n"
-		"2:"
-		: "=r" (__dest), "=r" (__src), "=&z" (__dummy)
-		: "0" (__dest), "1" (__src), "r" (__src+__n)
-		: "memory", "t");
-
-	return __xdest;
-}
-
 #define __HAVE_ARCH_STRCMP
 static inline int strcmp(const char *__cs, const char *__ct)
 {
-- 
2.7.4

Re: [PATCH v2 0/3] sh: fixup strncpy()

[Yoshinori Sato]

On Thu, 19 Dec 2019 10:20:46 +0900,
Kuninori Morimoto wrote:
> 
> 
> Hi Sato-san, Rich
> 
> These are strncpy() fixup patches, but using different solutions.
> Karl's patches are updating current strncpy(), but using 2 patterns.
> Kuninori's patch is using generic strncpy().
> 
> We don't know which is the best, but we can follow
> SH maintainer's selection.
> 
> Karl Nasrallah (2):
>       sh: fixup strncpy() warning and add missing padding
>       sh: fixup strncpy() warning and add missing padding
> 
> Kuninori Morimoto (4):
>       sh: use generic strncpy()
> 
> Thank you for your help !!
> Best regards
> ---
> Kuninori Morimoto

I think the generic version is better, but I want to decide after comparing what code is generated.
If it is not very inefficient, I would like to make it a generic version.

--
Yoshinori Sato

Re: [PATCH v2 0/3] sh: fixup strncpy()

[Karl Nasrallah]

On Sat, Dec 21, 2019 at 12:42:02AM +0900, Yoshinori Sato wrote:
> On Thu, 19 Dec 2019 10:20:46 +0900,
> Kuninori Morimoto wrote:
> > 
> > 
> > Hi Sato-san, Rich
> > 
> > These are strncpy() fixup patches, but using different solutions.
> > Karl's patches are updating current strncpy(), but using 2 patterns.
> > Kuninori's patch is using generic strncpy().
> > 
> > We don't know which is the best, but we can follow
> > SH maintainer's selection.
> > 
> > Karl Nasrallah (2):
> >       sh: fixup strncpy() warning and add missing padding
> >       sh: fixup strncpy() warning and add missing padding
> > 
> > Kuninori Morimoto (4):
> >       sh: use generic strncpy()
> > 
> > Thank you for your help !!
> > Best regards
> > ---
> > Kuninori Morimoto
> 
> I think the generic version is better, but I want to decide after comparing what code is generated.
> If it is not very inefficient, I would like to make it a generic version.
> 
> --
> Yoshinori Sato

Hello,

I did some testing using the following pure C version:

static inline char *strncpy(char *__dest, const char *__src, size_t __n)
{
	register char * __xdest = __dest;
	const char * __dest_end = __dest + __n - 1;

	if (__n == 0)
		return __xdest;

	while(__dest != __dest_end)
	{
		if(!(*__src))
		{
			*__dest++ = 0;
		}
		else
		{
			*__dest++ = *__src++;
		}
	}

	return __xdest;
}

This takes 8 instructions for the while loop using Os and O2 on SH4 
under GCC 9.2.0. I challenged myself to beat GCC and was only able to
do it in 8 in-loop instructions at best. On SH2A it should be possible
to hit closer to 6, and I think it may be possible to hit 7 on SH4, but
these are the kind of numbers we are looking at.

For reference, this is what I came up with, using only instructions
common to all SH cores:

static inline char *strncpy(char *__dest, const char *__src, size_t __n)
{
	register char * __xdest = __dest;
	const char * __dest_end = __dest + __n - 1;

	if (__n == 0)
		return __xdest;

	__asm__ __volatile__(
		"strncpy_start:\n\t"
			"mov.b @%[src]+,r0\n\t"
		"strncpy_loop:\n\t"
			"cmp/eq %[dest],%[dest_end]\n\t"
			"bt.s strncpy_end\n\t"
			" mov.b r0,@%[dest]\n\t"
			"cmp/eq #0,r0\n\t"
			"bt.s strncpy_loop\n\t"
			" add #1,%[dest]\n\t"
			"bf.s strncpy_loop\n\t"
			" mov.b @%[src]+,r0\n\t"
		"strncpy_end:\n\t"
		: [dest] "+r" (__dest), [src] "+r" (__src)
		: [dest_end] "r" (__dest_end)
		: "r0","t","memory");

	return retval;
}
(In maintaining the spirit of the original work, consider these pieces of
code public domain.)

I did also discover that the m68k and xtensa architectures have similar
assembly strncpy() implementations that do not add padding.

See arch/m68k/include/asm/string.h and arch/xtensa/include/asm/string.h.

Another oddity is that it does not appear that all online documentation
notes that strncpy() is supposed to add padding if the size parameter
exceeds the string size. The official man page of strncpy(3) states it
should, but some other sources make no mention of such behavior.

Hope this helps,
Karl Nasrallah