can someone solve string_32.h issue for SH ?

can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi SH ML

We get too many below strncpy() warning on SH.
Can someone solve it ?
I don't remember SH assembler code / can't test it...

In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
                 from /home/morimoto/WORK/linux/include/linux/string.h:20,
                 from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
                 from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
                 from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
                 from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
                 from /home/morimoto/WORK/linux/include/linux/slab.h:15,
                 from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
/home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
/home/morimoto/WORK/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)
                                     ~~~~~^~~~

	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;
	}

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

can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi SH ML

We get too many below strncpy() warning on SH.
Can someone solve it ?
I don't remember SH assembler code / can't test it...

In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
                 from /home/morimoto/WORK/linux/include/linux/string.h:20,
                 from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
                 from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
                 from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
                 from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
                 from /home/morimoto/WORK/linux/include/linux/slab.h:15,
                 from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
/home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
/home/morimoto/WORK/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)
                                     ~~~~~^~~~

	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;
	}

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

This appears to be your culprit:

char vub_name[3 + (9 * 8) + 4 + 1];

in struct vub300_mmc_host on line 302 of the file /drivers/mmc/host/vub300.c.

This is the exact line triggering your warning, also in vub300.c:

515 strncpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name));

And the fix would be to probably fix vub300.c, do a sizeof() on the string or hardcode the string size to 26 characters since the string is already hardcoded in as it stands.

The asm doesn't look wrong.

Hope this helps!
(Sorry I can't do any pretty formatting: mail client and majordomo don't play very well together and html gets generated at even the slightest provocation, despite having disabled it...)

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl

> This appears to be your culprit:
> 
> char vub_name[3 + (9 * 8) + 4 + 1];
> 
> in struct vub300_mmc_host on line 302 of the file /drivers/mmc/host/vub300.c.
> 
> This is the exact line triggering your warning, also in vub300.c:
> 
> 515 strncpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name));
> 
> And the fix would be to probably fix vub300.c, do a sizeof() on the string or hardcode the string size to 26 characters since the string is already hardcoded in as it stands.
> 
> The asm doesn't look wrong.
> 
> Hope this helps!
> (Sorry I can't do any pretty formatting: mail client and majordomo don't play very well together and html gets generated at even the slightest provocation, despite having disabled it...)

Oops, indeed
I got too many this kind of warning on SH.
I though, it is SH side issue.
(Why I don't get same warning on other CPU.. ??)

But anyway, Thanks a lot.
I will fixup driver side !!

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl, again

> > This appears to be your culprit:
> > 
> > char vub_name[3 + (9 * 8) + 4 + 1];
> > 
> > in struct vub300_mmc_host on line 302 of the file /drivers/mmc/host/vub300.c.
> > 
> > This is the exact line triggering your warning, also in vub300.c:
> > 
> > 515 strncpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name));
> > 
> > And the fix would be to probably fix vub300.c, do a sizeof() on the string or hardcode the string size to 26 characters since the string is already hardcoded in as it stands.
> > 
> > The asm doesn't look wrong.
> > 
> > Hope this helps!
> > (Sorry I can't do any pretty formatting: mail client and majordomo don't play very well together and html gets generated at even the slightest provocation, despite having disabled it...)

Oops ??

strncpy() should behave like this,
and people expect it ?

	char s[4];

	strncpy(s, "abc", 4);
	// s = {'a', 'b', 'c', '\0'}

	strncpy(s, "abcd", 4);
	// s = {'a', 'b', 'c', 'd'}

	strncpy(s, "ab", 4);
	// s = {'a', 'b', '\0', '\0'}


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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

I'm terribly sorry; I think I made a mistake. I looked at it again, double-checked the defined behavior of strncpy, and I think you actually just need to replace __src+__n with __dest+__n in your strncpy.
Additionally, I don't know what the linux kernel rules are on this, but maybe unsigned long __dummy; should be initialized to 0 explicitly for readability?

Sorry about that!

-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: linux-sh <linux-sh@vger.kernel.org>
Sent: Tue, Dec 17, 2019 2:46 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Karl


> This appears to be your culprit:
> 
> char vub_name[3 + (9 * 8) + 4 + 1];
> 
> in struct vub300_mmc_host on line 302 of the file /drivers/mmc/host/vub300.c.
> 
> This is the exact line triggering your warning, also in vub300.c:
> 
> 515 strncpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name));
> 
> And the fix would be to probably fix vub300.c, do a sizeof() on the string or hardcode the string size to 26 characters since the string is already hardcoded in as it stands.
> 
> The asm doesn't look wrong.
> 
> Hope this helps!
> (Sorry I can't do any pretty formatting: mail client and majordomo don't play very well together and html gets generated at even the slightest provocation, despite having disabled it...)


