ARM : missing corrupted reg in __do_div_asm
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
__xl(R0 in little endian system, or R1 in big endian system) is corrupted after calling __do_div64 and compiler is not informed about this in macro __do_div_asm. If n is used again afterwards, __xl won't be reloaded and n will contain incorrect value.
--- arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arm/include/asm/div64.h +4 -2
@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh) __asmeq("%1", "r2") + __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;
Signature · 1 line
2.4.3
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 14 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +1
-0@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh)__asmeq ("%1" ,"r2" )
arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 10 lines
arch/arm/include/asm/div64.h +3
-2+ __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;
Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Arnd
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 32 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 19 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +3
-1@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh) __asmeq("%1", "r2") + __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4")"bl __do_div64"
Quoted 8 lines
arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 5 lines
arch/arm/include/asm/div64.h +1
-1- : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Since __xl is not used for any real argument to the asm, I think we can just add __xl to the clobber list directly, without needing to introduce an extra register variable ... no?
Cheers
---Dave
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 42 lines · Dave Martin
Quoted 32 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 19 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +3
-1@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh) __asmeq("%1", "r2") + __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4")"bl __do_div64" Quoted 8 lines
arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 5 lines
arch/arm/include/asm/div64.h +1
-1- : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?
No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since __xl may be r0, and __n _is_ r0, you can't specify r0 in the clobber list.
Signature · 3 lines
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 43 lines · Russell King - ARM Linux
Quoted 42 lines · Dave Martin
Quoted 40 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 14 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +1
-0@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh)__asmeq ("%1" ,"r2" )arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 10 lines
arch/arm/include/asm/div64.h +3
-2+ __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 7 lines
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since
__xlmay be r0, and__n_is_ r0, you can't specify r0 in the clobber list.
Hmm, you're right -- in which case the change looks reasonable.
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;
(providing that GCC doesn't make a mess of the "easy" shift).
Cheers
---Dave
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 60 lines · Dave Martin
Quoted 40 lines · Russell King - ARM Linux
Quoted 39 lines · Dave Martin
Quoted 38 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 14 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +1
-0@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh)__asmeq ("%1" ,"r2" )arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 10 lines
arch/arm/include/asm/div64.h +3
-2+ __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 6 lines
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since
__xlmay be r0, and__n_is_ r0, you can't specify r0 in the clobber list.Hmm, you're right -- in which case the change looks reasonable.
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
Quoted 3 lines
Cheers
---Dave
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 61 lines · ??(Gangchen)
Quoted 60 lines · Dave Martin
Quoted 40 lines · Russell King - ARM Linux
Quoted 39 lines · Dave Martin
Quoted 38 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 14 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +1
-0@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh)__asmeq ("%1" ,"r2" )arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 10 lines
arch/arm/include/asm/div64.h +3
-2+ __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 6 lines
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since
__xlmay be r0, and__n_is_ r0, you can't specify r0 in the clobber list.Hmm, you're right -- in which case the change looks reasonable.
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;
Quoted 3 lines
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
Cheers
---Dave
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 62 lines · Dave Martin
Quoted 61 lines · ??(Gangchen)
Quoted 60 lines · Dave Martin
Quoted 40 lines · Russell King - ARM Linux
Quoted 39 lines · Dave Martin
Quoted 38 lines · Arnd Bergmann
Quoted 8 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
Quoted 14 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +1
-0@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh)__asmeq ("%1" ,"r2" )arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 10 lines
arch/arm/include/asm/div64.h +3
-2+ __asmeq("%3", "r0") + __asmeq("%4", "r4") __asmeq("%2", "r0") - __asmeq("%3", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 6 lines
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since
__xlmay be r0, and__n_is_ r0, you can't specify r0 in the clobber list.Hmm, you're right -- in which case the change looks reasonable.
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
With the help of other colleagues, I understand your proposal now. I create a patch and I can verify that it works!
Should I submit it, as it seems better than this one I sent?
Cheers ---Dave
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 3 lines · ??(Gangchen)
Quoted 2 lines · Dave Martin
Quoted 1 line · ??(Gangchen)
On 03/29/2016 06:56 PM, Dave Martin wrote:
[...]
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
With the help of other colleagues, I understand your proposal now. I create a patch and I can verify that it works!
Ah, OK. I was wondering whether I made a mistake somewhere.
Quoted 1 line
Should I submit it, as it seems better than this one I sent?
It's up to you -- I think my approach is a bit cleaner, but your approach worked too and is not vulnerable to compilers that generate silly code for (uint64_t) >> 32.
Note that I only tested my code for little endian -- it should do the right thing for BE, but I recommend that you try it and examine the generated code, to make sure.
Cheers
---Date
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 23 lines · Dave Martin
Quoted 3 lines · ??(Gangchen)
Quoted 2 lines · Dave Martin
Quoted 1 line · ??(Gangchen)
On 03/29/2016 06:56 PM, Dave Martin wrote:
[...]
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
Quoted 11 lines
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;Quoted 3 lines
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
Quoted 8 lines
Quoted 2 lines
With the help of other colleagues, I understand your proposal now. I create a patch and I can verify that it works!
Ah, OK. I was wondering whether I made a mistake somewhere.
Quoted 1 line
Should I submit it, as it seems better than this one I sent?
It's up to you -- I think my approach is a bit cleaner, but your approach worked too and is not vulnerable to compilers that generate silly code for (
uint64_t) >> 32.
(uint64_t) >> 32 is pretty much the standard way to get upper half of an uint64 variable, compiler should not
generate silly code for this.
Quoted 3 lines
Note that I only tested my code for little endian -- it should do the right thing for BE, but I recommend that you try it and examine the generated code, to make sure.
I don't have a BE system to test, but I did check assembly code generated for BE system and didn't find
anything wrong.
I will update the new patch soon.
The following is my test code and assembly dump for BE system of the function.
typedef unsigned long long ull;
ull mydiv64y(ull tt, unsigned base, unsigned *p)
{
ull t = tt;
*p = do_div(t, base);
*p = do_div(tt, base+1);
return tt;
}/mnt/2nd_disk/rdaMicro/aosp_4.4/test_modules/div_test/.tmp_test_div.o:
file format elf32-bigarm
Disassembly of section .text:
00000000 <mydiv64y>:
0: e92d 47f0 stmdb sp!, {r4, r5, r6, r7, r8, r9, sl, lr}
4: 4607 mov r7, r0
6: 2600 movs r6, #0
8: 4698 mov r8, r3
a: ea57 0306 orrs.w r3, r7, r6
e: 4681 mov r9, r0
10: 460d mov r5, r1
12: 4692 mov sl, r2
14: d111 bne.n 3a <mydiv64y+0x3a>
16: f102 0401 add.w r4, r2, #1
1a: 4608 mov r0, r1
1c: 4621 mov r1, r4
1e: f7ff fffe bl 0 <__aeabi_uidivmod>
22: 4628 mov r0, r5
24: 460f mov r7, r1
26: 4621 mov r1, r4
28: f7ff fffe bl 0 <__aeabi_uidiv>
2c: 4632 mov r2, r6
2e: 4601 mov r1, r0
30: f8c8 7000 str.w r7, [r8]
34: 4610 mov r0, r2
36: e8bd 87f0 ldmia.w sp!, {r4, r5, r6, r7, r8, r9, sl, pc}
3a: 4614 mov r4, r2
3c: f7ff fffe bl 0 <__do_div64>
40: f10a 0401 add.w r4, sl, #1
44: f8c8 0000 str.w r0, [r8]
48: 4629 mov r1, r5
4a: 4648 mov r0, r9
4c: f7ff fffe bl 0 <__do_div64>
50: 4619 mov r1, r3
52: 4607 mov r7, r0
54: e7ec b.n 30 <mydiv64y+0x30>
56: bf00 nopQuoted 3 lines
Cheers
---Date
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
Quoted 37 lines · ??(Gangchen)
Quoted 23 lines · Dave Martin
Quoted 3 lines · ??(Gangchen)
Quoted 2 lines · Dave Martin
Quoted 1 line · ??(Gangchen)
On 03/29/2016 06:56 PM, Dave Martin wrote:
[...]
Quoted 17 lines
Quoted 17 lines
Quoted 11 lines
Quoted 11 lines
I wonder whether the following would be cleaner than having these aliased arguments:
asm( /* ... */ "bl __do_div64" : "+r" (__n), "=r" (__res) : "r" (__base) : "ip", "lr", "cc"); *n = __res; return __n >> 32;Quoted 3 lines
Quoted 1 line
(providing that GCC doesn't make a mess of the "easy" shift).
I tried your proposal. It didn't make any difference: this is inline function and gcc just ignores your trick.
What doesn't work for you when using this method?
Why does the fact that this is an inline function make a difference?
Quoted 8 lines
Quoted 2 lines
With the help of other colleagues, I understand your proposal now. I create a patch and I can verify that it works!
Ah, OK. I was wondering whether I made a mistake somewhere.
Quoted 1 line
Should I submit it, as it seems better than this one I sent?
It's up to you -- I think my approach is a bit cleaner, but your approach worked too and is not vulnerable to compilers that generate silly code for (
uint64_t) >> 32.(
uint64_t) >> 32 is pretty much the standard way to get upper half of an uint64 variable, compiler should not
generate silly code for this.
Quoted 6 lines
Quoted 3 lines
Note that I only tested my code for little endian -- it should do the right thing for BE, but I recommend that you try it and examine the generated code, to make sure.
I don't have a BE system to test, but I did check assembly code generated for BE system and didn't find
anything wrong.
That should be enough -- the code generation is simple enough in this case to review directly.
Quoted 26 lines
I will update the new patch soon.
The following is my test code and assembly dump for BE system of the function.
typedef unsigned long long ull;
ullmydiv64y(ull tt, unsigned base, unsigned *p){ ull t = tt; *p = do_div(t, base); *p = do_div(tt, base+1); return tt; }
/mnt/2nd_disk/rdaMicro/aosp_4.4/test_modules/div_test/.tmp_test_div.o:
file format elf32-bigarmDisassembly of section .text:
00000000 <mydiv64y>:
0: e92d 47f0 stmdb sp!, {r4, r5, r6, r7, r8, r9, sl, lr} 4: 4607 mov r7, r0 6: 2600 movs r6, #0 8: 4698 mov r8, r3 a: ea57 0306 orrs.w r3, r7, r6
[...]
Quoted 1 line
14: d111 bne.n 3a <mydiv64y+0x3a>
[...]
Quoted 3 lines
30: f8c8 7000 str.w r7, [r8]
34: 4610 mov r0, r2
36: e8bd 87f0 ldmia.w sp!, {r4, r5, r6, r7, r8, r9, sl, pc}[...]
Quoted 2 lines
3a: 4614 mov r4, r2 3c: f7ff fffe bl 0 <__do_div64>
[...]
Quoted 1 line
44: f8c8 0000 str.w r0, [r8]
[...]
Quoted 4 lines
4c: f7ff fffe bl 0 <__do_div64> 50: 4619 mov r1, r3 52: 4607 mov r7, r0 54: e7ec b.n 30 <mydiv64y+0x30>
This looks sensible for the BE case. I see the high part of the remainder (r0) being stored back to *p in each case...
For the LE case, we should see r1 stored instead, IIUC.
Cheers
---Dave
PATCH v3 1/1 ARM : missing corrupted reg in __div64_32
__xl(R0 in little endian system, or R1 in big endian system) is corrupted after calling __do_div64 and compiler is not informed about this. If n is used again afterwards, __xl won't be reloaded and n will contain incorrect value.
--- arch/arm/include/asm/div64.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
arch/arm/include/asm/div64.h +8 -14
@@ -21,30 +21,24 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* +* rem is saved at the up half of __n, see __do_div64 for more +*/ +return __n >>32 ; }#define __div64_32 __div64_32
Signature · 1 line
2.4.3
PATCH v3 1/1 ARM : missing corrupted reg in __div64_32
Quoted 4 lines · chengang
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.
You should briefly explain the fix here.
Quoted 12 lines
Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>--- arch/arm/include/asm/div64.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)arch/arm/include/asm/div64.h +0
-0
arch/arm/include/asm/div64.h line 30
Quoted 10 lines
arch/arm/include/asm/div64.h +0
-7@@ -21,30 +21,24 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif
These defines aren't used by anything else, right?
I grepped for them, but I didn't see any other matches.
Quoted 14 lines
arch/arm/include/asm/div64.h +2
-4static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4")"bl __do_div64"
arch/arm/include/asm/div64.h line 40
Quoted 10 lines
arch/arm/include/asm/div64.h +5
-3- : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* +* rem is saved at the up half of __n, see __do_div64 for more +*/
Minor nit:
It's useful to have a comment here, but rem is gone, and you might want to say "upper half", e.g., something like:
/* __do_div returns the remainder in the upper word of __n: */
Quoted 6 lines
arch/arm/include/asm/div64.h +1
-0+return __n >>32 ; }#define __div64_32 __div64_32Signature · 1 line
2.4.3
Cheers
---Dave
PATCH v4 1/1 ARM : missing corrupted reg in __div64_32
__xl(R0 in little endian system, or R1 in big endian system) is corrupted by the call to __do_div64i(). The compiler is not aware of this, so if n is used afterwards __xl won't be reloaded and n will contain incorrect value.
Mark 64bits variable __n as inputi/output to inform compiler both r0 and r1 are changed by the funciton call.
--- arch/arm/include/asm/div64.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)
arch/arm/include/asm/div64.h +8 -14
@@ -21,30 +21,24 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* +* __do_div64 returns the remainder in the upper word of __n +*/ +return __n >>32 ; }#define __div64_32 __div64_32
Signature · 1 line
2.4.3
PATCH v4 1/1 ARM : missing corrupted reg in __div64_32
Quoted 2 lines · chengang
__xl(R0 in little endian system, or R1 in big endian system)is corrupted by the call to__do_div64i(). The compiler is not aware of this, so if
Should that be "__do_div64", not "__do_div64i"?
Quoted 3 lines
n is used afterwards
__xlwon't be reloaded and n will contain incorrect value.Mark 64bits variable
__nas inputi/output to inform compiler both r0 and r1
Typo: "inputi"
Quoted 37 lines
are changed by the funciton call.
Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>--- arch/arm/include/asm/div64.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)arch/arm/include/asm/div64.h +2
-11@@ -21,30 +21,24 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4")"bl __do_div64"
arch/arm/include/asm/div64.h line 40
Quoted 10 lines
arch/arm/include/asm/div64.h +5
-3- : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* +* __do_div64 returns the remainder in the upper word of __n +*/
Minor nit: ^ this is short enough to be a one-line comment
arch/arm/include/asm/div64.h line 43
Quoted 3 lines
arch/arm/include/asm/div64.h +1
-0+return __n >>32 ; }#define __div64_32 __div64_32
With those changes,
Cheers
---Dave
PATCH v5 1/1 ARM : missing corrupted reg in __div64_32
__xl(R0 in little endian system, or R1 in big endian system) is corrupted by the call to __do_div64(). The compiler is not aware of this, so if n is used afterwards __xl won't be reloaded and n will contain incorrect value.
Mark 64bits variable __n as input/output to inform compiler both r0 and r1 are changed by the funciton call.
--- arch/arm/include/asm/div64.h | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
arch/arm/include/asm/div64.h +6 -14
@@ -21,30 +21,22 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* __do_div64 returns the remainder in the upper word of __n */ +return __n >>32 ; }#define __div64_32 __div64_32
Signature · 1 line
2.4.3
PATCH v5 1/1 ARM : missing corrupted reg in __div64_32
Quoted 6 lines · chengang
__xl(R0 in little endian system, or R1 in big endian system)is corrupted by the call to__do_div64(). The compiler is not aware of this, so if n is used afterwards__xlwon't be reloaded and n will contain incorrect value.Mark 64bits variable
__nas input/output to inform compiler both r0 and r1 are changed by the funciton call.
Looks good for me now.
Quoted 47 lines
Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>--- arch/arm/include/asm/div64.h | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)arch/arm/include/asm/div64.h +6
-14@@ -21,30 +21,22 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* __do_div64 returns the remainder in the upper word of __n */ +return __n >>32 ; }#define __div64_32 __div64_32
PATCH v4 1/1 ARM : missing corrupted reg in __div64_32
In reply to Dave MartinQuoted 59 lines · Dave Martin
Quoted 2 lines · chengang
__xl(R0 in little endian system, or R1 in big endian system)is corrupted by the call to__do_div64i(). The compiler is not aware of this, so ifShould that be "
__do_div64", not "__do_div64i"?Quoted 3 lines
n is used afterwards
__xlwon't be reloaded and n will contain incorrect value.Mark 64bits variable
__nas inputi/output to inform compiler both r0 and r1Typo: "inputi"
Quoted 37 lines
are changed by the funciton call.
Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>--- arch/arm/include/asm/div64.h | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)arch/arm/include/asm/div64.h +2
-11@@ -21,30 +21,24 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4")"bl __do_div64" arch/arm/include/asm/div64.h line 40
Quoted 10 lines
arch/arm/include/asm/div64.h +5
-3- : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +/* +* __do_div64 returns the remainder in the upper word of __n +*/ Minor nit: ^ this is short enough to be a one-line comment
Quoted 6 lines
arch/arm/include/asm/div64.h line 43
Quoted 3 lines
arch/arm/include/asm/div64.h +1
-0+return __n >>32 ; }#define __div64_32 __div64_32With those changes,
Reviewed-by Dave Martin <[email protected]>
Thanks a lot , especially for your patience! I just sent V5, cheers!
Quoted 3 lines
Cheers
---Dave
PATCH v2 1/1 ARM : missing corrupted reg in __div64_32
In reply to Dave Martin__xl(R0 in little endian system, or R1 in big endian system) is corrupted after calling __do_div64 and compiler is not informed about this. If n is used again afterwards, __xl won't be reloaded and n will contain incorrect value.
--- arch/arm/include/asm/div64.h | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-)
arch/arm/include/asm/div64.h +6 -14
@@ -21,30 +21,22 @@* for arguments and results (beware). */ -#ifdef __ARMEB__ -#define __xh "r0" -#define __xl "r1" -#else -#define __xl "r0" -#define __xh "r1" -#endif static inline uint32_t __div64_32 (uint64_t *n,uint32_t base) { register unsigned int __base asm("r4") = base; register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); - register unsigned int __rem asm(__xh); - asm( __asmeq("%0", __xh) + asm( + __asmeq("%0", "r0") __asmeq("%1", "r2") - __asmeq("%2", "r0") - __asmeq("%3", "r4") + __asmeq("%2", "r4") "bl __do_div64" - : "=r" (__rem), "=r" (__res) - : "r" (__n), "r" (__base) + : "+r" (__n), "=r" (__res) + : "r" (__base) : "ip", "lr", "cc"); *n = __res; -return __rem; +return __n >>32 ; }#define __div64_32 __div64_32
Signature · 1 line
2.4.3
PATCH 1/1 ARM : missing corrupted reg in __do_div_asm
In reply to Russell King - ARM LinuxQuoted 8 lines · Russell King - ARM Linux
Quoted 7 lines · Dave Martin
Quoted 6 lines · Arnd Bergmann
Quoted 5 lines · On Monday 28 March 2016 12:19:03 Chen Gang wrote:
__xl(R0 in little endian system, or R1 in big endian system)is corrupted after calling__do_div64and compiler is not informed about this in macro__do_div_asm. If n is used again afterwards,__xlwon't be reloaded and n will contain incorrect value.
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Signed-off-by Chen Gang <[email protected]>Signed-off-by Chen Gang <[email protected]>---
How did you find this? Did you run into this problem on a live system or see it through inspection?
We (in RDAMicro) found the problem when we added TLC nand support in MTD driver.
I submit this patch several time last year.
And this patch also fix one problems of XFS:
http://permalink.gmane.org/gmane.comp.file-systems.xfs.general/69224
Quoted 33 lines
Quoted 26 lines
Quoted 18 lines
Quoted 18 lines
arch/arm/include/asm/div64.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/div64.h +3
-1@@ -35,12 +35,14 @@ static inline uint32_t __div64_32(uint64_t *n, uint32_t base) register unsigned long long __n asm("r0") = *n; registerunsigned long long __res asm("r2" ); register unsigned int __rem asm(__xh); + register unsigned int __clobber asm(__xl); asm( __asmeq("%0", __xh) __asmeq("%1", "r2") + __asmeq("%3", "r0") + __asmeq("%4", "r4")__asmeq ("%2" ,"r0" ) -__asmeq ("%3" ,"r4" )Quoted 8 lines
arch/arm/include/asm/div64.h static inline uint32_t __div64_32(uint64_t *n, uint32_t base) · line 48
Quoted 6 lines
arch/arm/include/asm/div64.h +1
-1"bl __do_div64" - : "=r" (__rem), "=r" (__res) + : "=r" (__rem), "=r" (__res), "=r" (__clobber) : "r" (__n), "r" (__base) : "ip", "lr", "cc"); *n = __res;Doesn't the clobber normally go in the third line along with "ip" and "lr"?
Quoted 3 lines
Since
__xlis not used for any real argument to the asm, I think we can just add__xlto the clobber list directly, without needing to introduce an extra register variable ... no?No, you can't. The clobber list is not allowed to specify registers that may be used for input or output operands, and since
__xlmay be r0, and__n_is_ r0, you can't specify r0 in the clobber list.