[PATCH] x86: fix two sparse warnings in early boot string handling

[PATCH] x86: fix two sparse warnings in early boot string handling

[Paul Gortmaker]

Fixes:

arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?

The atou one could be considered a false positive; it seems somehow
caused by including ./string.c from within /compressed/string.c file.
However git grep shows only the atou prototype and declaration, so
it is completely unused and we can hence delete it.

Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: x86@kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 arch/x86/boot/boot.h   | 2 +-
 arch/x86/boot/string.c | 8 --------
 2 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/arch/x86/boot/boot.h b/arch/x86/boot/boot.h
index 50f8c5e0f37e..d318b6b86197 100644
--- a/arch/x86/boot/boot.h
+++ b/arch/x86/boot/boot.h
@@ -344,8 +344,8 @@ void initregs(struct biosregs *regs);
 /* string.c */
 int strcmp(const char *str1, const char *str2);
 int strncmp(const char *cs, const char *ct, size_t count);
+char *strstr(const char *s1, const char *s2);
 size_t strnlen(const char *s, size_t maxlen);
-unsigned int atou(const char *s);
 unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base);
 size_t strlen(const char *s);
 
diff --git a/arch/x86/boot/string.c b/arch/x86/boot/string.c
index 574dedfe2890..59b219c03c77 100644
--- a/arch/x86/boot/string.c
+++ b/arch/x86/boot/string.c
@@ -57,14 +57,6 @@ size_t strnlen(const char *s, size_t maxlen)
 	return (es - s);
 }
 
-unsigned int atou(const char *s)
-{
-	unsigned int i = 0;
-	while (isdigit(*s))
-		i = i * 10 + (*s++ - '0');
-	return i;
-}
-
 /* Works only for digits and letters, but small and fast */
 #define TOLOWER(x) ((x) | 0x20)
 
-- 
1.8.5.2

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[David Rientjes]

On Tue, 11 Feb 2014, Paul Gortmaker wrote:

> Fixes:
> 
> arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
> arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?
> 
> The atou one could be considered a false positive; it seems somehow
> caused by including ./string.c from within /compressed/string.c file.
> However git grep shows only the atou prototype and declaration, so
> it is completely unused and we can hence delete it.
> 

Declaring a prototype in a header file would be pointless if there is no 
current breakage; I don't see why you can't remove strstr() in 
arch/x86/boot/string.c entirely.  What breaks?

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[Paul Gortmaker]

[Re: [PATCH] x86: fix two sparse warnings in early boot string handling] On 11/02/2014 (Tue 14:26) David Rientjes wrote:

> On Tue, 11 Feb 2014, Paul Gortmaker wrote:
> 
> > Fixes:
> > 
> > arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
> > arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?
> > 
> > The atou one could be considered a false positive; it seems somehow
> > caused by including ./string.c from within /compressed/string.c file.
> > However git grep shows only the atou prototype and declaration, so
> > it is completely unused and we can hence delete it.
> > 
> 
> Declaring a prototype in a header file would be pointless if there is no 
> current breakage; I don't see why you can't remove strstr() in 
> arch/x86/boot/string.c entirely.  What breaks?

Explicit breakage vs. sparse warnings are two different things.  It may
be that we can delete strstr() just like I did for atou() -- but in the
interest of doing the minimal change, I did just what was needed for
fixing the sparse warnings for strstr.  I can test if it can be removed,
but it has the smell of generic-libc usage all over it...

