[PATCH] sparse: Allow override of sizeof(bool) warning

[PATCH] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

Allow an override to emit or not the sizeof(bool) warning

Signed-off-by: Joe Perches <joe@perches.com>
---
 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

Re: [PATCH] sparse: Allow override of sizeof(bool) warning

[Josh Triplett]

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 <joe@perches.com>

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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V2] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

Allow an override to emit or not the sizeof(bool) warning
Add a description to the manpage.

Signed-off-by: Joe Perches <joe@perches.com>
---
 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Josh Triplett]

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 <joe@perches.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V3] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

Allow an override to emit or not the sizeof(bool) warning.
Add a "-Wsizeof-bool" description to the manpage.

Signed-off-by: Joe Perches <joe@perches.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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 <joe@perches.com>
> 
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> 

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

(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 <joe@perches.com>
> > 
> > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> > 
> 
> 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 <blp@nicira.com>
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 <blp@nicira.com>
    Signed-off-by: Christopher Li <sparse@chrisli.org>

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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 <joe@perches.com> 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 <joe@perches.com>
>> > 
>> > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
>> > 
>> 
>> 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 <blp@nicira.com>
>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 <blp@nicira.com>
>    Signed-off-by: Christopher Li <sparse@chrisli.org>

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Borislav Petkov]

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.
--

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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 <bp@alien8.de> 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.

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Theodore Ts'o]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Borislav Petkov]

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.
--

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Dan Carpenter]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[James Hogan]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Ben Pfaff]

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 <joe@perches.com>
> > > 
> > > Reviewed-by: Josh Triplett <josh@joshtriplett.org>
> > > 
> > 
> > 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.

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Ben Pfaff]

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?

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Christopher Li]

On Wed, Feb 26, 2014 at 8:32 PM, H. Peter Anvin <hpa@zytor.com> 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[H. Peter Anvin]

I would.

On February 27, 2014 12:22:45 PM PST, Christopher Li <sparse@chrisli.org> wrote:
>On Wed, Feb 26, 2014 at 8:32 PM, H. Peter Anvin <hpa@zytor.com> 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.

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

On Thu, 2014-02-27 at 12:26 -0800, H. Peter Anvin wrote:
> On February 27, 2014 12:22:45 PM PST, Christopher Li <sparse@chrisli.org> 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Christopher Li]

On Thu, Feb 27, 2014 at 12:39 PM, Joe Perches <joe@perches.com> 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

On Thu, 2014-02-27 at 12:55 -0800, Christopher Li wrote:
> On Thu, Feb 27, 2014 at 12:39 PM, Joe Perches <joe@perches.com> 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Christopher Li]

On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin <hpa@zytor.com> 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 <joe@perches.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
Signed-off-by: Christopher Li <sparse@chrisli.org>
---
 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Joe Perches]

On Thu, 2014-02-27 at 12:44 -0800, Christopher Li wrote:
> On Thu, Feb 27, 2014 at 12:26 PM, H. Peter Anvin <hpa@zytor.com> 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;

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Christopher Li]

On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches <joe@perches.com> 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 <hpa@zytor.com> 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

Re: [PATCH V2] sparse: Allow override of sizeof(bool) warning

[Christopher Li]

On Thu, Feb 27, 2014 at 1:00 PM, Joe Perches <joe@perches.com> wrote:

> Of course
>

The change has applied and pushed.

Chris