Oops, indeed
I got too many this kind of warning on SH.
I though, it is SH side issue.
(Why I don't get same warning on other CPU.. ??)

But anyway, Thanks a lot.
I will fixup driver side !!

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Well, this will teach me to try and solve a problem at 3 AM. Please ignore my last message.

strncpy behaves like this: linux (dot) die (dot) net/man/3/strncpy

Which is exactly what you just wrote. So yes, strncpy is expected to write null bytes when the source is shorter than n.

-----Original Message-----
From: Karl Nasrallah <knnspeed@aol.com>
To: kuninori.morimoto.gx <kuninori.morimoto.gx@renesas.com>
Cc: linux-sh <linux-sh@vger.kernel.org>
Sent: Tue, Dec 17, 2019 3:15 am
Subject: Re: can someone solve string_32.h issue for SH ?


Hello,

I'm terribly sorry; I think I made a mistake. I looked at it again, double-checked the defined behavior of strncpy, and I think you actually just need to replace __src+__n with __dest+__n in your strncpy.
Additionally, I don't know what the linux kernel rules are on this, but maybe unsigned long __dummy; should be initialized to 0 explicitly for readability?

Sorry about that!


-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: linux-sh <linux-sh@vger.kernel.org>
Sent: Tue, Dec 17, 2019 2:46 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Karl


> This appears to be your culprit:
> 
> char vub_name[3 + (9 * 8) + 4 + 1];
> 
> in struct vub300_mmc_host on line 302 of the file /drivers/mmc/host/vub300.c.
> 
> This is the exact line triggering your warning, also in vub300.c:
> 
> 515 strncpy(vub300->vub_name, "EMPTY Processing Disabled", sizeof(vub300->vub_name));
> 
> And the fix would be to probably fix vub300.c, do a sizeof() on the string or hardcode the string size to 26 characters since the string is already hardcoded in as it stands.
> 
> The asm doesn't look wrong.
> 
> Hope this helps!
> (Sorry I can't do any pretty formatting: mail client and majordomo don't play very well together and html gets generated at even the slightest provocation, despite having disabled it...)


Oops, indeed
I got too many this kind of warning on SH.
I though, it is SH side issue.
(Why I don't get same warning on other CPU.. ??)

But anyway, Thanks a lot.
I will fixup driver side !!

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> We get too many below strncpy() warning on SH.
> Can someone solve it ?
> I don't remember SH assembler code / can't test it...

I never touched SH assembler code at all.
But it looks a bit like RISCified m68k, so let's give it a try ;-)

> In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
>                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
>                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
>                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
>                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
>                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
>                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
>                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> /home/morimoto/WORK/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)
>                                      ~~~~~^~~~

Yeah, these array warnings are (sometimes) a PITA.

>         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;
>         }


My first thought was to just replace "__src+__n" by "__dest+__n", and
change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
However, "%0" isn't incremented until the branch delay slot of the loop.
So I had to move the increment up, and fill the branch delay slot with a nop.

Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:

--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
char *__src, size_t __n)
        __asm__ __volatile__(
                "1:\n"
                "mov.b  @%1+, %2\n\t"
-               "mov.b  %2, @%0\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"
+               " cmp/eq        %5,%0\n\t"
                "bf/s   1b\n\t"
-               " add   #1, %0\n"
+               " nop\n"
                "2:"
                : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
-               : "0" (__dest), "1" (__src), "r" (__src+__n)
+               : "0" (__dest), "1" (__src), "r" (__dest+__n)
                : "memory", "t");

        return __xdest;

Does this make sense?
Can it be improved, by putting something useful in the delay slot?

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> We get too many below strncpy() warning on SH.
> Can someone solve it ?
> I don't remember SH assembler code / can't test it...

I never touched SH assembler code at all.
But it looks a bit like RISCified m68k, so let's give it a try ;-)

> In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
>                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
>                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
>                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
>                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
>                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
>                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
>                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> /home/morimoto/WORK/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)
>                                      ~~~~~^~~~

Yeah, these array warnings are (sometimes) a PITA.

>         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;
>         }


My first thought was to just replace "__src+__n" by "__dest+__n", and
change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
However, "%0" isn't incremented until the branch delay slot of the loop.
So I had to move the increment up, and fill the branch delay slot with a nop.

Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:

--- a/arch/sh/include/asm/string_32.h
+++ b/arch/sh/include/asm/string_32.h
@@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
char *__src, size_t __n)
        __asm__ __volatile__(
                "1:\n"
                "mov.b  @%1+, %2\n\t"
-               "mov.b  %2, @%0\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"
+               " cmp/eq        %5,%0\n\t"
                "bf/s   1b\n\t"
-               " add   #1, %0\n"
+               " nop\n"
                "2:"
                : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
-               : "0" (__dest), "1" (__src), "r" (__src+__n)
+               : "0" (__dest), "1" (__src), "r" (__dest+__n)
                : "memory", "t");

        return __xdest;

Does this make sense?
Can it be improved, by putting something useful in the delay slot?

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Geert

Thank you for your feedback

> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\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"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
> 
>         return __xdest;
> 
> Does this make sense?
> Can it be improved, by putting something useful in the delay slot?

Unfortunately, I can't test it :(
Can someone test it ?

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Geert

Thank you for your feedback

> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\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"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
> 
>         return __xdest;
> 
> Does this make sense?
> Can it be improved, by putting something useful in the delay slot?

Unfortunately, I can't test it :(
Can someone test it ?

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 9:37 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Unfortunately, I can't test it :(
> Can someone test it ?

