The number of sparse errors in the current kernel is staggering, and it
makes sparse a lot less valuable of a tool that it otherwise could be.
On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
Out of those, 12,358 come from linux/err.h. Given that the latter
basically spams *everything*, I can only conclude that almost noone uses
sparse unless they have a filter script.
So a lot of these are certainly nuisance problems, like the
<linux/err.h> stuff which has to do with the handling of error values,
but some of these look like real bugs.
What do we need to do to actually make our tools be able to do work for
us? Newbie projects to clean up? Trying to get the larger Linux
companies to put resources on it?
-hpa
On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
> What do we need to do to actually make our tools be able to do work
> for us? Newbie projects to clean up?
It certainly would be a much better way for newbies to get involved than
all the useless OCD-jerking off that floats back and forth nowadays. And
who knows, newbies might *actually* even learn something while doing
that.
Btw, rw could help with the newbie project, I think.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
> The number of sparse errors in the current kernel is staggering, and it
> makes sparse a lot less valuable of a tool that it otherwise could be.
> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
> Out of those, 12,358 come from linux/err.h. Given that the latter
> basically spams *everything*, I can only conclude that almost noone uses
> sparse unless they have a filter script.
What errors are you seeing from err.h? I don't see those when building
different subdirectories with sparse (which is how I normally use it.)
And what version of sparse are you running:
$ sparse --version
v0.4.5-rc1-407-g345e8943fc36
> So a lot of these are certainly nuisance problems, like the
> <linux/err.h> stuff which has to do with the handling of error values,
> but some of these look like real bugs.
>
> What do we need to do to actually make our tools be able to do work for
> us? Newbie projects to clean up? Trying to get the larger Linux
> companies to put resources on it?
It's not the easiest "newbie" project as usually the first reflex to
"just cast it away" is wrong for a lot of sparse warnings. I know this
from people trying to fix up the sparse warnings in drivers/staging/
thanks,
greg k-h
On Wed, Feb 26, 2014 at 03:28:59PM -0800, Greg KH wrote:
> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
> > The number of sparse errors in the current kernel is staggering, and it
> > makes sparse a lot less valuable of a tool that it otherwise could be.
> > On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
> > Out of those, 12,358 come from linux/err.h. Given that the latter
> > basically spams *everything*, I can only conclude that almost noone uses
> > sparse unless they have a filter script.
>
> What errors are you seeing from err.h? I don't see those when building
> different subdirectories with sparse (which is how I normally use it.)
>
> And what version of sparse are you running:
> $ sparse --version
> v0.4.5-rc1-407-g345e8943fc36
Ah, 0.5.0 is now out, maybe you should update to that version?
thanks,
greg k-h
On 02/26/2014 03:29 PM, Greg KH wrote:
> On Wed, Feb 26, 2014 at 03:28:59PM -0800, Greg KH wrote:
>> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
>>> The number of sparse errors in the current kernel is staggering, and it
>>> makes sparse a lot less valuable of a tool that it otherwise could be.
>>> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
>>> Out of those, 12,358 come from linux/err.h. Given that the latter
>>> basically spams *everything*, I can only conclude that almost noone uses
>>> sparse unless they have a filter script.
>>
>> What errors are you seeing from err.h? I don't see those when building
>> different subdirectories with sparse (which is how I normally use it.)
>>
>> And what version of sparse are you running:
>> $ sparse --version
>> v0.4.5-rc1-407-g345e8943fc36
>
> Ah, 0.5.0 is now out, maybe you should update to that version?
>
Yes... it looks like the 0.4.5-rc1 that shipped in Fedora is indeed out
of date. With 0.5.0 I "only" see 8,207 messages, which means that at
least the linux/err.h issue is gone.
-hpa
On 02/26/2014 03:31 PM, H. Peter Anvin wrote:
> On 02/26/2014 03:29 PM, Greg KH wrote:
>> On Wed, Feb 26, 2014 at 03:28:59PM -0800, Greg KH wrote:
>>> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
>>>> The number of sparse errors in the current kernel is staggering, and it
>>>> makes sparse a lot less valuable of a tool that it otherwise could be.
>>>> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
>>>> Out of those, 12,358 come from linux/err.h. Given that the latter
>>>> basically spams *everything*, I can only conclude that almost noone uses
>>>> sparse unless they have a filter script.
>>>
>>> What errors are you seeing from err.h? I don't see those when building
>>> different subdirectories with sparse (which is how I normally use it.)
>>>
>>> And what version of sparse are you running:
>>> $ sparse --version
>>> v0.4.5-rc1-407-g345e8943fc36
>>
>> Ah, 0.5.0 is now out, maybe you should update to that version?
>>
>
> Yes... it looks like the 0.4.5-rc1 that shipped in Fedora is indeed out
> of date. With 0.5.0 I "only" see 8,207 messages, which means that at
> least the linux/err.h issue is gone.
>
For what it's worth, the rpm is called sparse-0.4.5.rc1-2.fc19.x86_64
and sparse --version reports 0.4.4...
-hpa
On 02/26/2014 03:28 PM, Greg KH wrote:
>>
>> What do we need to do to actually make our tools be able to do work for
>> us? Newbie projects to clean up? Trying to get the larger Linux
>> companies to put resources on it?
>
> It's not the easiest "newbie" project as usually the first reflex to
> "just cast it away" is wrong for a lot of sparse warnings. I know this
> from people trying to fix up the sparse warnings in drivers/staging/
>
I have seen this phenomenon, too. I also see a bunch of sparse warnings
which are clearly bogus, for example complaining about sizeof(bool) when
in bits like:
__this_cpu_write(swallow_nmi, false);
So getting this to the point where it is genuinely useful and can be
made a ubiquitous part of the Linux development process is going to take
more work and probably involve improvements to sparse so we can indicate
in the kernel sources when something is okay or removing completely
bogus warnings, and so on.
The bigger question, again, is what do we need to do to make this
happen, assuming it is worth doing? We certainly have had bugs,
including security holes, which sparse would have caught. At the same
time, this kind of work tends to not be the kind that attract the top
hackers, unfortunately, as it is not "fun".
-hpa
On Wed, 2014-02-26 at 14:49 -0800, H. Peter Anvin wrote:
> The number of sparse errors in the current kernel is staggering, and it
> makes sparse a lot less valuable of a tool that it otherwise could be.
> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
> Out of those, 12,358 come from linux/err.h. Given that the latter
> basically spams *everything*, I can only conclude that almost noone uses
> sparse unless they have a filter script.
>
> So a lot of these are certainly nuisance problems, like the
> <linux/err.h> stuff which has to do with the handling of error values,
> but some of these look like real bugs.
>
> What do we need to do to actually make our tools be able to do work for
> us? Newbie projects to clean up? Trying to get the larger Linux
> companies to put resources on it?
gcc 4.8 does annoyingly warn on all unsigned/signed
mismatches/implicit conversions too.
err.h could also return bool instead of long for the
IS_ERR and IS_ERR_OR_NULL tests.
Maybe something like this could be useful.
Shut up the unsigned<->signed pointer conversions
and implicit conversions in the Makefile.
Use bool not long for IS_ERR and IS_ERR_OR_NULL
Update the dentry description of kernel pointers
left over from 2002's move to err.h
unsigned...
---
Makefile | 1 +
include/linux/err.h | 7 ++++---
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/Makefile b/Makefile
index fce2ba7..a9c11c4 100644
--- a/Makefile
+++ b/Makefile
@@ -381,6 +381,7 @@ KBUILD_CPPFLAGS := -D__KERNEL__
KBUILD_CFLAGS := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
-fno-strict-aliasing -fno-common \
-Werror-implicit-function-declaration \
+ -Wno-sign-conversion -Wno-pointer-sign \
-Wno-format-security \
-fno-delete-null-pointer-checks
KBUILD_AFLAGS_KERNEL :=
diff --git a/include/linux/err.h b/include/linux/err.h
index 15f92e0..a729120 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -2,12 +2,13 @@
#define _LINUX_ERR_H
#include <linux/compiler.h>
+#include <linux/types.h>
#include <asm/errno.h>
/*
* Kernel pointers have redundant information, so we can use a
- * scheme where we can return either an error code or a dentry
+ * scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
@@ -29,12 +30,12 @@ static inline long __must_check PTR_ERR(__force const void *ptr)
return (long) ptr;
}
-static inline long __must_check IS_ERR(__force const void *ptr)
+static inline bool __must_check IS_ERR(__force const void *ptr)
{
return IS_ERR_VALUE((unsigned long)ptr);
}
-static inline long __must_check IS_ERR_OR_NULL(__force const void *ptr)
+static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
{
return !ptr || IS_ERR_VALUE((unsigned long)ptr);
}
On 02/26/2014 04:48 PM, Joe Perches wrote:
> err.h could also return bool instead of long for the
> IS_ERR and IS_ERR_OR_NULL tests.
This is definitely true... although we should check that that doesn't
make the code worse as this is used *all over* the kernel.
> Maybe something like this could be useful.
>
> Shut up the unsigned<->signed pointer conversions
> and implicit conversions in the Makefile.
Sounds like two separate patches to me.
-hpa
On Wed, 2014-02-26 at 16:51 -0800, H. Peter Anvin wrote:
> On 02/26/2014 04:48 PM, Joe Perches wrote:
> > err.h could also return bool instead of long for the
> > IS_ERR and IS_ERR_OR_NULL tests.
>
> This is definitely true... although we should check that that doesn't
> make the code worse as this is used *all over* the kernel.
I tested using arch/x86/crypto/ x86-64 and 32
and there are no .o file differences using
bool or long via objdump -d new and old
> > Maybe something like this could be useful.
> >
> > Shut up the unsigned<->signed pointer conversions
> > and implicit conversions in the Makefile.
>
> Sounds like two separate patches to me.
Yeah, it was just for discussion.
I think gcc 4.8 is overly noisy about those 2.
On Wed, Feb 26, 2014 at 6:37 PM, H. Peter Anvin <[email protected]> wrote:
> On 02/26/2014 03:31 PM, H. Peter Anvin wrote:
>> On 02/26/2014 03:29 PM, Greg KH wrote:
>>> On Wed, Feb 26, 2014 at 03:28:59PM -0800, Greg KH wrote:
>>>> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
>>>>> The number of sparse errors in the current kernel is staggering, and it
>>>>> makes sparse a lot less valuable of a tool that it otherwise could be.
>>>>> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
>>>>> Out of those, 12,358 come from linux/err.h. Given that the latter
>>>>> basically spams *everything*, I can only conclude that almost noone uses
>>>>> sparse unless they have a filter script.
>>>>
>>>> What errors are you seeing from err.h? I don't see those when building
>>>> different subdirectories with sparse (which is how I normally use it.)
>>>>
>>>> And what version of sparse are you running:
>>>> $ sparse --version
>>>> v0.4.5-rc1-407-g345e8943fc36
>>>
>>> Ah, 0.5.0 is now out, maybe you should update to that version?
>>>
>>
>> Yes... it looks like the 0.4.5-rc1 that shipped in Fedora is indeed out
>> of date. With 0.5.0 I "only" see 8,207 messages, which means that at
>> least the linux/err.h issue is gone.
>>
>
> For what it's worth, the rpm is called sparse-0.4.5.rc1-2.fc19.x86_64
> and sparse --version reports 0.4.4...
Fedora rawhide has sparse-0.5.0-1.fc21. If it matters, we could
probably update F20 and F19 with that.
josh
That would be good.
On February 26, 2014 5:19:51 PM PST, Josh Boyer <[email protected]> wrote:
>On Wed, Feb 26, 2014 at 6:37 PM, H. Peter Anvin <[email protected]> wrote:
>> On 02/26/2014 03:31 PM, H. Peter Anvin wrote:
>>> On 02/26/2014 03:29 PM, Greg KH wrote:
>>>> On Wed, Feb 26, 2014 at 03:28:59PM -0800, Greg KH wrote:
>>>>> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
>>>>>> The number of sparse errors in the current kernel is staggering,
>and it
>>>>>> makes sparse a lot less valuable of a tool that it otherwise
>could be.
>>>>>> On a build of x86-64 allyesconfig I'm getting 20,676 sparse
>messages.
>>>>>> Out of those, 12,358 come from linux/err.h. Given that the
>latter
>>>>>> basically spams *everything*, I can only conclude that almost
>noone uses
>>>>>> sparse unless they have a filter script.
>>>>>
>>>>> What errors are you seeing from err.h? I don't see those when
>building
>>>>> different subdirectories with sparse (which is how I normally use
>it.)
>>>>>
>>>>> And what version of sparse are you running:
>>>>> $ sparse --version
>>>>> v0.4.5-rc1-407-g345e8943fc36
>>>>
>>>> Ah, 0.5.0 is now out, maybe you should update to that version?
>>>>
>>>
>>> Yes... it looks like the 0.4.5-rc1 that shipped in Fedora is indeed
>out
>>> of date. With 0.5.0 I "only" see 8,207 messages, which means that
>at
>>> least the linux/err.h issue is gone.
>>>
>>
>> For what it's worth, the rpm is called sparse-0.4.5.rc1-2.fc19.x86_64
>> and sparse --version reports 0.4.4...
>
>Fedora rawhide has sparse-0.5.0-1.fc21. If it matters, we could
>probably update F20 and F19 with that.
>
>josh
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Wed, Feb 26, 2014 at 04:11:30PM -0800, H. Peter Anvin wrote:
> On 02/26/2014 03:28 PM, Greg KH wrote:
> >>
> >> What do we need to do to actually make our tools be able to do work for
> >> us? Newbie projects to clean up? Trying to get the larger Linux
> >> companies to put resources on it?
> >
> > It's not the easiest "newbie" project as usually the first reflex to
> > "just cast it away" is wrong for a lot of sparse warnings. I know this
> > from people trying to fix up the sparse warnings in drivers/staging/
> >
>
> I have seen this phenomenon, too. I also see a bunch of sparse warnings
> which are clearly bogus, for example complaining about sizeof(bool) when
> in bits like:
>
> __this_cpu_write(swallow_nmi, false);
>
> So getting this to the point where it is genuinely useful and can be
> made a ubiquitous part of the Linux development process is going to take
> more work and probably involve improvements to sparse so we can indicate
> in the kernel sources when something is okay or removing completely
> bogus warnings, and so on.
Yes, for some areas of the kernel it will take some work, but for
others, sparse works really well. As an example, building all of
drivers/usb/* with sparse only brings up 2 issues, both of which should
probably be fixed (or annotated properly in the case of the locking
warning.) So keeping things "sparse clean" there is quite easy and part
of taking new patches.
> The bigger question, again, is what do we need to do to make this
> happen, assuming it is worth doing? We certainly have had bugs,
> including security holes, which sparse would have caught. At the same
> time, this kind of work tends to not be the kind that attract the top
> hackers, unfortunately, as it is not "fun".
I guess the first thing would be to do is look to try to fix things like
the "bool" issue you have here. And just grind away at the issues, one
by one. Some of us like doing those types of things, so I'm sure you
can find people willing to do it if you get the word out.
Once we hit a "tipping point" of the kernel being almost sparse clean,
having the main subsystem maintainers always run it would be good. I
know the 0-day bot runs it, as I get patches all the time from it to fix
up some sparse warnings.
greg k-h
Use the more natural return of bool for these tests.
No difference observed in .o files produced by gcc
for x86.
Remove the dentry description of kernel pointers
left over from the 90's and 2002's cleanup move
of parts of fs.h to err.h.
Signed-off-by: Joe Perches <[email protected]>
---
include/linux/err.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/include/linux/err.h b/include/linux/err.h
index 15f92e0..a729120 100644
--- a/include/linux/err.h
+++ b/include/linux/err.h
@@ -2,12 +2,13 @@
#define _LINUX_ERR_H
#include <linux/compiler.h>
+#include <linux/types.h>
#include <asm/errno.h>
/*
* Kernel pointers have redundant information, so we can use a
- * scheme where we can return either an error code or a dentry
+ * scheme where we can return either an error code or a normal
* pointer with the same return value.
*
* This should be a per-architecture thing, to allow different
@@ -29,12 +30,12 @@ static inline long __must_check PTR_ERR(__force const void *ptr)
return (long) ptr;
}
-static inline long __must_check IS_ERR(__force const void *ptr)
+static inline bool __must_check IS_ERR(__force const void *ptr)
{
return IS_ERR_VALUE((unsigned long)ptr);
}
-static inline long __must_check IS_ERR_OR_NULL(__force const void *ptr)
+static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
{
return !ptr || IS_ERR_VALUE((unsigned long)ptr);
}
On 02/26/2014 07:11 PM, H. Peter Anvin wrote:
> On 02/26/2014 03:28 PM, Greg KH wrote:
>>>
>>> What do we need to do to actually make our tools be able to do work for
>>> us? Newbie projects to clean up? Trying to get the larger Linux
>>> companies to put resources on it?
>>
>> It's not the easiest "newbie" project as usually the first reflex to
>> "just cast it away" is wrong for a lot of sparse warnings. I know this
>> from people trying to fix up the sparse warnings in drivers/staging/
>>
>
> I have seen this phenomenon, too. I also see a bunch of sparse warnings
> which are clearly bogus, for example complaining about sizeof(bool) when
> in bits like:
>
> __this_cpu_write(swallow_nmi, false);
>
> So getting this to the point where it is genuinely useful and can be
> made a ubiquitous part of the Linux development process is going to take
> more work and probably involve improvements to sparse so we can indicate
> in the kernel sources when something is okay or removing completely
> bogus warnings, and so on.
>
> The bigger question, again, is what do we need to do to make this
> happen, assuming it is worth doing? We certainly have had bugs,
> including security holes, which sparse would have caught. At the same
> time, this kind of work tends to not be the kind that attract the top
> hackers, unfortunately, as it is not "fun".
Well there was that "should we do a bug-fix-only 4.0 release?" message
from Linus back at the 3.12 release.
Or do like Geert does with the build message regressions/fixes. I always scan
that to make sure none of my work is in it :) (And that could be chunked
up by maintainer).
Just a thought.
Regards,
Peter Hurley
Allow an override to emit or not the sizeof(bool) warning
Signed-off-by: Joe Perches <[email protected]>
---
evaluate.c | 3 ++-
lib.c | 2 ++
lib.h | 1 +
3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 6655615..a45f59b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
}
if (size == 1 && is_bool_type(type)) {
- warning(expr->pos, "expression using sizeof bool");
+ if (Wsizeof_bool)
+ warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
diff --git a/lib.c b/lib.c
index bf3e91c..676b72e 100644
--- a/lib.c
+++ b/lib.c
@@ -225,6 +225,7 @@ int Wone_bit_signed_bitfield = 1;
int Wparen_string = 0;
int Wptr_subtraction_blows = 0;
int Wreturn_void = 0;
+int Wsizeof_bool = 1;
int Wshadow = 0;
int Wtransparent_union = 0;
int Wtypesign = 0;
@@ -437,6 +438,7 @@ static const struct warning {
{ "paren-string", &Wparen_string },
{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
{ "return-void", &Wreturn_void },
+ { "sizeof-bool", &Wsizeof_bool },
{ "shadow", &Wshadow },
{ "transparent-union", &Wtransparent_union },
{ "typesign", &Wtypesign },
diff --git a/lib.h b/lib.h
index f09b338..7e3432f 100644
--- a/lib.h
+++ b/lib.h
@@ -119,6 +119,7 @@ extern int Wone_bit_signed_bitfield;
extern int Wparen_string;
extern int Wptr_subtraction_blows;
extern int Wreturn_void;
+extern int Wsizeof_bool;
extern int Wshadow;
extern int Wtransparent_union;
extern int Wtypesign;
--
1.8.1.2.459.gbcd45b4.dirty
Add an override for sizeof(bool)
Requires sparse 0.5.0+ if -Wsizeof-bool is accepted there.
Signed-off-by: Joe Perches <[email protected]>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index fce2ba7..d9cea07 100644
--- a/Makefile
+++ b/Makefile
@@ -350,7 +350,7 @@ PERL = perl
CHECK = sparse
CHECKFLAGS := -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ \
- -Wbitwise -Wno-return-void $(CF)
+ -Wbitwise -Wno-return-void -Wno-sizeof-bool $(CF)
CFLAGS_MODULE =
AFLAGS_MODULE =
LDFLAGS_MODULE =
On Wed, 2014-02-26 at 17:34 -0800, Greg KH wrote:
> On Wed, Feb 26, 2014 at 04:11:30PM -0800, H. Peter Anvin wrote:
> > I have seen this phenomenon, too. I also see a bunch of sparse warnings
> > which are clearly bogus, for example complaining about sizeof(bool) when
> > in bits like:
> >
> > __this_cpu_write(swallow_nmi, false);
[]
> I guess the first thing would be to do is look to try to fix things like
> the "bool" issue you have here.
I wonder what the "fix" would be as by standard
_Bool doesn't have a defined size.
Probably another sparse -Wno-sizeof-bool flag.
(just added and submitted)
On Wed, Feb 26, 2014 at 06:03:39PM -0800, Joe Perches wrote:
> Allow an override to emit or not the sizeof(bool) warning
>
> Signed-off-by: Joe Perches <[email protected]>
Seems reasonable. However, please document the new flag in the manpage.
> evaluate.c | 3 ++-
> lib.c | 2 ++
> lib.h | 1 +
> 3 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/evaluate.c b/evaluate.c
> index 6655615..a45f59b 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
> }
>
> if (size == 1 && is_bool_type(type)) {
> - warning(expr->pos, "expression using sizeof bool");
> + if (Wsizeof_bool)
> + warning(expr->pos, "expression using sizeof bool");
> size = bits_in_char;
> }
>
> diff --git a/lib.c b/lib.c
> index bf3e91c..676b72e 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -225,6 +225,7 @@ int Wone_bit_signed_bitfield = 1;
> int Wparen_string = 0;
> int Wptr_subtraction_blows = 0;
> int Wreturn_void = 0;
> +int Wsizeof_bool = 1;
> int Wshadow = 0;
> int Wtransparent_union = 0;
> int Wtypesign = 0;
> @@ -437,6 +438,7 @@ static const struct warning {
> { "paren-string", &Wparen_string },
> { "ptr-subtraction-blows", &Wptr_subtraction_blows },
> { "return-void", &Wreturn_void },
> + { "sizeof-bool", &Wsizeof_bool },
> { "shadow", &Wshadow },
> { "transparent-union", &Wtransparent_union },
> { "typesign", &Wtypesign },
> diff --git a/lib.h b/lib.h
> index f09b338..7e3432f 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -119,6 +119,7 @@ extern int Wone_bit_signed_bitfield;
> extern int Wparen_string;
> extern int Wptr_subtraction_blows;
> extern int Wreturn_void;
> +extern int Wsizeof_bool;
> extern int Wshadow;
> extern int Wtransparent_union;
> extern int Wtypesign;
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Allow an override to emit or not the sizeof(bool) warning
Add a description to the manpage.
Signed-off-by: Joe Perches <[email protected]>
---
evaluate.c | 3 ++-
lib.c | 2 ++
lib.h | 1 +
sparse.1 | 9 +++++++++
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 6655615..a45f59b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
}
if (size == 1 && is_bool_type(type)) {
- warning(expr->pos, "expression using sizeof bool");
+ if (Wsizeof_bool)
+ warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
diff --git a/lib.c b/lib.c
index bf3e91c..676b72e 100644
--- a/lib.c
+++ b/lib.c
@@ -225,6 +225,7 @@ int Wone_bit_signed_bitfield = 1;
int Wparen_string = 0;
int Wptr_subtraction_blows = 0;
int Wreturn_void = 0;
+int Wsizeof_bool = 1;
int Wshadow = 0;
int Wtransparent_union = 0;
int Wtypesign = 0;
@@ -437,6 +438,7 @@ static const struct warning {
{ "paren-string", &Wparen_string },
{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
{ "return-void", &Wreturn_void },
+ { "sizeof-bool", &Wsizeof_bool },
{ "shadow", &Wshadow },
{ "transparent-union", &Wtransparent_union },
{ "typesign", &Wtypesign },
diff --git a/lib.h b/lib.h
index f09b338..7e3432f 100644
--- a/lib.h
+++ b/lib.h
@@ -119,6 +119,7 @@ extern int Wone_bit_signed_bitfield;
extern int Wparen_string;
extern int Wptr_subtraction_blows;
extern int Wreturn_void;
+extern int Wsizeof_bool;
extern int Wshadow;
extern int Wtransparent_union;
extern int Wtypesign;
diff --git a/sparse.1 b/sparse.1
index cd6be26..b4546be 100644
--- a/sparse.1
+++ b/sparse.1
@@ -288,6 +288,15 @@ programs consider this poor style, and those programs can use
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wsizeof-bool
+Warn when checking the sizeof a _Bool.
+
+C99 does not specify the sizeof a _Bool. gcc uses 1.
+
+Sparse issues these warnings by default. To turn them off, use
+\fB\-Wno\-sizeof\-bool\fR.
+.
+.TP
.B \-Wshadow
Warn when declaring a symbol which shadows a declaration with the same name in
an outer scope.
--
1.8.1.2.459.gbcd45b4.dirty
On Wed, Feb 26, 2014 at 06:53:14PM -0800, Joe Perches wrote:
> Allow an override to emit or not the sizeof(bool) warning
> Add a description to the manpage.
>
> Signed-off-by: Joe Perches <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
> evaluate.c | 3 ++-
> lib.c | 2 ++
> lib.h | 1 +
> sparse.1 | 9 +++++++++
> 4 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/evaluate.c b/evaluate.c
> index 6655615..a45f59b 100644
> --- a/evaluate.c
> +++ b/evaluate.c
> @@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
> }
>
> if (size == 1 && is_bool_type(type)) {
> - warning(expr->pos, "expression using sizeof bool");
> + if (Wsizeof_bool)
> + warning(expr->pos, "expression using sizeof bool");
> size = bits_in_char;
> }
>
> diff --git a/lib.c b/lib.c
> index bf3e91c..676b72e 100644
> --- a/lib.c
> +++ b/lib.c
> @@ -225,6 +225,7 @@ int Wone_bit_signed_bitfield = 1;
> int Wparen_string = 0;
> int Wptr_subtraction_blows = 0;
> int Wreturn_void = 0;
> +int Wsizeof_bool = 1;
> int Wshadow = 0;
> int Wtransparent_union = 0;
> int Wtypesign = 0;
> @@ -437,6 +438,7 @@ static const struct warning {
> { "paren-string", &Wparen_string },
> { "ptr-subtraction-blows", &Wptr_subtraction_blows },
> { "return-void", &Wreturn_void },
> + { "sizeof-bool", &Wsizeof_bool },
> { "shadow", &Wshadow },
> { "transparent-union", &Wtransparent_union },
> { "typesign", &Wtypesign },
> diff --git a/lib.h b/lib.h
> index f09b338..7e3432f 100644
> --- a/lib.h
> +++ b/lib.h
> @@ -119,6 +119,7 @@ extern int Wone_bit_signed_bitfield;
> extern int Wparen_string;
> extern int Wptr_subtraction_blows;
> extern int Wreturn_void;
> +extern int Wsizeof_bool;
> extern int Wshadow;
> extern int Wtransparent_union;
> extern int Wtypesign;
> diff --git a/sparse.1 b/sparse.1
> index cd6be26..b4546be 100644
> --- a/sparse.1
> +++ b/sparse.1
> @@ -288,6 +288,15 @@ programs consider this poor style, and those programs can use
> Sparse does not issue these warnings by default.
> .
> .TP
> +.B \-Wsizeof-bool
> +Warn when checking the sizeof a _Bool.
> +
> +C99 does not specify the sizeof a _Bool. gcc uses 1.
> +
> +Sparse issues these warnings by default. To turn them off, use
> +\fB\-Wno\-sizeof\-bool\fR.
> +.
> +.TP
> .B \-Wshadow
> Warn when declaring a symbol which shadows a declaration with the same name in
> an outer scope.
> --
> 1.8.1.2.459.gbcd45b4.dirty
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Allow an override to emit or not the sizeof(bool) warning.
Add a "-Wsizeof-bool" description to the manpage.
Signed-off-by: Joe Perches <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
---
v2: Add manpage
v3: gah. Joe knows his alphabet (sometimes). (shadow _then_ sizeof)
evaluate.c | 3 ++-
lib.c | 2 ++
lib.h | 1 +
sparse.1 | 9 +++++++++
4 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 6655615..a45f59b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
}
if (size == 1 && is_bool_type(type)) {
- warning(expr->pos, "expression using sizeof bool");
+ if (Wsizeof_bool)
+ warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
diff --git a/lib.c b/lib.c
index bf3e91c..d6ac79d 100644
--- a/lib.c
+++ b/lib.c
@@ -226,6 +226,7 @@ int Wparen_string = 0;
int Wptr_subtraction_blows = 0;
int Wreturn_void = 0;
int Wshadow = 0;
+int Wsizeof_bool = 1;
int Wtransparent_union = 0;
int Wtypesign = 0;
int Wundef = 0;
@@ -438,6 +439,7 @@ static const struct warning {
{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
{ "return-void", &Wreturn_void },
{ "shadow", &Wshadow },
+ { "sizeof-bool", &Wsizeof_bool },
{ "transparent-union", &Wtransparent_union },
{ "typesign", &Wtypesign },
{ "undef", &Wundef },
diff --git a/lib.h b/lib.h
index f09b338..f6cd9b4 100644
--- a/lib.h
+++ b/lib.h
@@ -120,6 +120,7 @@ extern int Wparen_string;
extern int Wptr_subtraction_blows;
extern int Wreturn_void;
extern int Wshadow;
+extern int Wsizeof_bool;
extern int Wtransparent_union;
extern int Wtypesign;
extern int Wundef;
diff --git a/sparse.1 b/sparse.1
index cd6be26..b7a5b5c 100644
--- a/sparse.1
+++ b/sparse.1
@@ -297,6 +297,15 @@ Such declarations can lead to error-prone code.
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wsizeof-bool
+Warn when checking the sizeof a _Bool.
+
+C99 does not specify the sizeof a _Bool. gcc uses 1.
+
+Sparse issues these warnings by default. To turn them off, use
+\fB\-Wno\-sizeof\-bool\fR.
+.
+.TP
.B \-Wtransparent\-union
Warn about any declaration using the GCC extension
\fB__attribute__((transparent_union))\fR.
--
1.8.1.2.459.gbcd45b4.dirty
On 02/26/2014 06:58 PM, Josh Triplett wrote:
> On Wed, Feb 26, 2014 at 06:53:14PM -0800, Joe Perches wrote:
>> Allow an override to emit or not the sizeof(bool) warning
>> Add a description to the manpage.
>>
>> Signed-off-by: Joe Perches <[email protected]>
>
> Reviewed-by: Josh Triplett <[email protected]>
>
I have to admit that this particular warning is a bit odd to me. I'm
wondering what kind of bugs it was intended to catch.
In particular, things that incorrectly assumes the size of bool to be
anything in particular would seem unlikely to actually use sizeof().
-hpa
(adding Ben Pfaff and Christopher Li)
On Wed, 2014-02-26 at 19:29 -0800, H. Peter Anvin wrote:
> On 02/26/2014 06:58 PM, Josh Triplett wrote:
> > On Wed, Feb 26, 2014 at 06:53:14PM -0800, Joe Perches wrote:
> >> Allow an override to emit or not the sizeof(bool) warning
> >> Add a description to the manpage.
> >>
> >> Signed-off-by: Joe Perches <[email protected]>
> >
> > Reviewed-by: Josh Triplett <[email protected]>
> >
>
> I have to admit that this particular warning is a bit odd to me. I'm
> wondering what kind of bugs it was intended to catch.
>
> In particular, things that incorrectly assumes the size of bool to be
> anything in particular would seem unlikely to actually use sizeof().
Dunno, the commit log for the commit that added it doesn't quite
match the code and is seemingly unaware that the c99 spec doesn't
specify sizeof(bool).
Ben Pfaff <[email protected]>
Date: Wed May 4 16:39:54 2011 -0700
evaluate: Allow sizeof(_Bool) to succeed.
Without this commit, sizeof(_Bool) provokes an error with "cannot size
expression" because _Bool is a 1-bit type and thus not a multiple of a full
byte in size. But sizeof(_Bool) is valid C that should evaluate to 1, so
this commit fixes the problem and adds a regression test.
Signed-off-by: Ben Pfaff <[email protected]>
Signed-off-by: Christopher Li <[email protected]>
On Wed, Feb 26, 2014 at 05:34:24PM -0800, Greg KH wrote:
> Yes, for some areas of the kernel it will take some work, but for
> others, sparse works really well. As an example, building all of
> drivers/usb/* with sparse only brings up 2 issues, both of which should
> probably be fixed (or annotated properly in the case of the locking
> warning.)
Hm. I see 102 in drivers/usb. Mostly in gadget.
http://paste.fedoraproject.org/80787/39347077/raw/
(Note that some of those are duplicates, which would be nice if sparse
would be quieter about)
Dave
sizeof(_Bool), like for many other types, is ABI-dependent, but that doesn't mean it is illegitimate.
I don't think C99 says that it is invalid (which means C99 doesn't permit is to be a packed bitmap.)
On February 26, 2014 7:38:46 PM PST, Joe Perches <[email protected]> wrote:
>(adding Ben Pfaff and Christopher Li)
>
>On Wed, 2014-02-26 at 19:29 -0800, H. Peter Anvin wrote:
>> On 02/26/2014 06:58 PM, Josh Triplett wrote:
>> > On Wed, Feb 26, 2014 at 06:53:14PM -0800, Joe Perches wrote:
>> >> Allow an override to emit or not the sizeof(bool) warning
>> >> Add a description to the manpage.
>> >>
>> >> Signed-off-by: Joe Perches <[email protected]>
>> >
>> > Reviewed-by: Josh Triplett <[email protected]>
>> >
>>
>> I have to admit that this particular warning is a bit odd to me. I'm
>> wondering what kind of bugs it was intended to catch.
>>
>> In particular, things that incorrectly assumes the size of bool to be
>> anything in particular would seem unlikely to actually use sizeof().
>
>Dunno, the commit log for the commit that added it doesn't quite
>match the code and is seemingly unaware that the c99 spec doesn't
>specify sizeof(bool).
>
>Ben Pfaff <[email protected]>
>Date: Wed May 4 16:39:54 2011 -0700
>
> evaluate: Allow sizeof(_Bool) to succeed.
>
> Without this commit, sizeof(_Bool) provokes an error with "cannot size
>expression" because _Bool is a 1-bit type and thus not a multiple of a
>full
>byte in size. But sizeof(_Bool) is valid C that should evaluate to 1,
>so
> this commit fixes the problem and adds a regression test.
>
> Signed-off-by: Ben Pfaff <[email protected]>
> Signed-off-by: Christopher Li <[email protected]>
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Wed, Feb 26, 2014 at 07:38:46PM -0800, Joe Perches wrote:
> (adding Ben Pfaff and Christopher Li)
>
> On Wed, 2014-02-26 at 19:29 -0800, H. Peter Anvin wrote:
> > On 02/26/2014 06:58 PM, Josh Triplett wrote:
> > > On Wed, Feb 26, 2014 at 06:53:14PM -0800, Joe Perches wrote:
> > >> Allow an override to emit or not the sizeof(bool) warning
> > >> Add a description to the manpage.
> > >>
> > >> Signed-off-by: Joe Perches <[email protected]>
> > >
> > > Reviewed-by: Josh Triplett <[email protected]>
> > >
> >
> > I have to admit that this particular warning is a bit odd to me. I'm
> > wondering what kind of bugs it was intended to catch.
> >
> > In particular, things that incorrectly assumes the size of bool to be
> > anything in particular would seem unlikely to actually use sizeof().
>
> Dunno, the commit log for the commit that added it doesn't quite
> match the code and is seemingly unaware that the c99 spec doesn't
> specify sizeof(bool).
The commit *relaxed* sparse behavior: because previously sizeof(bool)
was an error. I'm not in favor of any diagnostic at all for
sizeof(bool), but my recollection is that a sparse maintainer wanted it
to yield one.
I don't care about the particular result for sizeof(bool) as long as it
matches the ABI.
On 02/26/2014 05:52 PM, Peter Hurley wrote:
>
> Well there was that "should we do a bug-fix-only 4.0 release?" message
> from Linus back at the 3.12 release.
>
Sure... but will it actually happen?
-hpa
On 02/26/2014 08:00 PM, Ben Pfaff wrote:
>
> The commit *relaxed* sparse behavior: because previously sizeof(bool)
> was an error. I'm not in favor of any diagnostic at all for
> sizeof(bool), but my recollection is that a sparse maintainer wanted it
> to yield one.
>
Still not clear as to why.
-hpa
On Wed, Feb 26, 2014 at 08:19:57PM -0800, H. Peter Anvin wrote:
> On 02/26/2014 08:00 PM, Ben Pfaff wrote:
> >
> > The commit *relaxed* sparse behavior: because previously sizeof(bool)
> > was an error. I'm not in favor of any diagnostic at all for
> > sizeof(bool), but my recollection is that a sparse maintainer wanted it
> > to yield one.
>
> Still not clear as to why.
The discussion is here:
http://comments.gmane.org/gmane.comp.parsers.sparse/2462
Quoting from that discussion, the core of Christopher Li's argument was
this:
> On Mon, May 9, 2011 at 1:02 PM, Ben Pfaff <blp <at> nicira.com> wrote:
> > Thank you for applying my patch. It does work for me, in the sense
> > that I get a warning instead of an error now, but I'm not so happy to
> > get any diagnostic at all. Is there some reason why sizeof(_Bool)
> > warrants a warning when, say, sizeof(long) does not? After all, both
> > sizes are implementation defined.
> Because sizeof(_Bool) is a little bit special compare to sizeof(long).
> In the case of long, all sizeof(long) * 8 bits are use in the actual value.
> But for the _Bool, only the 1 bit is used in the 8 bits size. In other words,
> the _Bool has a special case of the actual bit size is not a multiple of 8.
> Sparse has two hats, it is a C compiler front end, and more often it is
> used in the Linux kernel source sanitize checking. Depending on the sizeof
> _Bool sounds a little bit suspicious in the kernel. I would love to the heard
> your actual usage case of the sizeof(_Bool). Why do you care about this
> warning?
On Wed, Feb 26, 2014 at 08:19:23PM -0800, H. Peter Anvin wrote:
> On 02/26/2014 05:52 PM, Peter Hurley wrote:
> >
> > Well there was that "should we do a bug-fix-only 4.0 release?" message
> > from Linus back at the 3.12 release.
> >
>
> Sure... but will it actually happen?
I sure hope not, the backlog it would cause would be immense.
greg k-h
On Wed, Feb 26, 2014 at 10:15:08PM -0500, Dave Jones wrote:
> On Wed, Feb 26, 2014 at 05:34:24PM -0800, Greg KH wrote:
>
> > Yes, for some areas of the kernel it will take some work, but for
> > others, sparse works really well. As an example, building all of
> > drivers/usb/* with sparse only brings up 2 issues, both of which should
> > probably be fixed (or annotated properly in the case of the locking
> > warning.)
>
> Hm. I see 102 in drivers/usb. Mostly in gadget.
> http://paste.fedoraproject.org/80787/39347077/raw/
Ick, gadget, I don't build that for my systems as I don't have that
hardware, sorry, I forgot to take that into consideration here.
Hey, at least I know what I'm going to be doing tomorrow :)
greg k-h
On 02/26/2014 08:26 PM, Ben Pfaff wrote:
>
>> Because sizeof(_Bool) is a little bit special compare to sizeof(long).
>> In the case of long, all sizeof(long) * 8 bits are use in the actual value.
>> But for the _Bool, only the 1 bit is used in the 8 bits size. In other words,
>> the _Bool has a special case of the actual bit size is not a multiple of 8.
Quite frankly, this is silly in my opinion, *and* it is not guaranteed
by C either (read about "trap representations").
>> Sparse has two hats, it is a C compiler front end, and more often it is
>> used in the Linux kernel source sanitize checking. Depending on the sizeof
>> _Bool sounds a little bit suspicious in the kernel. I would love to the heard
>> your actual usage case of the sizeof(_Bool). Why do you care about this
>> warning?
Anything that moves data around in a generic fashion. It can be as
simple as:
memcpy(foo, bar, sizeof *foo);
-hpa
On Wed, Feb 26, 2014 at 07:42:37PM -0800, H. Peter Anvin wrote:
> sizeof(_Bool), like for many other types, is ABI-dependent, but that
> doesn't mean it is illegitimate.
>
> I don't think C99 says that it is invalid (which means C99 doesn't
> permit is to be a packed bitmap.)
Ok, but what can be said about the __pcpu_size_call() use case where we
do sizeof(bool)? We have there accessors for sizes 1,2,4 and 8. Can we
simply assume that the ABI will give us a size of bool which is one of
those?
What if sizeof(bool) is 3?
Or, are we saying that sizeof(bool) will always be of some natural,
native size like byte, short, int or long so we're good there?
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
Am 27.02.2014 00:25, schrieb Borislav Petkov:
> On Wed, Feb 26, 2014 at 02:49:26PM -0800, H. Peter Anvin wrote:
>> What do we need to do to actually make our tools be able to do work
>> for us? Newbie projects to clean up?
>
> It certainly would be a much better way for newbies to get involved than
> all the useless OCD-jerking off that floats back and forth nowadays. And
> who knows, newbies might *actually* even learn something while doing
> that.
>
> Btw, rw could help with the newbie project, I think.
Yeah, on #kernelnewbies newbies often ask how they can help.
Maybe a short info on http://kernelnewbies.org/ how to deal with sparse
would help too.
Thanks,
//richard
On Thu, Feb 27, 2014 at 2:52 AM, Peter Hurley <[email protected]> wrote:
>> The bigger question, again, is what do we need to do to make this
>> happen, assuming it is worth doing? We certainly have had bugs,
>> including security holes, which sparse would have caught. At the same
>> time, this kind of work tends to not be the kind that attract the top
>> hackers, unfortunately, as it is not "fun".
>
> Well there was that "should we do a bug-fix-only 4.0 release?" message
> from Linus back at the 3.12 release.
>
> Or do like Geert does with the build message regressions/fixes. I always
> scan
> that to make sure none of my work is in it :) (And that could be chunked
> up by maintainer).
A quick test shows that my scripts would catch (many) sparse errors and
warnings too, iff they would be in the kissb build logs.
So as soon as kissb starts building with C=1, we can start tracking sparse
regressions. The first report would contain _lots_ of regressions, though ;-)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
>> So getting this to the point where it is genuinely useful and can be
>> made a ubiquitous part of the Linux development process is going to take
>> more work and probably involve improvements to sparse so we can indicate
>> in the kernel sources when something is okay or removing completely
>> bogus warnings, and so on.
>
> Yes, for some areas of the kernel it will take some work, but for
> others, sparse works really well. As an example, building all of
Works quite nicely for me. I run both spatch and smatch on drivers/hwmon
and (partially) on drivers/watchdog. There is only one (false) warning
in hwmon from spatch, plus about a dozen smatch warnings. I find it very
valuable; even if warnings are false positives they often point to
less than perfect code.
I filter out some noise from smatch, but at least so far I did not have
to do any filtering for spatch.
Guenter
* H. Peter Anvin ([email protected]) wrote:
> The number of sparse errors in the current kernel is staggering, and it
> makes sparse a lot less valuable of a tool that it otherwise could be.
> On a build of x86-64 allyesconfig I'm getting 20,676 sparse messages.
> Out of those, 12,358 come from linux/err.h. Given that the latter
> basically spams *everything*, I can only conclude that almost noone uses
> sparse unless they have a filter script.
I did a bit of sparse fixing a few years ago; my strategy then was to
get used to which types of warnings were more likely to be real
and ignore all the rest - it was the only way to find anything useful from
them.
There were some that were very fruitful; the warnings for gfp_t types were
great at spotting where someone had swapped the parameters to kmalloc
for example.
Dave
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ gro.gilbert @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
Keep in mind, too, that for the kernel we don't care about the full C standard but a subset. We rely on extrastandard behavior all over the place. For all ABIs supported by the kernel, sizeof(_Book) == 1 and so everything is sane.
On February 27, 2014 12:25:29 AM PST, Borislav Petkov <[email protected]> wrote:
>On Wed, Feb 26, 2014 at 07:42:37PM -0800, H. Peter Anvin wrote:
>> sizeof(_Bool), like for many other types, is ABI-dependent, but that
>> doesn't mean it is illegitimate.
>>
>> I don't think C99 says that it is invalid (which means C99 doesn't
>> permit is to be a packed bitmap.)
>
>Ok, but what can be said about the __pcpu_size_call() use case where we
>do sizeof(bool)? We have there accessors for sizes 1,2,4 and 8. Can we
>simply assume that the ABI will give us a size of bool which is one of
>those?
>
>What if sizeof(bool) is 3?
>
>Or, are we saying that sizeof(bool) will always be of some natural,
>native size like byte, short, int or long so we're good there?
>
>Thanks.
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Thu, Feb 27, 2014 at 07:10:25AM -0800, H. Peter Anvin wrote:
> Keep in mind, too, that for the kernel we don't care about the full
> C standard but a subset. We rely on extrastandard behavior all over
> the place. For all ABIs supported by the kernel, sizeof(_Book) == 1
> and so everything is sane.
Do we have a fairly comprehensive list of what these extrastandard
requirements / assumptions are? It might be a good idea to have one
that we can point to, so that (a) people who are trying to define a
new architecture knows what they need to handle, (b) and so we can
give a list of things that static code analyzers like smatch and
coverity and sparse should be able to suppress (perhaps in a Linux
kernel-only mode).
- Ted
On 02/27/2014 07:24 AM, Theodore Ts'o wrote:
> On Thu, Feb 27, 2014 at 07:10:25AM -0800, H. Peter Anvin wrote:
>> Keep in mind, too, that for the kernel we don't care about the full
>> C standard but a subset. We rely on extrastandard behavior all over
>> the place. For all ABIs supported by the kernel, sizeof(_Book) == 1
>> and so everything is sane.
>
> Do we have a fairly comprehensive list of what these extrastandard
> requirements / assumptions are? It might be a good idea to have one
> that we can point to, so that (a) people who are trying to define a
> new architecture knows what they need to handle, (b) and so we can
> give a list of things that static code analyzers like smatch and
> coverity and sparse should be able to suppress (perhaps in a Linux
> kernel-only mode).
>
No, but I think we can certainly make a list... a lot of it right now
sits in various people's heads.
Here are a couple:
- Bytes are 8 bits
- Signed integers will be 2's complement
- sizeof char, short, int, long, and long long will be 1, 2, 4, 4, 8 or
1, 2, 4, 8, 8 on 32- and 64-bit processors, respectively.
- sizeof(long) == sizeof(void *)
- NULL is represented by all zero
- Structures will not add padding as long as all the members are
naturally aligned.
Someone want to set up a collaborative document of some kind and collect
more?
-hpa
On Thu, Feb 27, 2014 at 07:48:35AM -0800, H. Peter Anvin wrote:
> No, but I think we can certainly make a list... a lot of it right now
> sits in various people's heads.
>
> Here are a couple:
>
> - Bytes are 8 bits
> - Signed integers will be 2's complement
> - sizeof char, short, int, long, and long long will be 1, 2, 4, 4, 8 or
^ ^
bool, ,1 ,
> 1, 2, 4, 8, 8 on 32- and 64-bit processors, respectively.
> - sizeof(long) == sizeof(void *)
> - NULL is represented by all zero
> - Structures will not add padding as long as all the members are
> naturally aligned.
>
> Someone want to set up a collaborative document of some kind and collect
> more?
https://wiki.kernel.org/ but I hear spammers love it.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine.
--
On Thu, Feb 27, 2014 at 07:48:35AM -0800, H. Peter Anvin wrote:
> > Do we have a fairly comprehensive list of what these extrastandard
> > requirements / assumptions are? It might be a good idea to have one
> > that we can point to, so that (a) people who are trying to define a
> > new architecture knows what they need to handle, (b) and so we can
> > give a list of things that static code analyzers like smatch and
> > coverity and sparse should be able to suppress (perhaps in a Linux
> > kernel-only mode).
> >
>
> No, but I think we can certainly make a list... a lot of it right now
> sits in various people's heads.
>
> Here are a couple:
>
> - Bytes are 8 bits
> - Signed integers will be 2's complement
> - sizeof char, short, int, long, and long long will be 1, 2, 4, 4, 8 or
> 1, 2, 4, 8, 8 on 32- and 64-bit processors, respectively.
> - sizeof(long) == sizeof(void *)
> - NULL is represented by all zero
> - Structures will not add padding as long as all the members are
> naturally aligned.
>
That last assumption has to change for the Meta architecture.
https://lwn.net/Articles/522188/
On meta, the structs and unions are padded to 4 bytes unless they are
explicitly marked as __packed.
regards,
dan carpenter
On 02/27/2014 08:10 AM, Dan Carpenter wrote:
>
> That last assumption has to change for the Meta architecture.
>
> https://lwn.net/Articles/522188/
>
> On meta, the structs and unions are padded to 4 bytes unless they are
> explicitly marked as __packed.
>
One have to wonder how likely they are to have that not continually break...
-hpa
On 27/02/14 16:52, H. Peter Anvin wrote:
> On 02/27/2014 08:10 AM, Dan Carpenter wrote:
>>
>> That last assumption has to change for the Meta architecture.
>>
>> https://lwn.net/Articles/522188/
>>
>> On meta, the structs and unions are padded to 4 bytes unless they are
>> explicitly marked as __packed.
>>
>
> One have to wonder how likely they are to have that not continually break...
Well it's only when it deals with stored/portable data formats that it's
an issue, and in those cases some amount of care over packing has
usually already been taken. It most often causes problems when one small
struct/union containing only chars or shorts is nested inside another
struct which is marked as packed (on the unfortunately mistaken
assumption that the inner struct cannot have padding at the end so
doesn't need packing itself).
But yeh, it is a PITA.
Cheers
James
On Wed, Feb 26, 2014 at 8:32 PM, H. Peter Anvin <[email protected]> wrote:
>
> Quite frankly, this is silly in my opinion, *and* it is not guaranteed
> by C either (read about "trap representations").
>>
> Anything that moves data around in a generic fashion. It can be as
> simple as:
>
> memcpy(foo, bar, sizeof *foo);
OK. I get it nobody wants a sizeof(_Bool) warning.
I am going to apply this patch.
Should we change the default to off then?
Thanks.
Chris
I would.
On February 27, 2014 12:22:45 PM PST, Christopher Li <[email protected]> wrote:
>On Wed, Feb 26, 2014 at 8:32 PM, H. Peter Anvin <[email protected]> wrote:
>>
>> Quite frankly, this is silly in my opinion, *and* it is not
>guaranteed
>> by C either (read about "trap representations").
>>>
>> Anything that moves data around in a generic fashion. It can be as
>> simple as:
>>
>> memcpy(foo, bar, sizeof *foo);
>
>OK. I get it nobody wants a sizeof(_Bool) warning.
>
>I am going to apply this patch.
>
>Should we change the default to off then?
>
>Thanks.
>
>Chris
--
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Thu, 2014-02-27 at 12:26 -0800, H. Peter Anvin wrote:
> On February 27, 2014 12:22:45 PM PST, Christopher Li <[email protected]> wrote:
> >OK. I get it nobody wants a sizeof(_Bool) warning.
> >I am going to apply this patch.
Please use V3 as I stuffed up the alphabetic order
of sizeof and shadow.
> >Should we change the default to off then?
> I would.
I'm not sure it matters much, but the linux-kernel
Makefile wouldn't need to be changed if Wsizeof_bool
is default 0.
Here's a couple of other nits:
Maybe the evaluate.c "size = bits_in_char;" assignment
if (size == 1 && is_bool_type(type)) {
- warning(expr->pos, "expression using sizeof bool");
+ if (Wsizeof_bool)
+ warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
should be
size = sizeof(_Bool) * 8;
And also, in sparse.1, Josh Triplett is shown as
the maintainer. Maybe that should be changed to
Christopher Li
On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin <[email protected]> wrote:
> I would.
>
Joe, I assume you are OK with this patch, the default is now off.
Chris
Allow an override to emit or not the sizeof(bool) warning.
Add a "-Wsizeof-bool" description to the manpage.
Signed-off-by: Joe Perches <[email protected]>
Reviewed-by: Josh Triplett <[email protected]>
Signed-off-by: Christopher Li <[email protected]>
---
evaluate.c | 3 ++-
lib.c | 2 ++
lib.h | 1 +
sparse.1 | 8 ++++++++
4 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/evaluate.c b/evaluate.c
index 6655615..a45f59b 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2057,7 +2057,8 @@ static struct symbol *evaluate_sizeof(struct
expression *expr)
}
if (size == 1 && is_bool_type(type)) {
- warning(expr->pos, "expression using sizeof bool");
+ if (Wsizeof_bool)
+ warning(expr->pos, "expression using sizeof bool");
size = bits_in_char;
}
diff --git a/lib.c b/lib.c
index 51b97fd..844797d 100644
--- a/lib.c
+++ b/lib.c
@@ -226,6 +226,7 @@ int Wparen_string = 0;
int Wptr_subtraction_blows = 0;
int Wreturn_void = 0;
int Wshadow = 0;
+int Wsizeof_bool = 0;
int Wtransparent_union = 0;
int Wtypesign = 0;
int Wundef = 0;
@@ -438,6 +439,7 @@ static const struct warning {
{ "ptr-subtraction-blows", &Wptr_subtraction_blows },
{ "return-void", &Wreturn_void },
{ "shadow", &Wshadow },
+ { "sizeof-bool", &Wsizeof_bool },
{ "transparent-union", &Wtransparent_union },
{ "typesign", &Wtypesign },
{ "undef", &Wundef },
diff --git a/lib.h b/lib.h
index f09b338..f6cd9b4 100644
--- a/lib.h
+++ b/lib.h
@@ -120,6 +120,7 @@ extern int Wparen_string;
extern int Wptr_subtraction_blows;
extern int Wreturn_void;
extern int Wshadow;
+extern int Wsizeof_bool;
extern int Wtransparent_union;
extern int Wtypesign;
extern int Wundef;
diff --git a/sparse.1 b/sparse.1
index cd6be26..54da09b 100644
--- a/sparse.1
+++ b/sparse.1
@@ -297,6 +297,14 @@ Such declarations can lead to error-prone code.
Sparse does not issue these warnings by default.
.
.TP
+.B \-Wsizeof-bool
+Warn when checking the sizeof a _Bool.
+
+C99 does not specify the sizeof a _Bool. gcc uses 1.
+
+Sparse does not issue these warnings by default.
+.
+.TP
.B \-Wtransparent\-union
Warn about any declaration using the GCC extension
\fB__attribute__((transparent_union))\fR.
--
1.8.5.3
On Thu, Feb 27, 2014 at 12:39 PM, Joe Perches <[email protected]> wrote:
> Please use V3 as I stuffed up the alphabetic order
> of sizeof and shadow.
Please send it your V3 then :-)
>
> I'm not sure it matters much, but the linux-kernel
> Makefile wouldn't need to be changed if Wsizeof_bool
> is default 0.
It seems default to off is the right thing to do.
>
> Here's a couple of other nits:
>
> Maybe the evaluate.c "size = bits_in_char;" assignment
>
> if (size == 1 && is_bool_type(type)) {
> - warning(expr->pos, "expression using sizeof bool");
> + if (Wsizeof_bool)
> + warning(expr->pos, "expression using sizeof bool");
> size = bits_in_char;
> }
>
> should be
>
> size = sizeof(_Bool) * 8;
The reason to use bits_in_xxxx is to allow sparse application to over write
the size of int etc. If you don't like the bits_in_char here. You can introduce
bits_in_bool and set that to sizeof(Bool)*8 by default. That way you don't
hard code it.
> And also, in sparse.1, Josh Triplett is shown as
> the maintainer. Maybe that should be changed to
> Christopher Li
Maybe a separate patch.
Waiting for your V3.
Chris
On Thu, 2014-02-27 at 12:44 -0800, Christopher Li wrote:
> On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin <[email protected]> wrote:
> > I would.
> Joe, I assume you are OK with this patch, the default is now off.
Of course
> diff --git a/lib.c b/lib.c
[]
> @@ -226,6 +226,7 @@ int Wparen_string = 0;
> +int Wsizeof_bool = 0;
On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches <[email protected]> wrote:
> On Thu, 2014-02-27 at 12:44 -0800, Christopher Li wrote:
>> On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin <[email protected]> wrote:
>> > I would.
>> Joe, I assume you are OK with this patch, the default is now off.
>
> Of course
Let me know if you are going to have a V3 or not.
Thanks
Chris
On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches <[email protected]> wrote:
> Of course
>
The change has applied and pushed.
Chris
On Thu, 2014-02-27 at 12:55 -0800, Christopher Li wrote:
> On Thu, Feb 27, 2014 at 12:39 PM, Joe Perches <[email protected]> wrote:
> > Maybe the evaluate.c "size = bits_in_char;" assignment
> >
> > if (size == 1 && is_bool_type(type)) {
> > - warning(expr->pos, "expression using sizeof bool");
> > + if (Wsizeof_bool)
> > + warning(expr->pos, "expression using sizeof bool");
> > size = bits_in_char;
> > }
> >
> > should be
> >
> > size = sizeof(_Bool) * 8;
>
> The reason to use bits_in_xxxx is to allow sparse application to over write
> the size of int etc. If you don't like the bits_in_char here. You can introduce
> bits_in_bool and set that to sizeof(Bool)*8 by default. That way you don't
> hard code it.
There already is a bits_in_bool and it's default 1.
$ git grep -E "\bbits_in_\w+\s*=[^=]"
lib.c: bits_in_long = 64;
lib.c: bits_in_pointer = 64;
target.c:int bits_in_bool = 1;
target.c:int bits_in_char = 8;
target.c:int bits_in_short = 16;
target.c:int bits_in_int = 32;
target.c:int bits_in_long = 32;
target.c:int bits_in_longlong = 64;
target.c:int bits_in_longlonglong = 128;
target.c:int bits_in_float = 32;
target.c:int bits_in_double = 64;
target.c:int bits_in_longdouble = 80;
target.c:int bits_in_pointer = 32;
target.c:int bits_in_enum = 32;
> > And also, in sparse.1, Josh Triplett is shown as
> > the maintainer. Maybe that should be changed to
> > Christopher Li
>
> Maybe a separate patch.
That's fine with me too. If you're the maintainer,
I think you should do that patch. I don't see a need
for me to send any more right now though.
cheers, Joe