diff mbox

[sparse,RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""

Message ID 20140715222350.028a7bf6@f20.localdomain (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Jeff Layton July 16, 2014, 2:23 a.m. UTC 
On Tue, 15 Jul 2014 12:51:02 -0700
Christopher Li <sparse@chrisli.org> wrote:

> On Tue, Jul 15, 2014 at 8:46 AM, Jeff Layton <jlayton@primarydata.com> wrote:
> >
> >
> > The problem here is that, at least with GCC on x86_64, a _Bool always
> > seems to be end up being 8 bits (which makes sense -- how would you get
> > a pointer to it otherwise?). If you declare and initialize an array like
> > this:
> >
> >     static _Bool boolarray[3] = {
> >             [0] = 1,
> >             [1] = 1,
> >     };
> >
> > ...you get warnings like this (which are bogus):
> >
> >     ./test.c:2:10: warning: Initializer entry defined twice
> >     ./test.c:3:10:   also defined here
> 
> 
> Right.  The bug is in bits_to_bytes(). It round down instead of
> round up. "bits_in_bool" is only a default value. It can be
> reset to other value by the sparse application (sparse as
> library). So the rest of the code still need to coupe with
> it.
> 
>  How about fix bits_to_bytes() to round up:
> 
> diff --git a/target.h b/target.h
> index 1030c7c..140df3c 100644
> --- a/target.h
> +++ b/target.h
> @@ -49,7 +49,7 @@ extern int enum_alignment;
> 
>  static inline int bits_to_bytes(int bits)
>  {
> -       return bits >= 0 ? bits / bits_in_char : -1;
> +       return bits >= 0 ? (bits + bits_in_char - 1) / bits_in_char : -1;
>  }
> 

This makes the bitfield.c validation test fail:

     TEST     bitfield to integer promotion (bitfields.c)
error: actual error text does not match expected error text.
error: see bitfields.c.error.* for further investigation.

Though I haven't looked in detail yet as to why it's failing.

> >
> > integer division, and that causes the above bogus warning.
> >
> > Reset it back to 8 bits, and add a validation test that creates the
> > above array to ensure that we don't end up breaking it again.
> 
> The rest of the change looks good. Thanks for the test case.
> 
> My patch is not tested. If you are OK with it, submit a V2
> or I can fix the change for you.
> 
> Thanks
> 
> Chris

Comments

Christopher Li July 16, 2014, 3:21 a.m. UTC | #1 
On Tue, Jul 15, 2014 at 7:23 PM, Jeff Layton
<jeff.layton@primarydata.com> wrote:
> On Tue, 15 Jul 2014 12:51:02 -0700
>> --- a/target.h
>> +++ b/target.h
>> @@ -49,7 +49,7 @@ extern int enum_alignment;
>>
>>  static inline int bits_to_bytes(int bits)
>>  {
>> -       return bits >= 0 ? bits / bits_in_char : -1;
>> +       return bits >= 0 ? (bits + bits_in_char - 1) / bits_in_char : -1;
>>  }
>>
>
> This makes the bitfield.c validation test fail:
>
>      TEST     bitfield to integer promotion (bitfields.c)
> error: actual error text does not match expected error text.
> error: see bitfields.c.error.* for further investigation.
> --- bitfields.c.error.expected  2014-07-15 22:01:09.236942195 -0400
> +++ bitfields.c.error.got       2014-07-15 22:01:09.237942176 -0400
> @@ -0,0 +1,2 @@
> +bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)
> +bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)
>
> Though I haven't looked in detail yet as to why it's failing.

Let me take a look and get back to you. I recall there are one more
place in array handling need to use  bits_to_bytes() instead of reference
the bit size directly.

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
Jeff Layton July 16, 2014, 4:24 p.m. UTC | #2 
On Tue, 15 Jul 2014 20:21:40 -0700
Christopher Li <sparse@chrisli.org> wrote:

> On Tue, Jul 15, 2014 at 7:23 PM, Jeff Layton
> <jeff.layton@primarydata.com> wrote:
> > On Tue, 15 Jul 2014 12:51:02 -0700
> >> --- a/target.h
> >> +++ b/target.h
> >> @@ -49,7 +49,7 @@ extern int enum_alignment;
> >>
> >>  static inline int bits_to_bytes(int bits)
> >>  {
> >> -       return bits >= 0 ? bits / bits_in_char : -1;
> >> +       return bits >= 0 ? (bits + bits_in_char - 1) / bits_in_char : -1;
> >>  }
> >>
> >
> > This makes the bitfield.c validation test fail:
> >
> >      TEST     bitfield to integer promotion (bitfields.c)
> > error: actual error text does not match expected error text.
> > error: see bitfields.c.error.* for further investigation.
> > --- bitfields.c.error.expected  2014-07-15 22:01:09.236942195 -0400
> > +++ bitfields.c.error.got       2014-07-15 22:01:09.237942176 -0400
> > @@ -0,0 +1,2 @@
> > +bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)
> > +bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)
> >
> > Though I haven't looked in detail yet as to why it's failing.
> 
> Let me take a look and get back to you. I recall there are one more
> place in array handling need to use  bits_to_bytes() instead of reference
> the bit size directly.
> 
> Chris

Thanks, I think I might have found it. init_ctype tries to correct for
the fact that bits_to_bytes rounds up instead of down. With the
corrected function, we don't need that correction anymore.

I'll send the v2 patch soon, which seems to do the right thing.

Thanks!
diff mbox

Patch

--- bitfields.c.error.expected	2014-07-15 22:01:09.236942195 -0400
+++ bitfields.c.error.got	2014-07-15 22:01:09.237942176 -0400
@@ -0,0 +1,2 @@ 
+bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)
+bitfields.c:16:19: warning: invalid access past the end of 'y' (0 1)