make rts7751r2dplus_defconfig
qemu-system-sh4 qemu-system-sh4 -M r2d -serial null -serial mon:stdio \
        -nographic -no-reboot -m 256 \
        -append "panic=1 HOST=sh4 console=ttySC1 noiotrap" \
        -kernel arch/sh/boot/zImage

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 9:37 AM Kuninori Morimoto
<kuninori.morimoto.gx@renesas.com> wrote:
> Unfortunately, I can't test it :(
> Can someone test it ?

make rts7751r2dplus_defconfig
qemu-system-sh4 qemu-system-sh4 -M r2d -serial null -serial mon:stdio \
        -nographic -no-reboot -m 256 \
        -append "panic=1 HOST=sh4 console=ttySC1 noiotrap" \
        -kernel arch/sh/boot/zImage

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 9:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> > We get too many below strncpy() warning on SH.
> > Can someone solve it ?
> > I don't remember SH assembler code / can't test it...
>
> I never touched SH assembler code at all.
> But it looks a bit like RISCified m68k, so let's give it a try ;-)
>
> > In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
> >                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
> >                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
> >                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
> >                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
> >                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
> >                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
> >                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> > /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> > /home/morimoto/WORK/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)
> >                                      ~~~~~^~~~
>
> Yeah, these array warnings are (sometimes) a PITA.
>
> >         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;
> >         }
>
>
> My first thought was to just replace "__src+__n" by "__dest+__n", and
> change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
> However, "%0" isn't incremented until the branch delay slot of the loop.
> So I had to move the increment up, and fill the branch delay slot with a nop.
>
> Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:
>
> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\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"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
>
>         return __xdest;
>
> Does this make sense?
> Can it be improved, by putting something useful in the delay slot?

BTW, there seems to be a serious security issue with this strncpy()
implementation: while it never writes more than n bytes in the
destination buffer, it doesn't pad the destination buffer with zeroes if
the source string is shorter than the buffer size.  This will leak
data.

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

Hi Morimoto-san,

On Tue, Dec 17, 2019 at 9:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> > We get too many below strncpy() warning on SH.
> > Can someone solve it ?
> > I don't remember SH assembler code / can't test it...
>
> I never touched SH assembler code at all.
> But it looks a bit like RISCified m68k, so let's give it a try ;-)
>
> > In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
> >                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
> >                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
> >                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
> >                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
> >                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
> >                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
> >                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> > /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> > /home/morimoto/WORK/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)
> >                                      ~~~~~^~~~
>
> Yeah, these array warnings are (sometimes) a PITA.
>
> >         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;
> >         }
>
>
> My first thought was to just replace "__src+__n" by "__dest+__n", and
> change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
> However, "%0" isn't incremented until the branch delay slot of the loop.
> So I had to move the increment up, and fill the branch delay slot with a nop.
>
> Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:
>
> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\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"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
>
>         return __xdest;
>
> Does this make sense?
> Can it be improved, by putting something useful in the delay slot?

BTW, there seems to be a serious security issue with this strncpy()
implementation: while it never writes more than n bytes in the
destination buffer, it doesn't pad the destination buffer with zeroes if
the source string is shorter than the buffer size.  This will leak
data.

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

Give me a day or so and I can do the following things:

1) Write you all a brand new standards-conforming strncpy in SH4 asm like this that is easier to read
2) Compile it with sh4-elf-GCC 9.2
3) Test it on a real SH4 (SH7750/SH7750R-like) 

The warning, if it shows up in my test, would likely then be a GCC thing--I have an idea of what it's doing, but I'll be sure after that.
Unfortunately it's 4AM here on the other side of the world right now...
-Karl


-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>; Rich Felker <dalias@libc.org>; Linux-SH <linux-sh@vger.kernel.org>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 3:51 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

Thank you for your help !!
Best regards

---
Kuninori Morimoto

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

Give me a day or so and I can do the following things:

1) Write you all a brand new standards-conforming strncpy in SH4 asm like this that is easier to read
2) Compile it with sh4-elf-GCC 9.2
3) Test it on a real SH4 (SH7750/SH7750R-like) 

The warning, if it shows up in my test, would likely then be a GCC thing--I have an idea of what it's doing, but I'll be sure after that.
Unfortunately it's 4AM here on the other side of the world right now...
-Karl


-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>; Rich Felker <dalias@libc.org>; Linux-SH <linux-sh@vger.kernel.org>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 3:51 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

Thank you for your help !!
Best regards

---
Kuninori Morimoto

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello!

I have a strncpy for you.

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

	// size_t is always unsigned
	if(__n == 0)
	{
		return retval;
	}

	__asm__ __volatile__ (
					"strncpy_start:\n\t"
							"mov.b @%[src]+,r0\n\t"
							"cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
							"bt.s strncpy_pad\n\t" // Done with the string
							"cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
							"bt.s strncpy_end\n\t"
							"mov.b r0,@%[dest]\n\t"
							"bra strncpy_start\n\t"
							"add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
					"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" // All done
		: [src] "+r" (__src), [dest] "+r" (__dest)
		: [dest_end] "r" (__dest_end)
		: "t"
	);

	return retval;
}

Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible system. No warnings, behaves exactly as per linux (dot) die (dot) net/man/3/strncpy and I optimized it with some tricks I devised from writing extremely optimized x86. If there are any doubts as to the authenticity, note that I am the sole author of this project: github (dot) com/KNNSpeed/AVX-Memmove