Paul.
--
`

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[David Rientjes]

On Tue, 11 Feb 2014, Paul Gortmaker wrote:

> > > Fixes:
> > > 
> > > arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
> > > arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?
> > > 
> > > The atou one could be considered a false positive; it seems somehow
> > > caused by including ./string.c from within /compressed/string.c file.
> > > However git grep shows only the atou prototype and declaration, so
> > > it is completely unused and we can hence delete it.
> > > 
> > 
> > Declaring a prototype in a header file would be pointless if there is no 
> > current breakage; I don't see why you can't remove strstr() in 
> > arch/x86/boot/string.c entirely.  What breaks?
> 
> Explicit breakage vs. sparse warnings are two different things.  It may
> be that we can delete strstr() just like I did for atou() -- but in the
> interest of doing the minimal change, I did just what was needed for
> fixing the sparse warnings for strstr.  I can test if it can be removed,
> but it has the smell of generic-libc usage all over it...
> 

When the minimal change is to add an unnecessary prototype for a function 
that is not referenced, it doesn't seem acceptable.

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[Paul Gortmaker]

On 14-02-11 09:23 PM, David Rientjes wrote:
> On Tue, 11 Feb 2014, Paul Gortmaker wrote:
> 
>>>> Fixes:
>>>>
>>>> arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
>>>> arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?
>>>>
>>>> The atou one could be considered a false positive; it seems somehow
>>>> caused by including ./string.c from within /compressed/string.c file.
>>>> However git grep shows only the atou prototype and declaration, so
>>>> it is completely unused and we can hence delete it.
>>>>
>>>
>>> Declaring a prototype in a header file would be pointless if there is no 
>>> current breakage; I don't see why you can't remove strstr() in 
>>> arch/x86/boot/string.c entirely.  What breaks?
>>
>> Explicit breakage vs. sparse warnings are two different things.  It may
>> be that we can delete strstr() just like I did for atou() -- but in the
>> interest of doing the minimal change, I did just what was needed for
>> fixing the sparse warnings for strstr.  I can test if it can be removed,
>> but it has the smell of generic-libc usage all over it...
>>
> 
> When the minimal change is to add an unnecessary prototype for a function 
> that is not referenced, it doesn't seem acceptable.

OK, fair enough -- it seems surprisingly unused, as well as strcmp, despite
my gut feeling that they'd be used in multiple places.  I'll send a v2
that deletes all three once it passes allyesconfig on linux-next for x86
32/64/uml.

Thanks,
Paul.

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[Paul Gortmaker]

On 14-02-12 09:57 AM, Paul Gortmaker wrote:
> On 14-02-11 09:23 PM, David Rientjes wrote:
>> On Tue, 11 Feb 2014, Paul Gortmaker wrote:
>>
>>>>> Fixes:
>>>>>
>>>>> arch/x86/boot/compressed/../string.c:60:14: warning: symbol 'atou' was not declared. Should it be static?
>>>>> arch/x86/boot/string.c:133:6: warning: symbol 'strstr' was not declared. Should it be static?
>>>>>
>>>>> The atou one could be considered a false positive; it seems somehow
>>>>> caused by including ./string.c from within /compressed/string.c file.
>>>>> However git grep shows only the atou prototype and declaration, so
>>>>> it is completely unused and we can hence delete it.
>>>>>
>>>>
>>>> Declaring a prototype in a header file would be pointless if there is no 
>>>> current breakage; I don't see why you can't remove strstr() in 
>>>> arch/x86/boot/string.c entirely.  What breaks?
>>>
>>> Explicit breakage vs. sparse warnings are two different things.  It may
>>> be that we can delete strstr() just like I did for atou() -- but in the
>>> interest of doing the minimal change, I did just what was needed for
>>> fixing the sparse warnings for strstr.  I can test if it can be removed,
>>> but it has the smell of generic-libc usage all over it...
>>>
>>
>> When the minimal change is to add an unnecessary prototype for a function 
>> that is not referenced, it doesn't seem acceptable.
> 
> OK, fair enough -- it seems surprisingly unused, as well as strcmp, despite
> my gut feeling that they'd be used in multiple places.  I'll send a v2
> that deletes all three once it passes allyesconfig on linux-next for x86
> 32/64/uml.

Actually no v2 pending.  The original v1 patch was/is correct as-is.

While x86-64 defconfig passed, it turns out that both strcmp and strstr
have to stay, else we will get this on i386 allyesconfig builds:

arch/x86/boot/compressed/eboot.o: In function `handle_cmdline_files.isra.5.constprop.6':
eboot.c:(.text+0x4cf): undefined reference to `strstr'
eboot.c:(.text+0x601): undefined reference to `strstr'
make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1

arch/x86/boot/edd.o: In function `query_edd':
arch/x86/boot/edd.c:136: undefined reference to `strcmp'
arch/x86/boot/edd.c:136: undefined reference to `strcmp'
arch/x86/boot/edd.c:140: undefined reference to `strcmp'
arch/x86/boot/edd.c:142: undefined reference to `strcmp'
make[1]: *** [arch/x86/boot/setup.elf] Error 1

So my gut feeling was right after all.  ;)

Thanks,
Paul.
--

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[David Rientjes]

On Wed, 12 Feb 2014, Paul Gortmaker wrote:

> Actually no v2 pending.  The original v1 patch was/is correct as-is.
> 
> While x86-64 defconfig passed, it turns out that both strcmp and strstr
> have to stay, else we will get this on i386 allyesconfig builds:
> 
> arch/x86/boot/compressed/eboot.o: In function `handle_cmdline_files.isra.5.constprop.6':
> eboot.c:(.text+0x4cf): undefined reference to `strstr'
> eboot.c:(.text+0x601): undefined reference to `strstr'
> make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
> 

This means there is a strstr() prototype that is visible to 
drivers/firmware/efi/efi-stub-helper.c but fails at linkage because you've 
removed the definition.  So, again, why would you add a duplicate 
prototype with your patch?

> arch/x86/boot/edd.o: In function `query_edd':
> arch/x86/boot/edd.c:136: undefined reference to `strcmp'
> arch/x86/boot/edd.c:136: undefined reference to `strcmp'
> arch/x86/boot/edd.c:140: undefined reference to `strcmp'
> arch/x86/boot/edd.c:142: undefined reference to `strcmp'
> make[1]: *** [arch/x86/boot/setup.elf] Error 1
> 
> So my gut feeling was right after all.  ;)
> 

I'm not sure what strcmp has to do with this.

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[Paul Gortmaker]

On 14-02-12 05:00 PM, David Rientjes wrote:
> On Wed, 12 Feb 2014, Paul Gortmaker wrote:
> 
>> Actually no v2 pending.  The original v1 patch was/is correct as-is.
>>
>> While x86-64 defconfig passed, it turns out that both strcmp and strstr
>> have to stay, else we will get this on i386 allyesconfig builds:
>>
>> arch/x86/boot/compressed/eboot.o: In function `handle_cmdline_files.isra.5.constprop.6':
>> eboot.c:(.text+0x4cf): undefined reference to `strstr'
>> eboot.c:(.text+0x601): undefined reference to `strstr'
>> make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
>>
> 
> This means there is a strstr() prototype that is visible to 
> drivers/firmware/efi/efi-stub-helper.c but fails at linkage because you've 
> removed the definition. 

Yes, because you suggested removal when you said, in what is
now deleted context text:

 "I don't see why you can't remove strstr() in 
 arch/x86/boot/string.c entirely.  What breaks?"

The above answers your question. The eboot.c breaks.  So
we can't remove strstr.

> So, again, why would you add a duplicate 
> prototype with your patch?

I'm sure there is an implicit path to <linux/string.h>
which allows eboot.c to see a prototype and hence compile.

But that prototype is not associated with the early boot
string.c support.  Those prototypes are in boot.h -- where
my 1st patch added the strstr one to be consistent with
all the other early boot string.c functions.  Is it clear now?

> 
>> arch/x86/boot/edd.o: In function `query_edd':
>> arch/x86/boot/edd.c:136: undefined reference to `strcmp'
>> arch/x86/boot/edd.c:136: undefined reference to `strcmp'
>> arch/x86/boot/edd.c:140: undefined reference to `strcmp'
>> arch/x86/boot/edd.c:142: undefined reference to `strcmp'
>> make[1]: *** [arch/x86/boot/setup.elf] Error 1
>>
>> So my gut feeling was right after all.  ;)
>>
> 
> I'm not sure what strcmp has to do with this.

Per the earlier mail, and you suggestion about removal of
anything unused, this demonstrated that strcmp was also being
used and hence could not be removed either.

Paul.

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[David Rientjes]

On Wed, 12 Feb 2014, Paul Gortmaker wrote:

> > This means there is a strstr() prototype that is visible to 
> > drivers/firmware/efi/efi-stub-helper.c but fails at linkage because you've 
> > removed the definition. 
> 
> Yes, because you suggested removal when you said, in what is
> now deleted context text:
> 
>  "I don't see why you can't remove strstr() in 
>  arch/x86/boot/string.c entirely.  What breaks?"
> 
> The above answers your question. The eboot.c breaks.  So
> we can't remove strstr.
> 

Thanks.

> > So, again, why would you add a duplicate 
> > prototype with your patch?
> 
> I'm sure there is an implicit path to <linux/string.h>
> which allows eboot.c to see a prototype and hence compile.
> 

Nope, linux/string.h only declares the prototype when 
#ifndef __HAVE_ARCH_STRSTR and the 32-bit x86 declaration in 
include/asm/string_32.h properly does #define __HAVE_ARCH_STRSTR.

There's also no #include ordering issue here since linux/string.h does 
#include <asm/string.h> first.

If you had a real problem here, the build would break.  So I'll renew my 
original objection: I don't think it's acceptable to add unneeded 
prototypes because sparse doesn't understand this.

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[H. Peter Anvin]

On 02/12/2014 03:14 PM, David Rientjes wrote:
> On Wed, 12 Feb 2014, Paul Gortmaker wrote:
> 
>>> This means there is a strstr() prototype that is visible to 
>>> drivers/firmware/efi/efi-stub-helper.c but fails at linkage because you've 
>>> removed the definition. 
>>
>> Yes, because you suggested removal when you said, in what is
>> now deleted context text:
>>
>>  "I don't see why you can't remove strstr() in 
>>  arch/x86/boot/string.c entirely.  What breaks?"
>>
>> The above answers your question. The eboot.c breaks.  So
>> we can't remove strstr.
>>
> 
> Thanks.
> 
>>> So, again, why would you add a duplicate 
>>> prototype with your patch?
>>
>> I'm sure there is an implicit path to <linux/string.h>
>> which allows eboot.c to see a prototype and hence compile.
>>
> 
> Nope, linux/string.h only declares the prototype when 
> #ifndef __HAVE_ARCH_STRSTR and the 32-bit x86 declaration in 
> include/asm/string_32.h properly does #define __HAVE_ARCH_STRSTR.
> 
> There's also no #include ordering issue here since linux/string.h does 
> #include <asm/string.h> first.
> 
> If you had a real problem here, the build would break.  So I'll renew my 
> original objection: I don't think it's acceptable to add unneeded 
> prototypes because sparse doesn't understand this.
> 

The real answer IMO ought to be that since arch/x86/boot/string.c is now
used separately from boot.h (eboot.c which includes efi-stub-helper.c
does *not* include boot.h) we may have to move those string functions
into a separate header file.

Matt, do you have any opinions here?

I don't think it is appropriate to leave these warnings in the code
"just because".  We have too many live warnings, some of which are real
bugs, and we need to encourage people to address them instead of leaving
them lost in the noise.

	-hpa

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[Matt Fleming]

On Wed, 12 Feb, at 03:31:05PM, H. Peter Anvin wrote:
> 
> The real answer IMO ought to be that since arch/x86/boot/string.c is now
> used separately from boot.h (eboot.c which includes efi-stub-helper.c
> does *not* include boot.h) we may have to move those string functions
> into a separate header file.
> 
> Matt, do you have any opinions here?
 
This makes sense to me. Perhaps arch/x86/boot/lib/string.c which would
be useable by both arch/x86/boot/*.c and arch/x86/boot/compressed/*.c?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] x86: fix two sparse warnings in early boot string handling

[H. Peter Anvin]

On 02/13/2014 02:31 AM, Matt Fleming wrote:
> On Wed, 12 Feb, at 03:31:05PM, H. Peter Anvin wrote:
>>
>> The real answer IMO ought to be that since arch/x86/boot/string.c is now
>> used separately from boot.h (eboot.c which includes efi-stub-helper.c
>> does *not* include boot.h) we may have to move those string functions
>> into a separate header file.
>>
>> Matt, do you have any opinions here?
>  
> This makes sense to me. Perhaps arch/x86/boot/lib/string.c which would
> be useable by both arch/x86/boot/*.c and arch/x86/boot/compressed/*.c?
> 

That would be fine, but the issue at hand isn't the .c file but the lack
of a corresponding .h file...

	-hpa