Message ID | 546DE833.3090707@myspectrum.nl |
---|---|
State | Not Applicable |
Headers | show |
> On 20-11-14 13:15, Stefan Agner wrote: >> No particular reason, I did not know how to fix this without digging >> into it. Hence, after I discovered this, I checked why those warnings >> do not happen for the kernel, then I applied just the AFLAGS the >> kernel is using. I guess fixing the underlying issue is the better >> option, and doing this also for the kernel would be the best >> way... Maybe the kernel community also knows better why they choose >> to use the AFLAGS instead (and if there are gas version which do have >> problems with a proper fix)... On 20 Nov 2014, jeroen@myspectrum.nl wrote: > for what it is worth, I have attached patch hanging around, but I > never actually tested it. It is for the current version. >> From c151254b3de49d8fccb69ab4f9442d884b9ff85c Mon Sep 17 00:00:00 >> 2001 > From: Jeroen Hofstee <jeroen@myspectrum.nl> > Date: Thu, 20 Nov 2014 14:06:26 +0100 > Subject: [PATCH] arm: memset: make it UAL compliant > --- > arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S > index 0cdf895..4fe38f6 100644 > --- a/arch/arm/lib/memset.S > +++ b/arch/arm/lib/memset.S > @@ -18,8 +18,8 @@ > 1: subs r2, r2, #4 @ 1 do we have enough > blt 5f @ 1 bytes to align with? > cmp r3, #2 @ 1 > - strltb r1, [r0], #1 @ 1 > - strleb r1, [r0], #1 @ 1 > + strblt r1, [r0], #1 @ 1 > + strble r1, [r0], #1 @ 1 To test this, can we just use 'objdump'. The hex codes should be identical; there is only one encoding. It should produce the same binaries. No need to run test-suites, etc. Fwiw, Bill.
Hi, On 20-11-14 16:18, Bill Pringlemeir wrote: > >> --- >> arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- >> 1 file changed, 20 insertions(+), 20 deletions(-) >> >> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S >> index 0cdf895..4fe38f6 100644 >> --- a/arch/arm/lib/memset.S >> +++ b/arch/arm/lib/memset.S >> @@ -18,8 +18,8 @@ >> 1: subs r2, r2, #4 @ 1 do we have enough >> blt 5f @ 1 bytes to align with? >> cmp r3, #2 @ 1 >> - strltb r1, [r0], #1 @ 1 >> - strleb r1, [r0], #1 @ 1 >> + strblt r1, [r0], #1 @ 1 >> + strble r1, [r0], #1 @ 1 > To test this, can we just use 'objdump'. The hex codes should be > identical; there is only one encoding. It should produce the same > binaries. No need to run test-suites, etc. > yes, I should be trivial to test (and find the trivial problem, with the patch I attached). I am wondering though if all version of gas accept the suffix notation... any idea? Regards, Jeroen
>>> --- >>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- >>> 1 file changed, 20 insertions(+), 20 deletions(-) >>> >>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S >>> index 0cdf895..4fe38f6 100644 >>> --- a/arch/arm/lib/memset.S >>> +++ b/arch/arm/lib/memset.S >>>>> -18,8 +18,8 @@ >>> 1: subs r2, r2, #4 @ 1 do we have enough >>> blt 5f @ 1 bytes to align with? >>> cmp r3, #2 @ 1 >>> - strltb r1, [r0], #1 @ 1 >>> - strleb r1, [r0], #1 @ 1 >>> + strblt r1, [r0], #1 @ 1 >>> + strble r1, [r0], #1 @ 1 >> To test this, can we just use 'objdump'. The hex codes should be >> identical; there is only one encoding. It should produce the same >> binaries. No need to run test-suites, etc. On 20 Nov 2014, jeroen@myspectrum.nl wrote: > yes, I should be trivial to test (and find the trivial problem, with > the patch I attached). I am wondering though if all version of > gas accept the suffix notation... any idea? One part of the answer is here, https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD The 'strCCb' version is definitely more popular in older ARM books. Certainly there could be bugs and/or patched versions that make a difference. Probably it would be helpful to know what versions are supported. Back in 1999 it seems that the code at least tries to take conditions anywhere, https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099 I think it is most likely to result in a parse error if it wasn't supported. Any version since Thumb2/Unified (2003-2005?) was introduced should be accepting this syntax with less issues. Ie, it seems like a better way forward. Historical versions are here, http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/ Who knows if some vendor patched things to mess something up? Probably grabbing an older 'gas' version and verifying it was the same binary before/after the patch would probably be fair confirmation? I don't think you can 100% guarantee this doesn't break with some archaic vendors gas. Fwiw, Bill Pringlemeir.
Hi, On 20-11-14 19:21, Bill Pringlemeir wrote: >>>> --- >>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- >>>> 1 file changed, 20 insertions(+), 20 deletions(-) >>>> >>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S >>>> index 0cdf895..4fe38f6 100644 >>>> --- a/arch/arm/lib/memset.S >>>> +++ b/arch/arm/lib/memset.S >>>>>> -18,8 +18,8 @@ >>>> 1: subs r2, r2, #4 @ 1 do we have enough >>>> blt 5f @ 1 bytes to align with? >>>> cmp r3, #2 @ 1 >>>> - strltb r1, [r0], #1 @ 1 >>>> - strleb r1, [r0], #1 @ 1 >>>> + strblt r1, [r0], #1 @ 1 >>>> + strble r1, [r0], #1 @ 1 >>> To test this, can we just use 'objdump'. The hex codes should be >>> identical; there is only one encoding. It should produce the same >>> binaries. No need to run test-suites, etc. > On 20 Nov 2014, jeroen@myspectrum.nl wrote: > >> yes, I should be trivial to test (and find the trivial problem, with >> the patch I attached). I am wondering though if all version of >> gas accept the suffix notation... any idea? > One part of the answer is here, > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD > > The 'strCCb' version is definitely more popular in older ARM books. > Certainly there could be bugs and/or patched versions that make a > difference. Probably it would be helpful to know what versions are > supported. > > Back in 1999 it seems that the code at least tries to take conditions > anywhere, > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099 > > I think it is most likely to result in a parse error if it wasn't > supported. Any version since Thumb2/Unified (2003-2005?) was introduced > should be accepting this syntax with less issues. Ie, it seems like a > better way forward. > > Historical versions are here, > > http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/ > > Who knows if some vendor patched things to mess something up? Probably > grabbing an older 'gas' version and verifying it was the same binary > before/after the patch would probably be fair confirmation? I don't > think you can 100% guarantee this doesn't break with some archaic > vendors gas. Ok thanks for digging that up, that doesn't sound like a problem then. Stefan, can you check if you can actually fix the warnings instead of suppressing them? Regards, Jeroen
On 2014-11-20 20:14, Jeroen Hofstee wrote: > Hi, > > On 20-11-14 19:21, Bill Pringlemeir wrote: >>>>> --- >>>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- >>>>> 1 file changed, 20 insertions(+), 20 deletions(-) >>>>> >>>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S >>>>> index 0cdf895..4fe38f6 100644 >>>>> --- a/arch/arm/lib/memset.S >>>>> +++ b/arch/arm/lib/memset.S >>>>>>> -18,8 +18,8 @@ >>>>> 1: subs r2, r2, #4 @ 1 do we have enough >>>>> blt 5f @ 1 bytes to align with? >>>>> cmp r3, #2 @ 1 >>>>> - strltb r1, [r0], #1 @ 1 >>>>> - strleb r1, [r0], #1 @ 1 >>>>> + strblt r1, [r0], #1 @ 1 >>>>> + strble r1, [r0], #1 @ 1 >>>> To test this, can we just use 'objdump'. The hex codes should be >>>> identical; there is only one encoding. It should produce the same >>>> binaries. No need to run test-suites, etc. >> On 20 Nov 2014, jeroen@myspectrum.nl wrote: >> >>> yes, I should be trivial to test (and find the trivial problem, with >>> the patch I attached). I am wondering though if all version of >>> gas accept the suffix notation... any idea? >> One part of the answer is here, >> >> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD >> >> The 'strCCb' version is definitely more popular in older ARM books. >> Certainly there could be bugs and/or patched versions that make a >> difference. Probably it would be helpful to know what versions are >> supported. >> >> Back in 1999 it seems that the code at least tries to take conditions >> anywhere, >> >> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099 >> >> I think it is most likely to result in a parse error if it wasn't >> supported. Any version since Thumb2/Unified (2003-2005?) was introduced >> should be accepting this syntax with less issues. Ie, it seems like a >> better way forward. >> >> Historical versions are here, >> >> http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/ >> >> Who knows if some vendor patched things to mess something up? Probably >> grabbing an older 'gas' version and verifying it was the same binary >> before/after the patch would probably be fair confirmation? I don't >> think you can 100% guarantee this doesn't break with some archaic >> vendors gas. > > Ok thanks for digging that up, that doesn't sound like a problem > then. Stefan, can you check if you can actually fix the warnings > instead of suppressing them? > Ok, I could apply the changes from your patch and it fixed the warnings in memset.S. However, when I build the file in ARM mode then (without CONFIG_SYS_THUMB_BUILD set). I get this: arch/arm/lib/memset.S: Assembler messages: arch/arm/lib/memset.S:92: Error: bad instruction `stmiage ip!,{r1,r3-r8,lr}' arch/arm/lib/memset.S:93: Error: bad instruction `stmiage ip!,{r1,r3-r8,lr}' arch/arm/lib/memset.S:95: Error: bad instruction `ldmfdeq sp!,{r4-r8,pc}' arch/arm/lib/memset.S:98: Error: bad instruction `stmiane ip!,{r1,r3-r8,lr}' arch/arm/lib/memset.S:100: Error: bad instruction `stmiane ip!,{r4-r7}' arch/arm/lib/memset.S:106: Error: bad instruction `stmiane ip!,{r1,r3}' arch/arm/lib/memset.S:114: Error: bad instruction `strbne r1,[ip],#1' arch/arm/lib/memset.S:115: Error: bad instruction `strbne r1,[ip],#1' arch/arm/lib/memset.S:117: Error: bad instruction `strbne r1,[ip],#1' arch/arm/lib/memset.S:123: Error: bad instruction `strblt r1,[ip],#1' arch/arm/lib/memset.S:124: Error: bad instruction `strble r1,[ip],#1' -- Stefan > Regards, > Jeroen
Hello Jeroen, Adding Stefan as it seems he was dropped from the recipients list. Plus : On Thu, 20 Nov 2014 20:14:01 +0100, Jeroen Hofstee <jeroen@myspectrum.nl> wrote: > Hi, > > On 20-11-14 19:21, Bill Pringlemeir wrote: > >>>> --- > >>>> arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- > >>>> 1 file changed, 20 insertions(+), 20 deletions(-) > >>>> > >>>> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S > >>>> index 0cdf895..4fe38f6 100644 > >>>> --- a/arch/arm/lib/memset.S > >>>> +++ b/arch/arm/lib/memset.S > >>>>>> -18,8 +18,8 @@ > >>>> 1: subs r2, r2, #4 @ 1 do we have enough > >>>> blt 5f @ 1 bytes to align with? > >>>> cmp r3, #2 @ 1 > >>>> - strltb r1, [r0], #1 @ 1 > >>>> - strleb r1, [r0], #1 @ 1 > >>>> + strblt r1, [r0], #1 @ 1 > >>>> + strble r1, [r0], #1 @ 1 > >>> To test this, can we just use 'objdump'. The hex codes should be > >>> identical; there is only one encoding. It should produce the same > >>> binaries. No need to run test-suites, etc. > > On 20 Nov 2014, jeroen@myspectrum.nl wrote: > > > >> yes, I should be trivial to test (and find the trivial problem, with > >> the patch I attached). I am wondering though if all version of > >> gas accept the suffix notation... any idea? > > One part of the answer is here, > > > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=history;f=gas/config/tc-arm.c;hb=HEAD > > > > The 'strCCb' version is definitely more popular in older ARM books. > > Certainly there could be bugs and/or patched versions that make a > > difference. Probably it would be helpful to know what versions are > > supported. > > > > Back in 1999 it seems that the code at least tries to take conditions > > anywhere, > > > > https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gas/config/tc-arm.c;hb=858f4ff6ff40a73f2a569fc8886157568f08c6db#l6099 > > > > I think it is most likely to result in a parse error if it wasn't > > supported. Any version since Thumb2/Unified (2003-2005?) was introduced > > should be accepting this syntax with less issues. Ie, it seems like a > > better way forward. > > > > Historical versions are here, > > > > http://ftp.mirrorservice.org/sites/sourceware.org/pub/binutils/old-releases/ > > > > Who knows if some vendor patched things to mess something up? Probably > > grabbing an older 'gas' version and verifying it was the same binary > > before/after the patch would probably be fair confirmation? I don't > > think you can 100% guarantee this doesn't break with some archaic > > vendors gas. > > Ok thanks for digging that up, that doesn't sound like a problem > then. Stefan, can you check if you can actually fix the warnings > instead of suppressing them? (in the right thread this time) Stefan, can you also drop the -mauto-it part? > Regards, > Jeroen Amicalement,
> On 2014-11-20 20:14, Jeroen Hofstee wrote: >> Ok thanks for digging that up, that doesn't sound like a problem >> then. Stefan, can you check if you can actually fix the warnings >> instead of suppressing them? On 21 Nov 2014, stefan@agner.ch wrote: > Ok, I could apply the changes from your patch and it fixed the > warnings in memset.S. However, when I build the file in ARM mode then > (without CONFIG_SYS_THUMB_BUILD set). I get this: > arch/arm/lib/memset.S: Assembler messages: arch/arm/lib/memset.S:92: > Error: bad instruction `stmiage ip!,{r1,r3-r8,lr}' I think you need '.syntax unified' if you want those in ARM mode. I guess you found that out too? I see, + .syntax unified +#ifdef CONFIG_SYS_THUMB_BUILD + .thumb + .thumb_func +#endif in '[PATCH v2] arm: build arch memset/memcpy in Thumb2 mode'; so this solved it? I think it is a very nice feature to have Thumb2 on Vybrid. Many boot devices may have limited bandwidth compared to the running system. Thanks for your work. Regards, Bill Pringlemeir
From c151254b3de49d8fccb69ab4f9442d884b9ff85c Mon Sep 17 00:00:00 2001 From: Jeroen Hofstee <jeroen@myspectrum.nl> Date: Thu, 20 Nov 2014 14:06:26 +0100 Subject: [PATCH] arm: memset: make it UAL compliant --- arch/arm/lib/memset.S | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S index 0cdf895..4fe38f6 100644 --- a/arch/arm/lib/memset.S +++ b/arch/arm/lib/memset.S @@ -18,8 +18,8 @@ 1: subs r2, r2, #4 @ 1 do we have enough blt 5f @ 1 bytes to align with? cmp r3, #2 @ 1 - strltb r1, [r0], #1 @ 1 - strleb r1, [r0], #1 @ 1 + strblt r1, [r0], #1 @ 1 + strble r1, [r0], #1 @ 1 strb r1, [r0], #1 @ 1 add r2, r2, r3 @ 1 (r2 = r2 - (4 - r3)) /* @@ -51,20 +51,20 @@ memset: mov lr, r1 2: subs r2, r2, #64 - stmgeia r0!, {r1, r3, ip, lr} @ 64 bytes at a time. - stmgeia r0!, {r1, r3, ip, lr} - stmgeia r0!, {r1, r3, ip, lr} - stmgeia r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} @ 64 bytes at a time. + stmiage r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} bgt 2b ldmeqfd sp!, {pc} @ Now <64 bytes to go. /* * No need to correct the count; we're only testing bits from now on */ tst r2, #32 - stmneia r0!, {r1, r3, ip, lr} - stmneia r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} tst r2, #16 - stmneia r0!, {r1, r3, ip, lr} + stmiage r0!, {r1, r3, ip, lr} ldr lr, [sp], #4 #else @@ -90,28 +90,28 @@ memset: rsb ip, ip, #32 sub r2, r2, ip movs ip, ip, lsl #(32 - 4) - stmcsia r0!, {r4, r5, r6, r7} - stmmiia r0!, {r4, r5} + stmiacs r0!, {r4, r5, r6, r7} + stmiami r0!, {r4, r5} tst ip, #(1 << 30) mov ip, r1 strne r1, [r0], #4 3: subs r2, r2, #64 - stmgeia r0!, {r1, r3-r7, ip, lr} - stmgeia r0!, {r1, r3-r7, ip, lr} + stmiage r0!, {r1, r3-r7, ip, lr} + stmiage r0!, {r1, r3-r7, ip, lr} bgt 3b - ldmeqfd sp!, {r4-r7, pc} + ldmfdeq sp!, {r4-r7, pc} tst r2, #32 - stmneia r0!, {r1, r3-r7, ip, lr} + stmiage r0!, {r1, r3-r7, ip, lr} tst r2, #16 - stmneia r0!, {r4-r7} + stmiage r0!, {r4-r7} ldmfd sp!, {r4-r7, lr} #endif 4: tst r2, #8 - stmneia r0!, {r1, r3} + stmiage r0!, {r1, r3} tst r2, #4 strne r1, [r0], #4 /* @@ -119,8 +119,8 @@ memset: * may have an unaligned pointer as well. */ 5: tst r2, #2 - strneb r1, [r0], #1 - strneb r1, [r0], #1 + strbne r1, [r0], #1 + strbne r1, [r0], #1 tst r2, #1 - strneb r1, [r0], #1 + strbne r1, [r0], #1 mov pc, lr -- 2.1.0