Hope this helps!
-Karl

(P.S. Consider this code public domain. If for whatever reason that doesn't fly, then I give the Linux kernel community explicit permission to use it as they see fit.)

-----Original Message-----
From: Karl Nasrallah <knnspeed@aol.com>
To: kuninori.morimoto.gx <kuninori.morimoto.gx@renesas.com>; geert <geert@linux-m68k.org>
Cc: ysato <ysato@users.sourceforge.jp>; dalias <dalias@libc.org>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 4:09 am
Subject: Re: can someone solve string_32.h issue for SH ?


Hello,

Give me a day or so and I can do the following things:

1) Write you all a brand new standards-conforming strncpy in SH4 asm like this that is easier to read
2) Compile it with sh4-elf-GCC 9.2
3) Test it on a real SH4 (SH7750/SH7750R-like) 

The warning, if it shows up in my test, would likely then be a GCC thing--I have an idea of what it's doing, but I'll be sure after that.
Unfortunately it's 4AM here on the other side of the world right now...
-Karl



-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>; Rich Felker <dalias@libc.org>; Linux-SH <linux-sh@vger.kernel.org>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 3:51 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

Thank you for your help !!
Best regards

---
Kuninori Morimoto

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello!

I have a strncpy for you.

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

	// size_t is always unsigned
	if(__n == 0)
	{
		return retval;
	}

	__asm__ __volatile__ (
					"strncpy_start:\n\t"
							"mov.b @%[src]+,r0\n\t"
							"cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
							"bt.s strncpy_pad\n\t" // Done with the string
							"cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
							"bt.s strncpy_end\n\t"
							"mov.b r0,@%[dest]\n\t"
							"bra strncpy_start\n\t"
							"add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
					"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" // All done
		: [src] "+r" (__src), [dest] "+r" (__dest)
		: [dest_end] "r" (__dest_end)
		: "t"
	);

	return retval;
}

Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible system. No warnings, behaves exactly as per linux (dot) die (dot) net/man/3/strncpy and I optimized it with some tricks I devised from writing extremely optimized x86. If there are any doubts as to the authenticity, note that I am the sole author of this project: github (dot) com/KNNSpeed/AVX-Memmove

Hope this helps!
-Karl

(P.S. Consider this code public domain. If for whatever reason that doesn't fly, then I give the Linux kernel community explicit permission to use it as they see fit.)

-----Original Message-----
From: Karl Nasrallah <knnspeed@aol.com>
To: kuninori.morimoto.gx <kuninori.morimoto.gx@renesas.com>; geert <geert@linux-m68k.org>
Cc: ysato <ysato@users.sourceforge.jp>; dalias <dalias@libc.org>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 4:09 am
Subject: Re: can someone solve string_32.h issue for SH ?


Hello,

Give me a day or so and I can do the following things:

1) Write you all a brand new standards-conforming strncpy in SH4 asm like this that is easier to read
2) Compile it with sh4-elf-GCC 9.2
3) Test it on a real SH4 (SH7750/SH7750R-like) 

The warning, if it shows up in my test, would likely then be a GCC thing--I have an idea of what it's doing, but I'll be sure after that.
Unfortunately it's 4AM here on the other side of the world right now...
-Karl



-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>; Rich Felker <dalias@libc.org>; Linux-SH <linux-sh@vger.kernel.org>; Linux-Renesas <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 3:51 am
Subject: Re: can someone solve string_32.h issue for SH ?



Hi Geert
Cc Yoshinori-san

> > --- a/arch/sh/include/asm/string_32.h
> > +++ b/arch/sh/include/asm/string_32.h
> > @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> > char *__src, size_t __n)
> >         __asm__ __volatile__(
> >                 "1:\n"
> >                 "mov.b  @%1+, %2\n\t"
> > -               "mov.b  %2, @%0\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"
> > +               " cmp/eq        %5,%0\n\t"
> >                 "bf/s   1b\n\t"
> > -               " add   #1, %0\n"
> > +               " nop\n"
> >                 "2:"
> >                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> > -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> > +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
> >                 : "memory", "t");
> >
> >         return __xdest;
> >
> > Does this make sense?
> > Can it be improved, by putting something useful in the delay slot?
> 
> BTW, there seems to be a serious security issue with this strncpy()
> implementation: while it never writes more than n bytes in the
> destination buffer, it doesn't pad the destination buffer with zeroes if
> the source string is shorter than the buffer size.  This will leak
> data.

Yeah...
I can only do is "Reporting issue" to SH ML, unfortunately...

Thank you for your help !!
Best regards

---
Kuninori Morimoto

Re: can someone solve string_32.h issue for SH ?

[Rich Felker]

