| Message ID | BANLkTikHwADf5Kv5=rQosKcNjsMYM-28wQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
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. -- 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
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
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. -- 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
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
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 -- 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
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 */