Skip to contentSkip to conversation

mips: lib: uncached: fix non-standard usage of variable 'sp'

PATCH Applied Anders Roxell 6 participants 6 messages Dec 11, 2020 – Dec 20, 2020 MIPS

lore
Share & tools
lore

Anders Roxell

author
#0 lore raw

PATCH v2 1/1 mips: lib: uncached: fix non-standard usage of variable 'sp'

When building mips tinyconfig with clang the following warning show up:

arch/mips/lib/uncached.c:45:6: warning: variable 'sp' is uninitialized when used here [-Wuninitialized]

        if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
            ^~
arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning
        register long sp __asm__("$sp");
                        ^
                         = 0

1 warning generated.

Rework to make an explicit inline move, instead of the non-standard use of specifying registers for local variables. This is what's written from the gcc-10 manual [1] about specifying registers for local variables:

"6.47.5.2 Specifying Registers for Local Variables
.................................................
[...]

"The only supported use for this feature is to specify registers for input and output operands when calling Extended 'asm' (*note Extended Asm::). [...]".

[1] https://docs.w3cub.com/gcc~10/local-register-variables

Signed-off-by Anders Roxell <[email protected]>
---
 arch/mips/lib/uncached.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/mips/lib/uncached.c +3 -1
index 09d5deea747f..f80a67c092b6 100644
@@ -37,10 +37,12 @@
  */
 unsigned long run_uncached(void *func)
 {
-	register long sp __asm__("$sp");
 	register long ret __asm__("$2");
 	long lfunc = (long)func, ufunc;
 	long usp;
+	long sp;
+
+	__asm__("move %0, $sp" : "=r" (sp));
 
 	if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
 		usp = CKSEG1ADDR(sp);
Signature · 1 line
2.29.2
Reviewed-by on v2 1/1
#1 lore raw
Quoted 21 lines · Anders Roxell

When building mips tinyconfig with clang the following warning show up:

arch/mips/lib/uncached.c:45:6: warning: variable 'sp' is uninitialized when used here [-Wuninitialized]

        if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
            ^~
arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning
        register long sp __asm__("$sp");
                        ^
                         = 0

1 warning generated.

Rework to make an explicit inline move, instead of the non-standard use of specifying registers for local variables. This is what's written from the gcc-10 manual [1] about specifying registers for local variables:

"6.47.5.2 Specifying Registers for Local Variables
.................................................
[...]

Quoted 6 lines

"The only supported use for this feature is to specify registers for input and output operands when calling Extended 'asm' (*note Extended Asm::). [...]".

[1] https://docs.w3cub.com/gcc~10/local-register-variables

Signed-off-by Anders Roxell <[email protected]>
Link https://github.com/ClangBuiltLinux/linux/issues/606
Reported-by Nathan Chancellor <[email protected]>
Reported-by Naresh Kamboju <[email protected]>
Reviewed-by Nick Desaulniers <[email protected]>
Quoted 25 lines
---
 arch/mips/lib/uncached.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
arch/mips/lib/uncached.c +3 -2
index 09d5deea747f..f80a67c092b6 100644
@@ -37,10 +37,12 @@
  */
 unsigned long run_uncached(void *func)
 {
-       register long sp __asm__("$sp");
        register long ret __asm__("$2");
        long lfunc = (long)func, ufunc;
        long usp;
+       long sp;
+
+       __asm__("move %0, $sp" : "=r" (sp));
 
        if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
                usp = CKSEG1ADDR(sp);
--

2.29.2

Signature · 2 lines
Thanks,
~Nick Desaulniers
#2 lore raw
In reply to Anders Roxell
Quoted 5 lines · Anders Roxell
arch/mips/lib/uncached.c +0 -0
index 09d5deea747f..f80a67c092b6 100644

arch/mips/lib/uncached.c line 45

Quoted 10 lines
arch/mips/lib/uncached.c +3 -1
@@ -37,10 +37,12 @@
  */
 unsigned long run_uncached(void *func)
 {
-	register long sp __asm__("$sp");
 	register long ret __asm__("$2");
 	long lfunc = (long)func, ufunc;
 	long usp;
+	long sp;
+
+	__asm__("move %0, $sp" : "=r" (sp));

I thought it might be better to make `sp' global instead, so that it's
the compiler that chooses how to schedule accesses. Have you tried that?

Maciej

#3 lore raw
Quoted 21 lines · Maciej W. Rozycki
Quoted 5 lines · Anders Roxell
arch/mips/lib/uncached.c +0 -0
index 09d5deea747f..f80a67c092b6 100644

arch/mips/lib/uncached.c line 45

Quoted 10 lines
arch/mips/lib/uncached.c +3 -1
@@ -37,10 +37,12 @@
  */
 unsigned long run_uncached(void *func)
 {
-	register long sp __asm__("$sp");
 	register long ret __asm__("$2");
 	long lfunc = (long)func, ufunc;
 	long usp;
+	long sp;
+
+	__asm__("move %0, $sp" : "=r" (sp));

I thought it might be better to make `sp' global instead, so that it's
the compiler that chooses how to schedule accesses. Have you tried that?

Quoted 1 line

Maciej

This will not work, as the LLVM Mips backend does not support using $sp as a global register variable:

https://github.com/llvm/llvm-project/commit/1440bb2a26ff13df1b29762658ee122cc0bc47ae

$ make -skj"$(nproc)" ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- LLVM=1 O=out \
distclean malta_kvm_guest_defconfig vmlinux
fatal error: error in backend: Invalid register name global variable
...

Cheers,
Nathan

maintainer Applied
#4 lore raw
In reply to Anders Roxell
Quoted 24 lines · Anders Roxell

When building mips tinyconfig with clang the following warning show up:

arch/mips/lib/uncached.c:45:6: warning: variable 'sp' is uninitialized when used here [-Wuninitialized]

        if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
            ^~
arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning
        register long sp __asm__("$sp");
                        ^
                         = 0

1 warning generated.

Rework to make an explicit inline move, instead of the non-standard use of specifying registers for local variables. This is what's written from the gcc-10 manual [1] about specifying registers for local variables:

"6.47.5.2 Specifying Registers for Local Variables
.................................................
[...]

"The only supported use for this feature is to specify registers for input and output operands when calling Extended 'asm' (*note Extended Asm::). [...]".

Quoted 5 lines

[1] https://docs.w3cub.com/gcc~10/local-register-variables

Signed-off-by Anders Roxell <[email protected]>
---
 arch/mips/lib/uncached.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

applied to mips-next.

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]
#5 lore raw
Quoted 4 lines · Thomas Bogendoerfer
Quoted 2 lines · Anders Roxell

When building mips tinyconfig with clang the following warning show up:

Quoted 8 lines
Quoted 8 lines

arch/mips/lib/uncached.c:45:6: warning: variable 'sp' is uninitialized when used here [-Wuninitialized]

        if (sp >= (long)CKSEG0 && sp < (long)CKSEG2)
            ^~
arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning
        register long sp __asm__("$sp");
                        ^
                         = 0

1 warning generated.

<trimp>

Quoted 7 lines
Quoted 5 lines

[1] https://docs.w3cub.com/gcc~10/local-register-variables

Signed-off-by Anders Roxell <[email protected]>
---
 arch/mips/lib/uncached.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

applied to mips-next.

We have noticed these build failures on stable tree also. May I request you to initiate the process to get into the stable tree and for the following branches.
- linux-5.10.y
- linux-5.9.y
- linux-5.4.y
- linux-4.19.y

- Naresh