[PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Ben Pfaff]

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>
---
 evaluate.c               |    4 +++-
 symbol.h                 |    7 +++++++
 validation/sizeof-bool.c |    9 +++++++++
 3 files changed, 19 insertions(+), 1 deletions(-)
 create mode 100644 validation/sizeof-bool.c

diff --git a/evaluate.c b/evaluate.c
index f8343c2..f196dbc 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2033,7 +2033,9 @@ static struct symbol *evaluate_sizeof(struct expression *expr)
 		size = bits_in_char;
 	}
 
-	if ((size < 0) || (size & (bits_in_char - 1)))
+	if (is_bool_type(type))
+		size = bits_in_char;
+	else if ((size < 0) || (size & (bits_in_char - 1)))
 		expression_error(expr, "cannot size expression");
 
 	expr->type = EXPR_VALUE;
diff --git a/symbol.h b/symbol.h
index e567305..2b8f20e 100644
--- a/symbol.h
+++ b/symbol.h
@@ -346,6 +346,13 @@ static inline int is_void_type(struct symbol *type)
 	return type == &void_ctype;
 }
 
+static inline int is_bool_type(struct symbol *type)
+{
+	if (type->type == SYM_NODE)
+		type = type->ctype.base_type;
+	return type == &bool_ctype;
+}
+
 static inline int is_function(struct symbol *type)
 {
 	return type && type->type == SYM_FN;
diff --git a/validation/sizeof-bool.c b/validation/sizeof-bool.c
new file mode 100644
index 0000000..dfcb12a
--- /dev/null
+++ b/validation/sizeof-bool.c
@@ -0,0 +1,9 @@
+static int a(void)
+{
+	return sizeof(_Bool);
+}
+/*
+ * check-name: sizeof(_Bool) is valid
+ * check-description: sizeof(_Bool) was rejected because _Bool is not an even
+ * number of bytes
+ */
-- 
1.7.4.4

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Christopher Li]

On Wed, May 4, 2011 at 4:39 PM, Ben Pfaff <blp@nicira.com> wrote:
> 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.

Thanks for the patch.

The sizeof _Bool is implementation define. Gcc make sizeof(_Bool) as 1.
I modify your patch to issue an warning. Applied and pushed.

The incremental change follows. Please check that works for you or not.

Chris

diff --git a/evaluate.c b/evaluate.c
index f196dbc..1309001 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2028,14 +2028,18 @@ static struct symbol *evaluate_sizeof(struct
expression *expr)
 		size = bits_in_char;
 	}

+	if (size == 1 && is_bool_type(type)) {
+		warning(expr->pos, "expression using sizeof bool");
+		size = bits_in_char;
+	}
+
 	if (is_function(type->ctype.base_type)) {
 		warning(expr->pos, "expression using sizeof on a function");
 		size = bits_in_char;
 	}

-	if (is_bool_type(type))
-		size = bits_in_char;
-	else if ((size < 0) || (size & (bits_in_char - 1)))
+
+	if ((size < 0) || (size & (bits_in_char - 1)))
 		expression_error(expr, "cannot size expression");

 	expr->type = EXPR_VALUE;
diff --git a/validation/sizeof-bool.c b/validation/sizeof-bool.c
index dfcb12a..6c68748 100644
--- a/validation/sizeof-bool.c
+++ b/validation/sizeof-bool.c
@@ -6,4 +6,7 @@ static int a(void)
  * check-name: sizeof(_Bool) is valid
  * check-description: sizeof(_Bool) was rejected because _Bool is not an even
  * number of bytes
+ * check-error-start
+sizeof-bool.c:3:16: warning: expression using sizeof bool
+ * check-error-end
  */
--
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

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Ben Pfaff]

On Sat, May 07, 2011 at 01:37:04PM -0700, Christopher Li wrote:
> On Wed, May 4, 2011 at 4:39 PM, Ben Pfaff <blp@nicira.com> wrote:
> > 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.
> 
> The sizeof _Bool is implementation define. Gcc make sizeof(_Bool) as 1.
> I modify your patch to issue an warning. Applied and pushed.
> 
> The incremental change follows. Please check that works for you or not.

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.