On Tue, Dec 17, 2019 at 10:16:28PM +0000, Karl Nasrallah wrote:
> Hello!
> 
> I have a strncpy for you.
> 
> static inline char *strncpy(char *__dest, const char *__src, size_t __n)
> {
> 	char * retval = __dest;
> 	const char * __dest_end = __dest + __n - 1;
> 
> 	// size_t is always unsigned
> 	if(__n = 0)
> 	{
> 		return retval;
> 	}
> 
> 	__asm__ __volatile__ (
> 					"strncpy_start:\n\t"
> 							"mov.b @%[src]+,r0\n\t"
> 							"cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
> 							"bt.s strncpy_pad\n\t" // Done with the string
> 							"cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
> 							"bt.s strncpy_end\n\t"
> 							"mov.b r0,@%[dest]\n\t"
> 							"bra strncpy_start\n\t"
> 							"add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
> 					"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" // All done
> 		: [src] "+r" (__src), [dest] "+r" (__dest)
> 		: [dest_end] "r" (__dest_end)
> 		: "t"
> 	);
> 
> 	return retval;
> }
> 
> Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible
> system. No warnings, behaves exactly as per linux (dot) die (dot)
> net/man/3/strncpy and I optimized it with some tricks I devised from
> writing extremely optimized x86. If there are any doubts as to the
> authenticity, note that I am the sole author of this project: github
> (dot) com/KNNSpeed/AVX-Memmove

You're using r0 explicitly in the asm but I don't see where you're
reserving it for your use. You need it either on the clobbers or bound
to a dummy output with earlyclobber.

Rich

Re: can someone solve string_32.h issue for SH ?

[Rich Felker]

On Tue, Dec 17, 2019 at 10:16:28PM +0000, Karl Nasrallah wrote:
> Hello!
> 
> I have a strncpy for you.
> 
> static inline char *strncpy(char *__dest, const char *__src, size_t __n)
> {
> 	char * retval = __dest;
> 	const char * __dest_end = __dest + __n - 1;
> 
> 	// size_t is always unsigned
> 	if(__n == 0)
> 	{
> 		return retval;
> 	}
> 
> 	__asm__ __volatile__ (
> 					"strncpy_start:\n\t"
> 							"mov.b @%[src]+,r0\n\t"
> 							"cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
> 							"bt.s strncpy_pad\n\t" // Done with the string
> 							"cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
> 							"bt.s strncpy_end\n\t"
> 							"mov.b r0,@%[dest]\n\t"
> 							"bra strncpy_start\n\t"
> 							"add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
> 					"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" // All done
> 		: [src] "+r" (__src), [dest] "+r" (__dest)
> 		: [dest_end] "r" (__dest_end)
> 		: "t"
> 	);
> 
> 	return retval;
> }
> 
> Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible
> system. No warnings, behaves exactly as per linux (dot) die (dot)
> net/man/3/strncpy and I optimized it with some tricks I devised from
> writing extremely optimized x86. If there are any doubts as to the
> authenticity, note that I am the sole author of this project: github
> (dot) com/KNNSpeed/AVX-Memmove

You're using r0 explicitly in the asm but I don't see where you're
reserving it for your use. You need it either on the clobbers or bound
to a dummy output with earlyclobber.

Rich

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

On Tue, Dec 17, 2019 at 9:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> > We get too many below strncpy() warning on SH.
> > Can someone solve it ?
> > I don't remember SH assembler code / can't test it...
>
> I never touched SH assembler code at all.
> But it looks a bit like RISCified m68k, so let's give it a try ;-)
>
> > In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
> >                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
> >                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
> >                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
> >                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
> >                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
> >                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
> >                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> > /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> > /home/morimoto/WORK/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)
> >                                      ~~~~~^~~~
>
> Yeah, these array warnings are (sometimes) a PITA.
>
> >         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;
> >         }
>
>
> My first thought was to just replace "__src+__n" by "__dest+__n", and
> change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
> However, "%0" isn't incremented until the branch delay slot of the loop.
> So I had to move the increment up, and fill the branch delay slot with a nop.
>
> Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:
>
> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\n\t"
> +               "mov.b  %2, @%0+\n\t"

While this compiles for SH2, it doesn't for SH4 :-(

>                 "cmp/eq #0, %2\n\t"
>                 "bt/s   2f\n\t"
> -               " cmp/eq        %5,%1\n\t"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
>
>         return __xdest;

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

Re: can someone solve string_32.h issue for SH ?

[Geert Uytterhoeven]

