[U-Boot] imx: get_ticks in syscounter.c get miscompiled by GCC 6
Hi,
It seems to me that both GCC 6.3 and 6.4 mis-compiles arch/arm/mach-imx/syscounter.c.
I'm attaching two files, bad.txt is the original syscounter.c and good.txt is the one with the following patch.
diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c index 9290918dca..30ed0109a2 100644 --- a/arch/arm/mach-imx/syscounter.c +++ b/arch/arm/mach-imx/syscounter.c @@ -82,7 +82,7 @@ unsigned long long get_ticks(void) { unsigned long long now;
- asm("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); + asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now));
gd->arch.tbl = (unsigned long)(now & 0xffffffff); gd->arch.tbu = (unsigned long)(now >> 32);
The target code is the while loop in the __udelay. void __udelay(unsigned long usec) { unsigned long long tmp; ulong tmo;
tmo = us_to_tick(usec); tmp = get_ticks() + tmo; /* get current timestamp */
while (get_ticks() < tmp) /* loop till event */ /*NOP*/; }
Here is the mis compiled asm from the above code (whole function is attached as bad.txt)
88: 428b cmp r3, r1 8a: f8ce 20a4 str.w r2, [lr, #164] ; 0xa4 8e: bf08 it eq 90: 4282 cmpeq r2, r0 92: f8ce 30a0 str.w r3, [lr, #160] ; 0xa0 96: d3f7 bcc.n 88 <__udelay+0x88>
Note that the last bcc.n to 88 and we don't see mrrc.
This seems to be that both get_ticks() are inlined and "mrrc"s are duplicated in the __udealy() and GCC sees it as an opportunity to optimize out.
GCC 5 and 8 seems to work fine. Unfortunately I don't have GCC 7 ATM so no idea how it compiles.
Does anyone see this?
Hi,
On Tue, 6 Mar 2018 15:06:08 +0900 Yasushi SHOJI wrote:
Hi,
It seems to me that both GCC 6.3 and 6.4 mis-compiles
s/mis-compiles/optimizes/
Without the 'volatile' attribute the compiler is entitled to move the asm code around or optimize it out. So, your patch is the correct fix independent from the gcc version used.
arch/arm/mach-imx/syscounter.c.
I'm attaching two files, bad.txt is the original syscounter.c and good.txt is the one with the following patch.
diff --git a/arch/arm/mach-imx/syscounter.c b/arch/arm/mach-imx/syscounter.c index 9290918dca..30ed0109a2 100644 --- a/arch/arm/mach-imx/syscounter.c +++ b/arch/arm/mach-imx/syscounter.c @@ -82,7 +82,7 @@ unsigned long long get_ticks(void) { unsigned long long now;
asm("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now));
asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (now)); gd->arch.tbl = (unsigned long)(now & 0xffffffff); gd->arch.tbu = (unsigned long)(now >> 32);
Lothar Waßmann
On Tue, Mar 6, 2018 at 9:31 AM, Lothar Waßmann LW@karo-electronics.de wrote:
Without the 'volatile' attribute the compiler is entitled to move the asm code around or optimize it out. So, your patch is the correct fix independent from the gcc version used.
Yes, but then it would be better to fix all the places where asm is used without volatile.
Hi,
On Tue, Mar 6, 2018 at 10:11 PM, Fabio Estevam festevam@gmail.com wrote:
On Tue, Mar 6, 2018 at 9:31 AM, Lothar Waßmann LW@karo-electronics.de wrote:
Without the 'volatile' attribute the compiler is entitled to move the asm code around or optimize it out. So, your patch is the correct fix independent from the gcc version used.
Yes, but then it would be better to fix all the places where asm is used without volatile.
That'd be a quite big patch. A quick grep shows me that there is over 100 asm() without volatile.
git grep -e '\basm *(' | grep -v -e volatile -e nop | wc -l 153
Do you guys really want to put volatile on all of these now? We are at rc4 and Tom is planing to cut the release March 12th.
I'm attaching a tentative patch to fix only syscounter.c. If it looks good, I'l resend it by git-send-email.
Best,
Hi Yasushi ,
On Wed, Mar 7, 2018 at 2:57 AM, Yasushi SHOJI yasushi.shoji@gmail.com wrote:
Do you guys really want to put volatile on all of these now? We are at rc4 and Tom is planing to cut the release March 12th.
This can be done at a later step.
I'm attaching a tentative patch to fix only syscounter.c. If it looks good, I'l resend it by git-send-email.
Patch looks good. Make sure to add your Signed-off-by line, then you can send it via git send-email.
Thanks
On Wed, Mar 07, 2018 at 10:42:44AM -0300, Fabio Estevam wrote:
Hi Yasushi ,
On Wed, Mar 7, 2018 at 2:57 AM, Yasushi SHOJI yasushi.shoji@gmail.com wrote:
Do you guys really want to put volatile on all of these now? We are at rc4 and Tom is planing to cut the release March 12th.
This can be done at a later step.
Yes. And it should be a little bit manual too. For example, using your regex (thanks!) I see we have some powerpc code that's doing asm("eieio") and that should be eieio() (which is in turn an asm volatile ...), as well as some sync;isync or just sync/isync that should be sync();isync(); or similar. And people that know x86 might have some useful comments there too.
I'm attaching a tentative patch to fix only syscounter.c. If it looks good, I'l resend it by git-send-email.
Patch looks good. Make sure to add your Signed-off-by line, then you can send it via git send-email.
Yes please, thanks!
Hi,
On Wed, Mar 7, 2018 at 11:27 PM, Tom Rini trini@konsulko.com wrote:
On Wed, Mar 07, 2018 at 10:42:44AM -0300, Fabio Estevam wrote:
Patch looks good. Make sure to add your Signed-off-by line, then you can send it via git send-email.
Yes please, thanks!
I've sent it to you with my signed-of-by.
thanks,
participants (4)
-
Fabio Estevam -
Lothar Waßmann -
Tom Rini -
Yasushi SHOJI