Patchwork [3/4] sparc32: return destination pointer on return from memcpy

login
register
mail settings
Submitter Konrad Eisele
Date Oct. 13, 2011, 7 a.m.
Message ID <1318489205-315-4-git-send-email-konrad@gaisler.com>
Download mbox | patch
Permalink /patch/119361/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Konrad Eisele - Oct. 13, 2011, 7 a.m.
Gcc can optimize constant strcpy to a memcpy call. However
the return value of strcpy is used in KGDB. Return the standard return
value instead of 0

Signed-off-by: Konrad Eisele <konrad@gaisler.com>
---
 arch/sparc/lib/memcpy.S |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
David Miller - Oct. 19, 2011, 4:07 a.m.
From: Konrad Eisele <konrad@gaisler.com>
Date: Thu, 13 Oct 2011 09:00:04 +0200

> -/* In kernel these functions don't return a value.
> - * One should use macros in asm/string.h for that purpose.
> - * We return 0, so that bugs are more apparent.
> - */
> -#define SETUP_RETL
> -#define RETL_INSN	clr	%o0
> +#define SETUP_RETL	mov	%o0, %g6
> +#define RETL_INSN	mov	%g6, %o0

Sigh...

The kernel uses %g6 as the global thread register, see
include/asm/thread_info_32.h:

register struct thread_info *current_thread_info_reg asm("g6");

Your kernel would have crashed early in the boot if you actually
tested this patch.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Konrad Eisele - Oct. 19, 2011, 8:33 a.m.
David Miller wrote:
> From: Konrad Eisele <konrad@gaisler.com>
> Date: Thu, 13 Oct 2011 09:00:04 +0200
> 
>> -/* In kernel these functions don't return a value.
>> - * One should use macros in asm/string.h for that purpose.
>> - * We return 0, so that bugs are more apparent.
>> - */
>> -#define SETUP_RETL
>> -#define RETL_INSN	clr	%o0
>> +#define SETUP_RETL	mov	%o0, %g6
>> +#define RETL_INSN	mov	%g6, %o0
> 
> Sigh...
> 
> The kernel uses %g6 as the global thread register, see
> include/asm/thread_info_32.h:
> 
> register struct thread_info *current_thread_info_reg asm("g6");
> 
> Your kernel would have crashed early in the boot if you actually
> tested this patch.
> 
> 

Isnt the simplest way to apply the original gdbstub.c patch instead?

-		ptr += strlen(strcpy(ptr, "thread:"));
+		strcpy(ptr, "thread:");
+		ptr += strlen(ptr);


Or even a simple

		strcpy(ptr, "thread:");
		ptr += 7;

In that case its the
"[PATCH 4/4] kernel,debug: SPARC KGDB stub strcpy fix" from [09/30/2011 03:47 PM].
instead of the memcpy patch.

It seems to me that memcpy is tightly programmed. Is saving 2 lines of gdbstub.c
diff worth a memcpy rewrite?

-- Konrad



--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 19, 2011, 7:29 p.m.
From: Konrad Eisele <konrad@gaisler.com>
Date: Wed, 19 Oct 2011 10:33:25 +0200

> Isnt the simplest way to apply the original gdbstub.c patch instead?

It's wrong, memcpy() is not implemented properly on 32-bit sparc, fix
it.

> It seems to me that memcpy is tightly programmed. Is saving 2 lines of gdbstub.c
> diff worth a memcpy rewrite?

You really can't find another register to save the initial %o0 value in?
Have you even tried?
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/sparc/lib/memcpy.S b/arch/sparc/lib/memcpy.S
index 34fe657..835014b 100644
--- a/arch/sparc/lib/memcpy.S
+++ b/arch/sparc/lib/memcpy.S
@@ -19,12 +19,8 @@  x:
 #undef FASTER_NONALIGNED
 #define FASTER_ALIGNED
 
-/* In kernel these functions don't return a value.
- * One should use macros in asm/string.h for that purpose.
- * We return 0, so that bugs are more apparent.
- */
-#define SETUP_RETL
-#define RETL_INSN	clr	%o0
+#define SETUP_RETL	mov	%o0, %g6
+#define RETL_INSN	mov	%g6, %o0
 
 #else