On Tue, Dec 17, 2019 at 9:29 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, Dec 17, 2019 at 7:09 AM Kuninori Morimoto
> <kuninori.morimoto.gx@renesas.com> wrote:
> > We get too many below strncpy() warning on SH.
> > Can someone solve it ?
> > I don't remember SH assembler code / can't test it...
>
> I never touched SH assembler code at all.
> But it looks a bit like RISCified m68k, so let's give it a try ;-)
>
> > In file included from /home/morimoto/WORK/linux/arch/sh/include/asm/string.h:3,
> >                  from /home/morimoto/WORK/linux/include/linux/string.h:20,
> >                  from /home/morimoto/WORK/linux/include/linux/bitmap.h:9,
> >                  from /home/morimoto/WORK/linux/include/linux/nodemask.h:95,
> >                  from /home/morimoto/WORK/linux/include/linux/mmzone.h:17,
> >                  from /home/morimoto/WORK/linux/include/linux/gfp.h:6,
> >                  from /home/morimoto/WORK/linux/include/linux/slab.h:15,
> >                  from /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c:38:
> > /home/morimoto/WORK/linux/drivers/mmc/host/vub300.c: In function 'new_system_port_status':
> > /home/morimoto/WORK/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)
> >                                      ~~~~~^~~~
>
> Yeah, these array warnings are (sometimes) a PITA.
>
> >         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;
> >         }
>
>
> My first thought was to just replace "__src+__n" by "__dest+__n", and
> change the "cmp/eq" from "%1" (current src) to "%0" (current dst).
> However, "%0" isn't incremented until the branch delay slot of the loop.
> So I had to move the increment up, and fill the branch delay slot with a nop.
>
> Untested (it-compiles-so-it-must-be-perfect ;-) whitespace-damaged patch:
>
> --- a/arch/sh/include/asm/string_32.h
> +++ b/arch/sh/include/asm/string_32.h
> @@ -40,15 +40,15 @@ static inline char *strncpy(char *__dest, const
> char *__src, size_t __n)
>         __asm__ __volatile__(
>                 "1:\n"
>                 "mov.b  @%1+, %2\n\t"
> -               "mov.b  %2, @%0\n\t"
> +               "mov.b  %2, @%0+\n\t"

While this compiles for SH2, it doesn't for SH4 :-(

>                 "cmp/eq #0, %2\n\t"
>                 "bt/s   2f\n\t"
> -               " cmp/eq        %5,%1\n\t"
> +               " cmp/eq        %5,%0\n\t"
>                 "bf/s   1b\n\t"
> -               " add   #1, %0\n"
> +               " nop\n"
>                 "2:"
>                 : "=r" (__dest), "=r" (__src), "=&z" (__dummy)
> -               : "0" (__dest), "1" (__src), "r" (__src+__n)
> +               : "0" (__dest), "1" (__src), "r" (__dest+__n)
>                 : "memory", "t");
>
>         return __xdest;

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hi Rich,

Thanks for the feedback. I've amended (and tested) it in two possible ways:

First:

static inline char *strncpy(char *__dest, const char *__src, size_t __n)
{
	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 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;
}

Second:

static inline char *sh_strncpy(char *__dest, const char *__src, size_t __n)
{
	char * retval = __dest;
	const char * __dest_end = __dest + __n - 1;

	/* size_t is always unsigned */
	if(__n == 0)
	{
		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;
}

I assume that a "memory" clobber would also be appropriate here?

I was unaware that explicitly using a register in extended asm meant that it would need to be listed in the clobber list or otherwise reserved. Guess I've been doing it wrong for a while!

By the way, thank you for adding -static-pie to GCC & binutils. It's been incredibly useful in writing bare-metal code for x86!
-Karl

-----Original Message-----
From: Rich Felker <dalias@libc.org>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: kuninori.morimoto.gx <kuninori.morimoto.gx@renesas.com>; geert <geert@linux-m68k.org>; ysato <ysato@users.sourceforge.jp>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 6:13 pm
Subject: Re: can someone solve string_32.h issue for SH ?

On Tue, Dec 17, 2019 at 10:16:28PM +0000, Karl Nasrallah wrote:
> Hello!
> 
> I have a strncpy for you.
> 
> static inline char *strncpy(char *__dest, const char *__src, size_t __n)
> {
>     char * retval = __dest;
>     const char * __dest_end = __dest + __n - 1;
> 
>     // size_t is always unsigned
>     if(__n == 0)
>     {
>         return retval;
>     }
> 
>     __asm__ __volatile__ (
>                     "strncpy_start:\n\t"
>                             "mov.b @%[src]+,r0\n\t"
>                             "cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
>                             "bt.s strncpy_pad\n\t" // Done with the string
>                             "cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
>                             "bt.s strncpy_end\n\t"
>                             "mov.b r0,@%[dest]\n\t"
>                             "bra strncpy_start\n\t"
>                             "add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
>                     "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" // All done
>         : [src] "+r" (__src), [dest] "+r" (__dest)
>         : [dest_end] "r" (__dest_end)
>         : "t"
>     );
> 
>     return retval;
> }
> 
> Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible
> system. No warnings, behaves exactly as per linux (dot) die (dot)
> net/man/3/strncpy and I optimized it with some tricks I devised from
> writing extremely optimized x86. If there are any doubts as to the
> authenticity, note that I am the sole author of this project: github
> (dot) com/KNNSpeed/AVX-Memmove

You're using r0 explicitly in the asm but I don't see where you're
reserving it for your use. You need it either on the clobbers or bound
to a dummy output with earlyclobber.


Rich

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hi Rich,

Thanks for the feedback. I've amended (and tested) it in two possible ways:

First:

static inline char *strncpy(char *__dest, const char *__src, size_t __n)
{
	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 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;
}

Second:

static inline char *sh_strncpy(char *__dest, const char *__src, size_t __n)
{
	char * retval = __dest;
	const char * __dest_end = __dest + __n - 1;

	/* size_t is always unsigned */
	if(__n == 0)
	{
		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;
}

I assume that a "memory" clobber would also be appropriate here?

I was unaware that explicitly using a register in extended asm meant that it would need to be listed in the clobber list or otherwise reserved. Guess I've been doing it wrong for a while!

By the way, thank you for adding -static-pie to GCC & binutils. It's been incredibly useful in writing bare-metal code for x86!
-Karl

-----Original Message-----
From: Rich Felker <dalias@libc.org>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: kuninori.morimoto.gx <kuninori.morimoto.gx@renesas.com>; geert <geert@linux-m68k.org>; ysato <ysato@users.sourceforge.jp>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 6:13 pm
Subject: Re: can someone solve string_32.h issue for SH ?

On Tue, Dec 17, 2019 at 10:16:28PM +0000, Karl Nasrallah wrote:
> Hello!
> 
> I have a strncpy for you.
> 
> static inline char *strncpy(char *__dest, const char *__src, size_t __n)
> {
>     char * retval = __dest;
>     const char * __dest_end = __dest + __n - 1;
> 
>     // size_t is always unsigned
>     if(__n == 0)
>     {
>         return retval;
>     }
> 
>     __asm__ __volatile__ (
>                     "strncpy_start:\n\t"
>                             "mov.b @%[src]+,r0\n\t"
>                             "cmp/eq #0,r0\n\t" // cmp/eq #imm8,r0 is its own instruction
>                             "bt.s strncpy_pad\n\t" // Done with the string
>                             "cmp/eq %[dest],%[dest_end]\n\t" // This takes care of the size parameter in only one instruction ;)
>                             "bt.s strncpy_end\n\t"
>                             "mov.b r0,@%[dest]\n\t"
>                             "bra strncpy_start\n\t"
>                             "add #1,%[dest]\n\t" // mov.b R0,@Rn+ is SH2A only, but we can fill the delay slot with the offset
>                     "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" // All done
>         : [src] "+r" (__src), [dest] "+r" (__dest)
>         : [dest_end] "r" (__dest_end)
>         : "t"
>     );
> 
>     return retval;
> }
> 
> Tested with sh4-elf-gcc 9.2.0 on a real SH7750/SH7750R-compatible
> system. No warnings, behaves exactly as per linux (dot) die (dot)
> net/man/3/strncpy and I optimized it with some tricks I devised from
> writing extremely optimized x86. If there are any doubts as to the
> authenticity, note that I am the sole author of this project: github
> (dot) com/KNNSpeed/AVX-Memmove

You're using r0 explicitly in the asm but I don't see where you're
reserving it for your use. You need it either on the clobbers or bound
to a dummy output with earlyclobber.


Rich

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi

> You're using r0 explicitly in the asm but I don't see where you're
> reserving it for your use. You need it either on the clobbers or bound
> to a dummy output with earlyclobber.
(snip)
> 	__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"
> 	);

Or, can we use general strncpy() instead of SH assemble one ?

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi

> You're using r0 explicitly in the asm but I don't see where you're
> reserving it for your use. You need it either on the clobbers or bound
> to a dummy output with earlyclobber.
(snip)
> 	__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"
> 	);

