* [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
@ 2014-07-15 15:46 Jeff Layton
2014-07-15 19:51 ` Christopher Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2014-07-15 15:46 UTC (permalink / raw)
To: linux-sparse; +Cc: Pekka Enberg
This reverts commit 65be24b8ddf54164ff25febfd3d5f9c26ae3877d.
bits_in_bool was originally set to 1 bit, and then was reset to 8 to
work around some issues with LLVM. Once those were resolved it was
apparently set back to being 1 bit.
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
The bits_to_bytes conversion ends up wrong because it relies on
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.
Cc: Pekka Enberg <penberg@kernel.org>
Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
target.c | 2 +-
validation/initializer-entry-defined-twice.c | 10 ++++++++++
validation/sizeof-bool.c | 6 +-----
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/target.c b/target.c
index 17b228ae924c..6a535bc0b2d7 100644
--- a/target.c
+++ b/target.c
@@ -14,7 +14,7 @@ int max_alignment = 16;
/*
* Integer data types
*/
-int bits_in_bool = 1;
+int bits_in_bool = 8;
int bits_in_char = 8;
int bits_in_short = 16;
int bits_in_int = 32;
diff --git a/validation/initializer-entry-defined-twice.c b/validation/initializer-entry-defined-twice.c
index 968e3dd1af2a..8a5bd3a95631 100644
--- a/validation/initializer-entry-defined-twice.c
+++ b/validation/initializer-entry-defined-twice.c
@@ -41,6 +41,16 @@ static struct same_offset not_an_error = {
.field1 = { },
.field2 = 0
};
+
+/*
+ * _Bools generally take a whole byte, so ensure that we can initialize
+ * them without spewing a warning.
+ */
+static _Bool boolarray[3] = {
+ [0] = 1,
+ [1] = 1,
+};
+
/*
* check-name: Initializer entry defined twice
*
diff --git a/validation/sizeof-bool.c b/validation/sizeof-bool.c
index 6c68748a0b3f..71ae1bce3699 100644
--- a/validation/sizeof-bool.c
+++ b/validation/sizeof-bool.c
@@ -4,9 +4,5 @@ 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
+ * check-description: sizeof(_Bool) is valid
*/
--
1.9.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
2014-07-15 15:46 [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits"" Jeff Layton
@ 2014-07-15 19:51 ` Christopher Li
2014-07-16 2:23 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Christopher Li @ 2014-07-15 19:51 UTC (permalink / raw)
To: Jeff Layton; +Cc: Linux-Sparse, Pekka Enberg
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;
}
>
> 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
2014-07-15 19:51 ` Christopher Li
@ 2014-07-16 2:23 ` Jeff Layton
2014-07-16 3:21 ` Christopher Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2014-07-16 2:23 UTC (permalink / raw)
To: Christopher Li; +Cc: Linux-Sparse, Pekka Enberg
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.
--- 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.
> >
> > 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
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
2014-07-16 2:23 ` Jeff Layton
@ 2014-07-16 3:21 ` Christopher Li
2014-07-16 16:24 ` Jeff Layton
0 siblings, 1 reply; 6+ messages in thread
From: Christopher Li @ 2014-07-16 3:21 UTC (permalink / raw)
To: Jeff Layton; +Cc: Linux-Sparse, Pekka Enberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
2014-07-16 3:21 ` Christopher Li
@ 2014-07-16 16:24 ` Jeff Layton
2014-07-16 21:39 ` Christopher Li
0 siblings, 1 reply; 6+ messages in thread
From: Jeff Layton @ 2014-07-16 16:24 UTC (permalink / raw)
To: Christopher Li; +Cc: Jeff Layton, Linux-Sparse, Pekka Enberg
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!
--
Jeff Layton <jlayton@primarydata.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits""
2014-07-16 16:24 ` Jeff Layton
@ 2014-07-16 21:39 ` Christopher Li
0 siblings, 0 replies; 6+ messages in thread
From: Christopher Li @ 2014-07-16 21:39 UTC (permalink / raw)
To: Jeff Layton; +Cc: Linux-Sparse, Pekka Enberg
On Wed, Jul 16, 2014 at 9:24 AM, Jeff Layton
<jeff.layton@primarydata.com> wrote:
>
> 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.
>
Great catch. I was looking at the it last night but haven't found
the cause. It was not cause by the array element alignment,
which I attach a purposed patch.
I will apply your V2 patch and the fix for the array. Every thing
should be happy now.
Chris
round up for array element size to byte align
When layout the array element, the element size should
round up to byte align.
Signed-off-by: Christopher Li <sparse@chrisli.org>
diff --git a/evaluate.c b/evaluate.c
index 7ce8c55..03992d0 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2302,7 +2302,7 @@ static struct expression
*check_designators(struct expression *e,
}
type = ctype->ctype.base_type;
if (ctype->bit_size >= 0 && type->bit_size >= 0) {
- unsigned offset = e->idx_to * type->bit_size;
+ unsigned offset =
array_element_offset(type->bit_size, e->idx_to);
if (offset >= ctype->bit_size) {
err = "index out of bounds in";
break;
diff --git a/symbol.c b/symbol.c
index 1a3df63..a697627 100644
--- a/symbol.c
+++ b/symbol.c
@@ -234,7 +234,8 @@ static struct symbol * examine_array_type(struct
symbol *sym)
return sym;
if (array_size) {
- bit_size = base_type->bit_size *
get_expression_value_silent(array_size);
+ bit_size = array_element_offset(base_type->bit_size,
+
get_expression_value_silent(array_size));
if (array_size->type != EXPR_VALUE) {
if (Wvla)
warning(array_size->pos, "Variable
length array is used.");
diff --git a/target.h b/target.h
index 140df3c..78c85cf 100644
--- a/target.h
+++ b/target.h
@@ -57,4 +57,12 @@ static inline int bytes_to_bits(int bytes)
return bytes * bits_in_char;
}
+static inline unsigned long array_element_offset(unsigned long
base_bits, int idx)
+{
+ int fragment = base_bits % bits_in_char;
+ if (fragment)
+ base_bits += bits_in_char - fragment;
+ return base_bits * idx;
+}
+
#endif
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-07-16 21:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-15 15:46 [sparse PATCH RFC] Revert "Revert "sparse: Bump up sizeof(_Bool) to 8 bits"" Jeff Layton
2014-07-15 19:51 ` Christopher Li
2014-07-16 2:23 ` Jeff Layton
2014-07-16 3:21 ` Christopher Li
2014-07-16 16:24 ` Jeff Layton
2014-07-16 21:39 ` Christopher Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox