12 Apr
2022
12 Apr
'22
8:38 a.m.
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/mems…
Before line 35 (lsl) insert this: uxtb r1, r1
Before line 71 (orr) insert this: uxtb a2, a2
Best regards,
Tom
12 Apr
12 Apr
8:59 a.m.
Hi Tom,
tombannink(a)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/mems…
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
9:17 a.m.
From 384c7efdecedaa12d195cce4a45b57d998a5de1d Mon Sep 17 00:00:00 2001
From: Tom Bannink <tombannink(a)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
9:27 a.m.
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(a)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
9:53 a.m.
>>>> "tombannink" == tombannink
<tombannink(a)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(a)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
10 a.m.
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
10:20 a.m.
>>>> "tombannink" == tombannink
<tombannink(a)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
10:27 a.m.
From 0a45e40834eeb8fc42f74645be715ca14432a767 Mon Sep 17 00:00:00 2001
From: Tom Bannink <tombannink(a)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
1:18 p.m.
>>>> "tombannink" == tombannink
<tombannink(a)gmail.com> writes:
From 0a45e40834eeb8fc42f74645be715ca14432a767 Mon Sep
17 00:00:00 2001
From: Tom Bannink <tombannink(a)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-instruction…
--
Bye, Peter Korsgaard
1:36 p.m.
Good catch, thanks Peter.
From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001
From: Tom Bannink <tombannink(a)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
10:07 p.m.
>>>> "tombannink" == tombannink
<tombannink(a)gmail.com> writes:
Good catch, thanks Peter.
From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001
From: Tom Bannink <tombannink(a)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(a)korsgaard.com>
--
Bye, Peter Korsgaard
13 Apr
13 Apr
3:12 p.m.
Hi Tom,
thanks applied and pushed.
best regards
Waldemar
tombannink(a)gmail.com wrote,
Good catch, thanks Peter.
From 880ae1a543f6518f37a32b3af5a734a7b72f9b39 Mon Sep 17 00:00:00 2001
From: Tom Bannink <tombannink(a)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(a)uclibc-ng.org
To unsubscribe send an email to devel-leave(a)uclibc-ng.org
752
days inactive
753
days old
11 comments
3 participants
participants (3)
-
Peter Korsgaard -
tombannink＠gmail.com -
Waldemar Brodkorb