Hi, The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce: int main() { char array[256]; memset(array, -5, 256); for (int i = 0; i < 256; ++i) { printf("%d, ", (int)array[i]); } return 0; } This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The other implementations that I checked (aarch64 and x86_64) do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset. With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it. How to fix/patch: In this file: https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/string/arm/memse... Before line 35 (lsl) insert this: uxtb r1, r1 Before line 71 (orr) insert this: uxtb a2, a2 Best regards, Tom
Hi Tom, tombannink@gmail.com wrote,
Hi,
The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce:
int main() { char array[256]; memset(array, -5, 256);
for (int i = 0; i < 256; ++i) { printf("%d, ", (int)array[i]); } return 0; }
This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The other implementations that I checked (aarch64 and x86_64) do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset.
With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it.
How to fix/patch:
In this file: https://cgit.uclibc-ng.org/cgi/cgit/uclibc-ng.git/tree/libc/string/arm/memse...
Before line 35 (lsl) insert this: uxtb r1, r1 Before line 71 (orr) insert this: uxtb a2, a2
Can you provide a git format-patch for testing? A similar bug was reported by Paul. best regards Waldemar
From 384c7efdecedaa12d195cce4a45b57d998a5de1d Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation --- libc/string/arm/memset.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 412270f50..4caa08328 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f + uxtb r1, r1 lsl r3, r1, #8 orr r1, r3 lsl r3, r1, #16 @@ -68,6 +69,7 @@ memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? blo 2f + uxtb a2, a2 orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: -- 2.35.1
It looks like I mixed up some tabs/spaces. Here is a patch where the tabs are consistent with the existing code: From 5e0845fc2dd20a3a7334663f55a1f349f98d4835 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation --- libc/string/arm/memset.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 412270f50..f4b30b3dc 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f + uxtb r1, r1 lsl r3, r1, #8 orr r1, r3 lsl r3, r1, #16 @@ -68,6 +69,7 @@ memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? blo 2f + uxtb a2, a2 orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: -- 2.35.1
"tombannink" == tombannink <tombannink@gmail.com> writes:
It looks like I mixed up some tabs/spaces. Here is a patch where the tabs are consistent with the existing code: From 5e0845fc2dd20a3a7334663f55a1f349f98d4835 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation
Why did you drop the description of the issue from your first mail? -- Bye, Peter Korsgaard
Hi Peter, I'm not sure what you mean, I'm not familiar with this style of sending patches (I'm used to pull-requests on github/gitlab). Should I copy the full contents of my first email (reproducible example, bug explanation, etc) in the commit message and then generate another patch file? Best, Tom
"tombannink" == tombannink <tombannink@gmail.com> writes:
Hi Peter, I'm not sure what you mean, I'm not familiar with this style of sending patches (I'm used to pull-requests on github/gitlab). Should I copy the full contents of my first email (reproducible example, bug explanation, etc) in the commit message and then generate another patch file?
Well, your commit just says "Fix bug in ARM memset implementation", which isn't really helpful. Please describe what the issue is, like you did in your initial mail. -- Bye, Peter Korsgaard
From 0a45e40834eeb8fc42f74645be715ca14432a767 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce: char array[256]; memset(array, -5, 256); This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The aarch64 and x86_64 implementations do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset. With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it. --- libc/string/arm/memset.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 412270f50..f4b30b3dc 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f + uxtb r1, r1 lsl r3, r1, #8 orr r1, r3 lsl r3, r1, #16 @@ -68,6 +69,7 @@ memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? blo 2f + uxtb a2, a2 orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: -- 2.35.1
"tombannink" == tombannink <tombannink@gmail.com> writes:
From 0a45e40834eeb8fc42f74645be715ca14432a767 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation
The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce:
Thanks!
+++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f
+ uxtb r1, r1
uxtb is an ARMv6+ instruction, maybe use and instead for compatibility? https://developer.arm.com/documentation/dui0489/i/arm-and-thumb-instructions... -- Bye, Peter Korsgaard
Good catch, thanks Peter. From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce: char array[256]; memset(array, -5, 256); This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The aarch64 and x86_64 implementations do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset. With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it. --- libc/string/arm/memset.S | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 412270f50..29c583f16 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f + and r1, r1, #0xFF lsl r3, r1, #8 orr r1, r3 lsl r3, r1, #16 @@ -68,6 +69,7 @@ memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? blo 2f + and a2, a2, #0xFF orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: -- 2.35.1
"tombannink" == tombannink <tombannink@gmail.com> writes:
Good catch, thanks Peter. From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation
The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce:
char array[256]; memset(array, -5, 256);
This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The aarch64 and x86_64 implementations do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset.
With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it.
Acked-by: Peter Korsgaard <peter@korsgaard.com> -- Bye, Peter Korsgaard
Hi Tom, thanks applied and pushed. best regards Waldemar tombannink@gmail.com wrote,
Good catch, thanks Peter.
From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001 From: Tom Bannink <tombannink@gmail.com> Date: Tue, 12 Apr 2022 11:15:41 +0200 Subject: [PATCH] Fix bug in ARM memset implementation
The ARM implementation of memset has a bug when the fill-value is negative or outside the [0, 255] range. To reproduce:
char array[256]; memset(array, -5, 256);
This is supposed to fill the array with int8 values -5, -5, -5, ... . On ARM, this does not work because the implementation assumes the high bytes of the fill-value argument are already zero. However in this test case they are filled with 1-bits. The aarch64 and x86_64 implementations do not have this problem: they first convert the fill-value to an unsigned byte following the specification of memset.
With GCC one can use `memset(ptr, (-5 & 0xFF), size)` as a workaround, but for clang users that does not work: clang optimizes the `& 0xFF` away because it assumes that memset will do it. --- libc/string/arm/memset.S | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/libc/string/arm/memset.S b/libc/string/arm/memset.S index 412270f50..29c583f16 100644 --- a/libc/string/arm/memset.S +++ b/libc/string/arm/memset.S @@ -32,6 +32,7 @@ memset: cmp r2, #8 @ at least 8 bytes to do? bcc 2f
+ and r1, r1, #0xFF lsl r3, r1, #8 orr r1, r3 lsl r3, r1, #16 @@ -68,6 +69,7 @@ memset: mov a4, a1 cmp a3, $8 @ at least 8 bytes to do? blo 2f + and a2, a2, #0xFF orr a2, a2, a2, lsl $8 orr a2, a2, a2, lsl $16 1: -- 2.35.1 _______________________________________________ devel mailing list -- devel@uclibc-ng.org To unsubscribe send an email to devel-leave@uclibc-ng.org
participants (3)
-
Peter Korsgaard -
tombannink＠gmail.com -
Waldemar Brodkorb