Or, can we use general strncpy() instead of SH assemble one ?

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm and has been there for a very long time. In fact, it appears that x86 also has similar utility functions defined in inline assembly: see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason that they are in inline assembly--optimization would be my guess--and converting it all to C may require an overhaul of the string.h backend (something I would not be much help with given that I am unable to get modern Linux booting on my machine, which is a Sega Dreamcast). I also do not know what the performance implications of switching it all to C would be, if there even are any.

Hopefully this information may be useful and my asm version is alright for the time being; I have not been able to unearth much more on the topic of why this is structured the way it is,
-Karl Nasrallah

-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: dalias <dalias@libc.org>; geert <geert@linux-m68k.org>; ysato <ysato@users.sourceforge.jp>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 9:01 pm
Subject: Re: can someone solve string_32.h issue for SH ?



Hi

> You're using r0 explicitly in the asm but I don't see where you're
> reserving it for your use. You need it either on the clobbers or bound
> to a dummy output with earlyclobber.
(snip)

>     __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"

>     );

Or, can we use general strncpy() instead of SH assemble one ?

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hello,

Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm and has been there for a very long time. In fact, it appears that x86 also has similar utility functions defined in inline assembly: see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason that they are in inline assembly--optimization would be my guess--and converting it all to C may require an overhaul of the string.h backend (something I would not be much help with given that I am unable to get modern Linux booting on my machine, which is a Sega Dreamcast). I also do not know what the performance implications of switching it all to C would be, if there even are any.

Hopefully this information may be useful and my asm version is alright for the time being; I have not been able to unearth much more on the topic of why this is structured the way it is,
-Karl Nasrallah

-----Original Message-----
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Karl Nasrallah <knnspeed@aol.com>
Cc: dalias <dalias@libc.org>; geert <geert@linux-m68k.org>; ysato <ysato@users.sourceforge.jp>; linux-sh <linux-sh@vger.kernel.org>; linux-renesas-soc <linux-renesas-soc@vger.kernel.org>
Sent: Tue, Dec 17, 2019 9:01 pm
Subject: Re: can someone solve string_32.h issue for SH ?



Hi

> You're using r0 explicitly in the asm but I don't see where you're
> reserving it for your use. You need it either on the clobbers or bound
> to a dummy output with earlyclobber.
(snip)

>     __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"

>     );