Thanks,

Ben.

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Christopher Li]

On Mon, May 9, 2011 at 1:02 PM, Ben Pfaff <blp@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?

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

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Ben Pfaff]

On Mon, May 09, 2011 at 01:31:10PM -0700, Christopher Li wrote:
> On Mon, May 9, 2011 at 1:02 PM, Ben Pfaff <blp@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?

I'm a developer on the Open vSwitch project (http://openvswitch.org).
Open vSwitch has both kernel and userspace code.  For a long time,
we've been using sparse to sanity-check our kernel code.  Now, I'm
adding support for sanity-checking of userspace code using the same
C=1 "make" option as the kernel.  (There's a patch series posted
starting at http://openvswitch.org/pipermail/dev/2011-May/008607.html
in case you're really curious.)  It's already found some bugs, mostly
due to the ability to mark network byte order types as special through
__attribute__((bitwise)).

This warning (formerly error) is the only sparse warning left in the
build, which is otherwise clean.

The context for the warning is a C file of code generated by database
interface definition language (IDL) bindings.  Each of these structs
corresponds to a row in a database table.  Here's the simplest struct
in question, for a database table with only two columns, named 'fault'
and 'mpid':

    /* Maintenance_Point table. */
    struct ovsrec_maintenance_point {
	   struct ovsdb_idl_row header_;

	   /* fault column. */
	   bool *fault;
	   size_t n_fault;

	   /* mpid column. */
	   int64_t mpid;
    };

The 'fault' member is 3-valued: a null pointer means that the row has
an empty "fault" column; otherwise it points either to a malloc()'d
true or false value.  The warning then crops up in the generated code
for populating this struct, which does something similar to the
following when the "fault" column is nonempty:

    row->fault = xmalloc(sizeof *row->fault);
    *row->fault = /* value parsed from database row */;

I could change my IDL code generator to do something different for
this case, but I don't see anything actually wrong with it.  This
userspace code is not performance-critical or sensitive to memory
usage, so it's not necessary on the face of it to optimize it.

Thanks,

Ben.

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Christopher Li]

On Mon, May 9, 2011 at 1:49 PM, Ben Pfaff <blp@nicira.com> wrote:
> The 'fault' member is 3-valued: a null pointer means that the row has
> an empty "fault" column; otherwise it points either to a malloc()'d
> true or false value.  The warning then crops up in the generated code
> for populating this struct, which does something similar to the
> following when the "fault" column is nonempty:
>
>    row->fault = xmalloc(sizeof *row->fault);
>    *row->fault = /* value parsed from database row */;

I don't see particular things wrong with it. Personally I think malloc is
a bit overkill here. It will likely pad the area to natural machine int size
any way.

> I could change my IDL code generator to do something different for
> this case, but I don't see anything actually wrong with it.  This
> userspace code is not performance-critical or sensitive to memory
> usage, so it's not necessary on the face of it to optimize it.

Removing the warning will lose the chance to see it on other potential
problematic usage. If you insist on no warning. You can submit a patch
to add a switch to disable it. We have a lot of those for fine grain warning
control. I just don't see this warning can hurt on the kernel checking side.

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

Re: [PATCH] evaluate: Allow sizeof(_Bool) to succeed.

[Ben Pfaff]

On Wed, May 11, 2011 at 05:09:42PM -0700, Christopher Li wrote:
> On Mon, May 9, 2011 at 1:49 PM, Ben Pfaff <blp@nicira.com> wrote:
> > I could change my IDL code generator to do something different for
> > this case, but I don't see anything actually wrong with it. ?This
> > userspace code is not performance-critical or sensitive to memory
> > usage, so it's not necessary on the face of it to optimize it.
> 
> Removing the warning will lose the chance to see it on other potential
> problematic usage. If you insist on no warning. You can submit a patch
> to add a switch to disable it. We have a lot of those for fine grain warning
> control. I just don't see this warning can hurt on the kernel checking side.

For now, I've decided to just rewrite code on the Open vSwitch side to
avoid sizeof(_Bool) entirely.  If it becomes a problem again, I'll
submit a -Wno-sizeof-bool patch.

Thanks for considering my reasons.

Ben