Message ID | 20201127083938.2666770-1-anders.roxell@linaro.org (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | mips: lib: uncached: fix uninitialized variable 'sp' | expand |
On Fri, Nov 27, 2020 at 12:39 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > When building mips tinyconfig with clang the following warning show up: > > /tmp/arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning > register long sp __asm__("$sp"); > ^ > = 0 Hi Anders, thank you for sending the patch. Do you have the full text of the warning; it looks like only the note was included? > > Rework to make an explicit inline move. > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > --- > arch/mips/lib/uncached.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/mips/lib/uncached.c b/arch/mips/lib/uncached.c > index 09d5deea747f..21a4b94a0558 100644 > --- a/arch/mips/lib/uncached.c > +++ b/arch/mips/lib/uncached.c > @@ -37,10 +37,11 @@ > */ > 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 > -- Thanks, ~Nick Desaulniers
On Mon, 30 Nov 2020 at 21:22, Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Nov 27, 2020 at 12:39 AM Anders Roxell <anders.roxell@linaro.org> wrote: > > > > When building mips tinyconfig with clang the following warning show up: > > > > /tmp/arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning > > register long sp __asm__("$sp"); > > ^ > > = 0 > > Hi Anders Hi Nick, >, thank you for sending the patch. Do you have the full text > of the warning; it looks like only the note was included? oops, looks like I missed this: /srv/src/kernel/next/arch/mips/lib/uncached.c:45:6: warning: variable 'sp' is uninitialized when used here [-Wuninitialized] if (sp >= (long)CKSEG0 && sp < (long)CKSEG2) ^~ /srv/src/kernel/next/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. Cheers, Anders > > > > > Rework to make an explicit inline move. > > > > Signed-off-by: Anders Roxell <anders.roxell@linaro.org> > > --- > > arch/mips/lib/uncached.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/mips/lib/uncached.c b/arch/mips/lib/uncached.c > > index 09d5deea747f..21a4b94a0558 100644 > > --- a/arch/mips/lib/uncached.c > > +++ b/arch/mips/lib/uncached.c > > @@ -37,10 +37,11 @@ > > */ > > 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 > > > > > -- > Thanks, > ~Nick Desaulniers
On Fri, 27 Nov 2020, Anders Roxell wrote: > When building mips tinyconfig with clang the following warning show up: > > /tmp/arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning > register long sp __asm__("$sp"); > ^ > = 0 > > Rework to make an explicit inline move. It's not uninitialised, because by definition $sp has been initialised. This code wouldn't have worked for the last 15 years otherwise. It is however non-standard usage (which may not have been clarified at the time this piece was made; I recall extensive discussions around this feature over the years): "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::). [...]" so the change description should refer to that as the rationale rather than an implementation-specific misleading warning (although that can be quoted for a reference of course). > diff --git a/arch/mips/lib/uncached.c b/arch/mips/lib/uncached.c > index 09d5deea747f..21a4b94a0558 100644 > --- a/arch/mips/lib/uncached.c > +++ b/arch/mips/lib/uncached.c > @@ -37,10 +37,11 @@ > */ > 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)); Missing newline between the variable block and code here; you may well swap it with the empty line below you have left. Also use `__asm__' for consistency, and remove the extraneous space ahead of the parenthesis as we don't use them in Linux. Using a tab to separate the mnemonic from operands is also preferable, i.e. "move\t%0, $sp" in this case (using actual tabs works better with multi-line assembly, but this is not one). I think this should be OK once the issues I mentioned here have been addressed. Maciej
On Sun, 6 Dec 2020, Maciej W. Rozycki wrote: > I think this should be OK once the issues I mentioned here have been > addressed. Or alternatively making the variable global would possibly be a better fix as it would let the compiler decide if to waste another register to keep a copy of $sp. Maciej
diff --git a/arch/mips/lib/uncached.c b/arch/mips/lib/uncached.c index 09d5deea747f..21a4b94a0558 100644 --- a/arch/mips/lib/uncached.c +++ b/arch/mips/lib/uncached.c @@ -37,10 +37,11 @@ */ 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);
When building mips tinyconfig with clang the following warning show up: /tmp/arch/mips/lib/uncached.c:40:18: note: initialize the variable 'sp' to silence this warning register long sp __asm__("$sp"); ^ = 0 Rework to make an explicit inline move. Signed-off-by: Anders Roxell <anders.roxell@linaro.org> --- arch/mips/lib/uncached.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)