* [PATCH] sh: use generic strncpy()
@ 2019-12-18 5:22 Kuninori Morimoto
2019-12-18 8:02 ` Geert Uytterhoeven
0 siblings, 1 reply; 5+ messages in thread
From: Kuninori Morimoto @ 2019-12-18 5:22 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker; +Cc: Linux-SH, Linux-Renesas
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)
~~~~~^~~~
In general, strncpy() should behave like below.
char dest[10];
char *src = "12345";
strncpy(dest, src, 10);
// dest = {'1', '2', '3', '4', '5',
'\0','\0','\0','\0','\0'}
But, current SH strnpy() has 2 issues.
1st is it will access to out-of-memory (= src + 10).
2nd is it needs big fixup for it, and maintenance __asm__
code is difficult.
To solve these issues, this patch simply uses generic strncpy()
instead of architecture specific one.
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] sh: use generic strncpy()
2019-12-18 5:22 [PATCH] sh: use generic strncpy() Kuninori Morimoto
@ 2019-12-18 8:02 ` Geert Uytterhoeven
2019-12-19 0:45 ` Kuninori Morimoto
0 siblings, 1 reply; 5+ messages in thread
From: Geert Uytterhoeven @ 2019-12-18 8:02 UTC (permalink / raw)
To: Kuninori Morimoto, Karl Nasrallah
Cc: Yoshinori Sato, Rich Felker, Linux-SH, Linux-Renesas,
open list:KERNEL SELFTEST FRAMEWORK
Hi Morimoto-san, Karl,
On Wed, Dec 18, 2019 at 6:22 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> 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)
> ~~~~~^~~~
>
> In general, strncpy() should behave like below.
>
> char dest[10];
> char *src = "12345";
>
> strncpy(dest, src, 10);
> // dest = {'1', '2', '3', '4', '5',
> '\0','\0','\0','\0','\0'}
>
> But, current SH strnpy() has 2 issues.
> 1st is it will access to out-of-memory (= src + 10).
I believe this is not correct: the code does not really access memory
beyond the end of the source string. (Recent) gcc just thinks so,
because "__src+__n" is used as a parameter to the routine.
> 2nd is it needs big fixup for it, and maintenance __asm__
> code is difficult.
Yeah, the padding is missing.
> To solve these issues, this patch simply uses generic strncpy()
> instead of architecture specific one.
That will definitely fix the issue, as we assume the generic
implementation is correct ;-)
Now, I've just tried, naively, to enable CONFIG_STRING_SELFTEST=y in my
rts7751r2d build (without your patch), and boot it in qemu:
String selftests succeeded
Woops, turns out lib/test_string.c does not have any testcases for
strncpy()...
So adding test code for the corner cases may be a valuable contribution.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sh: use generic strncpy()
2019-12-18 8:02 ` Geert Uytterhoeven
@ 2019-12-19 0:45 ` Kuninori Morimoto
0 siblings, 0 replies; 5+ messages in thread
From: Kuninori Morimoto @ 2019-12-19 0:45 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Karl Nasrallah, Yoshinori Sato, Rich Felker, Linux-SH,
Linux-Renesas, open list:KERNEL SELFTEST FRAMEWORK
Hi Geert
Thank you for your review
> > But, current SH strnpy() has 2 issues.
> > 1st is it will access to out-of-memory (= src + 10).
>
> I believe this is not correct: the code does not really access memory
> beyond the end of the source string. (Recent) gcc just thinks so,
> because "__src+__n" is used as a parameter to the routine.
I see.
Will fix in v2
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] sh: use generic strncpy()
2019-12-18 7:32 ` [PATCH] " Kuninori Morimoto
@ 2019-12-18 7:35 ` Kuninori Morimoto
0 siblings, 0 replies; 5+ messages in thread
From: Kuninori Morimoto @ 2019-12-18 7:35 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, Karl Nasrallah; +Cc: Linux-SH, Linux-Renesas
Hi
> 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)
> ~~~~~^~~~
>
> This patch fixup it
>
> Signed-off-by: Karl Nasrallah <knnspeed@aol.com>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
Grr, I failed Subject.
Sato-san, Rich, please let me know if you select this patch.
I can repost it with correct Subject
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] sh: use generic strncpy()
2019-12-18 7:31 [PATCH 0/2] " Kuninori Morimoto
@ 2019-12-18 7:32 ` Kuninori Morimoto
2019-12-18 7:35 ` Kuninori Morimoto
0 siblings, 1 reply; 5+ messages in thread
From: Kuninori Morimoto @ 2019-12-18 7:32 UTC (permalink / raw)
To: Yoshinori Sato, Rich Felker, Karl Nasrallah; +Cc: Linux-SH, Linux-Renesas
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)
~~~~~^~~~
This patch fixup it
Signed-off-by: Karl Nasrallah <knnspeed@aol.com>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
This is Karl's 1st pattern
arch/sh/include/asm/string_32.h | 54 ++++++++++++++++++++++++++---------------
1 file changed, 35 insertions(+), 19 deletions(-)
diff --git a/arch/sh/include/asm/string_32.h b/arch/sh/include/asm/string_32.h
index 3558b1d..d35b92f 100644
--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -31,27 +31,43 @@ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-19 0:46 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-18 5:22 [PATCH] sh: use generic strncpy() Kuninori Morimoto
2019-12-18 8:02 ` Geert Uytterhoeven
2019-12-19 0:45 ` Kuninori Morimoto
2019-12-18 7:31 [PATCH 0/2] " Kuninori Morimoto
2019-12-18 7:32 ` [PATCH] " Kuninori Morimoto
2019-12-18 7:35 ` Kuninori Morimoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).