Or, can we use general strncpy() instead of SH assemble one ?

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl

Thank you for your help

> Or, can we use general strncpy() instead of SH assemble one ?
(snip)
> Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm
> and has been there for a very long time.
> In fact, it appears that x86 also has similar utility functions defined in inline assembly:
> see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason
> that they are in inline assembly--optimization would be my guess--and converting it all to C may require>
> an overhaul of the string.h backend (something I would not be much help with given that I am unable to get
> modern Linux booting on my machine, which is a Sega Dreamcast).
> I also do not know what the performance implications of switching it all to C would be, if there even are any.
>
> Hopefully this information may be useful and my asm version is alright for the time being;
> I have not been able to unearth much more on the topic of why this is structured the way it is,

How about this ?
You / I post each patch
(= Me:  use generic strnpy() patch
   You: fixup SH strnpy() patch)
And follow SH maintainer's judge

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl

Thank you for your help

> Or, can we use general strncpy() instead of SH assemble one ?
(snip)
> Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm
> and has been there for a very long time.
> In fact, it appears that x86 also has similar utility functions defined in inline assembly:
> see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason
> that they are in inline assembly--optimization would be my guess--and converting it all to C may require>
> an overhaul of the string.h backend (something I would not be much help with given that I am unable to get
> modern Linux booting on my machine, which is a Sega Dreamcast).
> I also do not know what the performance implications of switching it all to C would be, if there even are any.
>
> Hopefully this information may be useful and my asm version is alright for the time being;
> I have not been able to unearth much more on the topic of why this is structured the way it is,

How about this ?
You / I post each patch
(= Me:  use generic strnpy() patch
   You: fixup SH strnpy() patch)
And follow SH maintainer's judge

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

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hi,

> Hi Karl
>
> Thank you for your help
>
> > Or, can we use general strncpy() instead of SH assemble one ?
>(snip)
> > Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm
> > and has been there for a very long time.
> > In fact, it appears that x86 also has similar utility functions defined in inline assembly:
> > see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason
> > that they are in inline assembly--optimization would be my guess--and converting it all to C may require>
> > an overhaul of the string.h backend (something I would not be much help with given that I am unable to get
> > modern Linux booting on my machine, which is a Sega Dreamcast).
> > I also do not know what the performance implications of switching it all to C would be, if there even are any.
> >
> > Hopefully this information may be useful and my asm version is alright for the time being;
> > I have not been able to unearth much more on the topic of why this is structured the way it is,
>
> How about this ?
> You / I post each patch
> (= Me:  use generic strnpy() patch
>   You: fixup SH strnpy() patch)
> And follow SH maintainer's judge

That sounds fine to me!

However, I may need to find a better way to send e-mails than via this webclient before doing so. It would appear I've been mucking up pretty badly with reply formatting (sorry!!! I don't know how to fix this thread in the mailing list archives... I had no idea that replying like a normal e-mail was a terrible thing to do!!).

-Karl Nasrallah

Re: can someone solve string_32.h issue for SH ?

[Karl Nasrallah]

Hi,

> Hi Karl
>
> Thank you for your help
>
> > Or, can we use general strncpy() instead of SH assemble one ?
>(snip)
> > Though ultimately I do not have a say in that, it would appear that string_32.h is in arch/sh/include/asm
> > and has been there for a very long time.
> > In fact, it appears that x86 also has similar utility functions defined in inline assembly:
> > see arch/x86/include/asm. As straightforward as it would be to make C versions, there may be a reason
> > that they are in inline assembly--optimization would be my guess--and converting it all to C may require>
> > an overhaul of the string.h backend (something I would not be much help with given that I am unable to get
> > modern Linux booting on my machine, which is a Sega Dreamcast).
> > I also do not know what the performance implications of switching it all to C would be, if there even are any.
> >
> > Hopefully this information may be useful and my asm version is alright for the time being;
> > I have not been able to unearth much more on the topic of why this is structured the way it is,
>
> How about this ?
> You / I post each patch
> (= Me:  use generic strnpy() patch
>   You: fixup SH strnpy() patch)
> And follow SH maintainer's judge

That sounds fine to me!

However, I may need to find a better way to send e-mails than via this webclient before doing so. It would appear I've been mucking up pretty badly with reply formatting (sorry!!! I don't know how to fix this thread in the mailing list archives... I had no idea that replying like a normal e-mail was a terrible thing to do!!).

-Karl Nasrallah

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl

> > How about this ?
> > You / I post each patch
> > (= Me:  use generic strnpy() patch
> >   You: fixup SH strnpy() patch)
> > And follow SH maintainer's judge
> 
> That sounds fine to me!
> 
> However, I may need to find a better way to send e-mails than via this webclient before doing so.

OK. I can post your patch
Please check it.

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

Re: can someone solve string_32.h issue for SH ?

[Kuninori Morimoto]

Hi Karl

> > How about this ?
> > You / I post each patch
> > (= Me:  use generic strnpy() patch
> >   You: fixup SH strnpy() patch)
> > And follow SH maintainer's judge
> 
> That sounds fine to me!
> 
> However, I may need to find a better way to send e-mails than via this webclient before doing so.

OK. I can post your patch
Please check it.

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