Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On 2018-04-19 06:42 PM, yuank...@codeaurora.org wrote:
On 2018-04-19 02:48 PM, yuank...@codeaurora.org wrote:
On 2018-04-19 01:16 PM, Julia Lawall wrote:
On Wed, 18 Apr 2018, Joe Perches wrote:
On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > >
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > >
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > >
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > >
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances. A regex can not.
> > > > > >
> > > > > > I got 12667.
> > > > >
> > > > > Could you please post the cocci script?
> > > > >
> > > > > > I'm not sure to understand the issue. Will using a bitfield help
if there
> > > > > > are no other bitfields in the structure?
> > > > >
> > > > > IMO, not really.
> > > > >
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > >
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > >
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > >
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > >
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > >
> > > [ZJ] Hopefully, this could make it better in your environment.
> > > IMHO, this is just for double check.
> >
> > I doubt this is actually better or smaller code.
> >
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
>
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.
I _really_ doubt there is any difference in size between the
below in any architecture
struct foo {
int bar;
bool baz:1;
int qux;
};
and
struct foo {
int bar;
bool baz;
int qux;
};
Where there would be a difference in size is
struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};
and
struct foo {
int bar;
bool baz1;
bool baz2;
int qux;
};
[ZJ] Even though, two bool:1 are grouped in the #3, finally 4 bytes are
padded
due for int is the most significant in the type size.
At least, they are all the same per x86 and arm based on gcc.(12
bytes).
[ZJ] However, #3 could be difference to #4 if compiling it if the size
of (_Bool)
is a bigger value(4 bytes maybe available in Alpha EV45 for ex.).
In the situation of the example there are two bools together in the
middle
of the structure and one at the end. Somehow, even converting to
bool:1
increases the size. But it seems plausible that putting all three
bools
together and converting them all to :1 would reduce the size. I
don't
know. The size increase (more than 8 bytes) seems out of proportion
for 3
bools.
[ZJ] Typically, addition saving is due for difference padding.
I was able to check around 3000 structures that were not declared
with any
attributes, that don't declare named types internally, and that are
compiled for x86. Around 10% become smaller whn using bool:1,
typically
by at most 8 bytes.
[ZJ] As my example, int (*)() requested 8 bytes in x86 arch, then 8
bytes is similiar to that.
While it request 4 bytes in arm arch. Typically, my previous
example struct can
reach to 32 bytes in x86 arch(compared to 40 bytes for original
version).
Similarly, 20 bytes in arm arch(compared to 24 bytes per original
version).
julia
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On 2018-04-19 02:48 PM, yuank...@codeaurora.org wrote:
On 2018-04-19 01:16 PM, Julia Lawall wrote:
On Wed, 18 Apr 2018, Joe Perches wrote:
On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > >
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > >
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > >
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > >
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances. A regex can not.
> > > > > >
> > > > > > I got 12667.
> > > > >
> > > > > Could you please post the cocci script?
> > > > >
> > > > > > I'm not sure to understand the issue. Will using a bitfield help
if there
> > > > > > are no other bitfields in the structure?
> > > > >
> > > > > IMO, not really.
> > > > >
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > >
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > >
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > >
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > >
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > >
> > > [ZJ] Hopefully, this could make it better in your environment.
> > > IMHO, this is just for double check.
> >
> > I doubt this is actually better or smaller code.
> >
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
>
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.
I _really_ doubt there is any difference in size between the
below in any architecture
struct foo {
int bar;
bool baz:1;
int qux;
};
and
struct foo {
int bar;
bool baz;
int qux;
};
Where there would be a difference in size is
struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};
and
struct foo {
int bar;
bool baz1;
bool baz2;
int qux;
};
[ZJ] Even though, two bool:1 are grouped in the #3, finally 4 bytes are
padded
due for int is the most significant in the type size.
At least, they are all the same per x86 and arm based on gcc.(12
bytes).
In the situation of the example there are two bools together in the
middle
of the structure and one at the end. Somehow, even converting to
bool:1
increases the size. But it seems plausible that putting all three
bools
together and converting them all to :1 would reduce the size. I don't
know. The size increase (more than 8 bytes) seems out of proportion
for 3
bools.
[ZJ] Typically, addition saving is due for difference padding.
I was able to check around 3000 structures that were not declared with
any
attributes, that don't declare named types internally, and that are
compiled for x86. Around 10% become smaller whn using bool:1,
typically
by at most 8 bytes.
[ZJ] As my example, int (*)() requested 8 bytes in x86 arch, then 8
bytes is similiar to that.
While it request 4 bytes in arm arch. Typically, my previous
example struct can
reach to 32 bytes in x86 arch(compared to 40 bytes for original
version).
Similarly, 20 bytes in arm arch(compared to 24 bytes per original
version).
julia
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On 2018-04-19 01:16 PM, Julia Lawall wrote:
On Wed, 18 Apr 2018, Joe Perches wrote:
On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > >
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > >
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > >
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > >
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances. A regex can not.
> > > > > >
> > > > > > I got 12667.
> > > > >
> > > > > Could you please post the cocci script?
> > > > >
> > > > > > I'm not sure to understand the issue. Will using a bitfield help
if there
> > > > > > are no other bitfields in the structure?
> > > > >
> > > > > IMO, not really.
> > > > >
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > >
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > >
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > >
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > >
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > >
> > > [ZJ] Hopefully, this could make it better in your environment.
> > > IMHO, this is just for double check.
> >
> > I doubt this is actually better or smaller code.
> >
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
>
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.
I _really_ doubt there is any difference in size between the
below in any architecture
struct foo {
int bar;
bool baz:1;
int qux;
};
and
struct foo {
int bar;
bool baz;
int qux;
};
Where there would be a difference in size is
struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};
and
struct foo {
int bar;
bool baz1;
bool baz2;
int qux;
};
In the situation of the example there are two bools together in the
middle
of the structure and one at the end. Somehow, even converting to
bool:1
increases the size. But it seems plausible that putting all three
bools
together and converting them all to :1 would reduce the size. I don't
know. The size increase (more than 8 bytes) seems out of proportion
for 3
bools.
[ZJ] Typically, addition saving is due for difference padding.
I was able to check around 3000 structures that were not declared with
any
attributes, that don't declare named types internally, and that are
compiled for x86. Around 10% become smaller whn using bool:1,
typically
by at most 8 bytes.
julia
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 18 Apr 2018, Joe Perches wrote:
> On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
> >
> > On Wed, 18 Apr 2018, Joe Perches wrote:
> >
> > > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > > Hi julia,
> > > >
> > > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > >
> > > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > > We already have some 500 bools-in-structs
> > > > > > > >
> > > > > > > > I got at least triple that only in include/
> > > > > > > > so I expect there are at probably an order
> > > > > > > > of magnitude more than 500 in the kernel.
> > > > > > > >
> > > > > > > > I suppose some cocci script could count the
> > > > > > > > actual number of instances. A regex can not.
> > > > > > >
> > > > > > > I got 12667.
> > > > > >
> > > > > > Could you please post the cocci script?
> > > > > >
> > > > > > > I'm not sure to understand the issue. Will using a bitfield help
> > > > > > > if there
> > > > > > > are no other bitfields in the structure?
> > > > > >
> > > > > > IMO, not really.
> > > > > >
> > > > > > The primary issue is described by Linus here:
> > > > > > https://lkml.org/lkml/2017/11/21/384
> > > > > >
> > > > > > I personally do not find a significant issue with
> > > > > > uncontrolled sizes of bool in kernel structs as
> > > > > > all of the kernel structs are transitory and not
> > > > > > written out to storage.
> > > > > >
> > > > > > I suppose bool bitfields are also OK, but for the
> > > > > > RMW required.
> > > > > >
> > > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > > has the negative of truncation so that the uint
> > > > > > has to be set with !! instead of a simple assign.
> > > > >
> > > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > > structure
> > > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > > with
> > > > > both approaches.
> > > >
> > > > [ZJ] Hopefully, this could make it better in your environment.
> > > > IMHO, this is just for double check.
> > >
> > > I doubt this is actually better or smaller code.
> > >
> > > Check the actual object code using objdump and the
> > > struct alignment using pahole.
> >
> > I didn't have a chance to try it, but it looks quite likely to result in a
> > smaller data structure based on the other examples that I looked at.
>
> I _really_ doubt there is any difference in size between the
> below in any architecture
>
> struct foo {
> int bar;
> bool baz:1;
> int qux;
> };
>
> and
>
> struct foo {
> int bar;
> bool baz;
> int qux;
> };
>
> Where there would be a difference in size is
>
> struct foo {
> int bar;
> bool baz1:1;
> bool baz2:1;
> int qux;
> };
>
> and
>
> struct foo {
> int bar;
> bool baz1;
> bool baz2;
>
> int qux;
> };
In the situation of the example there are two bools together in the middle
of the structure and one at the end. Somehow, even converting to bool:1
increases the size. But it seems plausible that putting all three bools
together and converting them all to :1 would reduce the size. I don't
know. The size increase (more than 8 bytes) seems out of proportion for 3
bools.
I was able to check around 3000 structures that were not declared with any
attributes, that don't declare named types internally, and that are
compiled for x86. Around 10% become smaller whn using bool:1, typically
by at most 8 bytes.
julia
>
>
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, 2018-04-19 at 06:40 +0200, Julia Lawall wrote:
>
> On Wed, 18 Apr 2018, Joe Perches wrote:
>
> > On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > > Hi julia,
> > >
> > > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > >
> > > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > > We already have some 500 bools-in-structs
> > > > > > >
> > > > > > > I got at least triple that only in include/
> > > > > > > so I expect there are at probably an order
> > > > > > > of magnitude more than 500 in the kernel.
> > > > > > >
> > > > > > > I suppose some cocci script could count the
> > > > > > > actual number of instances. A regex can not.
> > > > > >
> > > > > > I got 12667.
> > > > >
> > > > > Could you please post the cocci script?
> > > > >
> > > > > > I'm not sure to understand the issue. Will using a bitfield help
> > > > > > if there
> > > > > > are no other bitfields in the structure?
> > > > >
> > > > > IMO, not really.
> > > > >
> > > > > The primary issue is described by Linus here:
> > > > > https://lkml.org/lkml/2017/11/21/384
> > > > >
> > > > > I personally do not find a significant issue with
> > > > > uncontrolled sizes of bool in kernel structs as
> > > > > all of the kernel structs are transitory and not
> > > > > written out to storage.
> > > > >
> > > > > I suppose bool bitfields are also OK, but for the
> > > > > RMW required.
> > > > >
> > > > > Using unsigned int :1 bitfield instead of bool :1
> > > > > has the negative of truncation so that the uint
> > > > > has to be set with !! instead of a simple assign.
> > > >
> > > > At least with gcc 5.4.0, a number of structures become larger with
> > > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > > structure
> > > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > > with
> > > > both approaches.
> > >
> > > [ZJ] Hopefully, this could make it better in your environment.
> > > IMHO, this is just for double check.
> >
> > I doubt this is actually better or smaller code.
> >
> > Check the actual object code using objdump and the
> > struct alignment using pahole.
>
> I didn't have a chance to try it, but it looks quite likely to result in a
> smaller data structure based on the other examples that I looked at.
I _really_ doubt there is any difference in size between the
below in any architecture
struct foo {
int bar;
bool baz:1;
int qux;
};
and
struct foo {
int bar;
bool baz;
int qux;
};
Where there would be a difference in size is
struct foo {
int bar;
bool baz1:1;
bool baz2:1;
int qux;
};
and
struct foo {
int bar;
bool baz1;
bool baz2;
int qux;
};
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 18 Apr 2018, Joe Perches wrote:
> On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> > Hi julia,
> >
> > On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > >
> > > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > > We already have some 500 bools-in-structs
> > > > > >
> > > > > > I got at least triple that only in include/
> > > > > > so I expect there are at probably an order
> > > > > > of magnitude more than 500 in the kernel.
> > > > > >
> > > > > > I suppose some cocci script could count the
> > > > > > actual number of instances. A regex can not.
> > > > >
> > > > > I got 12667.
> > > >
> > > > Could you please post the cocci script?
> > > >
> > > > > I'm not sure to understand the issue. Will using a bitfield help if
> > > > > there
> > > > > are no other bitfields in the structure?
> > > >
> > > > IMO, not really.
> > > >
> > > > The primary issue is described by Linus here:
> > > > https://lkml.org/lkml/2017/11/21/384
> > > >
> > > > I personally do not find a significant issue with
> > > > uncontrolled sizes of bool in kernel structs as
> > > > all of the kernel structs are transitory and not
> > > > written out to storage.
> > > >
> > > > I suppose bool bitfields are also OK, but for the
> > > > RMW required.
> > > >
> > > > Using unsigned int :1 bitfield instead of bool :1
> > > > has the negative of truncation so that the uint
> > > > has to be set with !! instead of a simple assign.
> > >
> > > At least with gcc 5.4.0, a number of structures become larger with
> > > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > > structure
> > > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > > with
> > > both approaches.
> >
> > [ZJ] Hopefully, this could make it better in your environment.
> > IMHO, this is just for double check.
>
> I doubt this is actually better or smaller code.
>
> Check the actual object code using objdump and the
> struct alignment using pahole.
I didn't have a chance to try it, but it looks quite likely to result in a
smaller data structure based on the other examples that I looked at.
julia
>
> > diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> > index 4f6d643..b46e170 100644
> > --- a/drivers/gpio/gpio-ich.c
> > +++ b/drivers/gpio/gpio-ich.c
> > @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> > #define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
> >
> > struct ichx_desc {
> > + /* GPO_BLINK is available on this chipset */
> > + bool uses_gpe0:1;
> > +
> > + /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > +bool uses_gpe0:1;
> > +
> > +/*
> > + * Some chipsets don't let reading output values on GPIO_LVL
> > register
> > + * this option allows driver caching written output values
> > + */
> > +bool use_outlvl_cache:1;
> > +
> > /* Max GPIO pins the chipset can have */
> > uint ngpio;
> >
> > @@ -77,24 +89,12 @@ struct ichx_desc {
> > const u8 (*regs)[3];
> > const u8 *reglen;
> >
> > - /* GPO_BLINK is available on this chipset */
> > - bool have_blink;
> > -
> > - /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> > */
> > - bool uses_gpe0;
> > -
> > /* USE_SEL is bogus on some chipsets, eg 3100 */
> > u32 use_sel_ignore[3];
> >
> > /* Some chipsets have quirks, let these use their own
> > request/get */
> > int (*request)(struct gpio_chip *chip, unsigned offset);
> > int (*get)(struct gpio_chip *chip, unsigned offset);
> > -
> > - /*
> > -* Some chipsets don't let reading output values on GPIO_LVL
> > register
> > -* this option allows driver caching written output values
> > -*/
> > - bool use_outlvl_cache;
> > };
> >
> >
> > ZJ
>
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Tue, 2018-04-17 at 17:07 +0800, yuank...@codeaurora.org wrote:
> Hi julia,
>
> On 2018-04-15 05:19 AM, Julia Lawall wrote:
> > On Wed, 11 Apr 2018, Joe Perches wrote:
> >
> > > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > > > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > > > We already have some 500 bools-in-structs
> > > > >
> > > > > I got at least triple that only in include/
> > > > > so I expect there are at probably an order
> > > > > of magnitude more than 500 in the kernel.
> > > > >
> > > > > I suppose some cocci script could count the
> > > > > actual number of instances. A regex can not.
> > > >
> > > > I got 12667.
> > >
> > > Could you please post the cocci script?
> > >
> > > > I'm not sure to understand the issue. Will using a bitfield help if
> > > > there
> > > > are no other bitfields in the structure?
> > >
> > > IMO, not really.
> > >
> > > The primary issue is described by Linus here:
> > > https://lkml.org/lkml/2017/11/21/384
> > >
> > > I personally do not find a significant issue with
> > > uncontrolled sizes of bool in kernel structs as
> > > all of the kernel structs are transitory and not
> > > written out to storage.
> > >
> > > I suppose bool bitfields are also OK, but for the
> > > RMW required.
> > >
> > > Using unsigned int :1 bitfield instead of bool :1
> > > has the negative of truncation so that the uint
> > > has to be set with !! instead of a simple assign.
> >
> > At least with gcc 5.4.0, a number of structures become larger with
> > unsigned int :1. bool:1 seems to mostly solve this problem. The
> > structure
> > ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
> > with
> > both approaches.
>
> [ZJ] Hopefully, this could make it better in your environment.
> IMHO, this is just for double check.
I doubt this is actually better or smaller code.
Check the actual object code using objdump and the
struct alignment using pahole.
> diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
> index 4f6d643..b46e170 100644
> --- a/drivers/gpio/gpio-ich.c
> +++ b/drivers/gpio/gpio-ich.c
> @@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
> #define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
>
> struct ichx_desc {
> + /* GPO_BLINK is available on this chipset */
> + bool uses_gpe0:1;
> +
> + /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> */
> +bool uses_gpe0:1;
> +
> +/*
> + * Some chipsets don't let reading output values on GPIO_LVL
> register
> + * this option allows driver caching written output values
> + */
> +bool use_outlvl_cache:1;
> +
> /* Max GPIO pins the chipset can have */
> uint ngpio;
>
> @@ -77,24 +89,12 @@ struct ichx_desc {
> const u8 (*regs)[3];
> const u8 *reglen;
>
> - /* GPO_BLINK is available on this chipset */
> - bool have_blink;
> -
> - /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
> */
> - bool uses_gpe0;
> -
> /* USE_SEL is bogus on some chipsets, eg 3100 */
> u32 use_sel_ignore[3];
>
> /* Some chipsets have quirks, let these use their own
> request/get */
> int (*request)(struct gpio_chip *chip, unsigned offset);
> int (*get)(struct gpio_chip *chip, unsigned offset);
> -
> - /*
> -* Some chipsets don't let reading output values on GPIO_LVL
> register
> -* this option allows driver caching written output values
> -*/
> - bool use_outlvl_cache;
> };
>
>
> ZJ
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
Hi julia,
On 2018-04-15 05:19 AM, Julia Lawall wrote:
On Wed, 11 Apr 2018, Joe Perches wrote:
On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> On Wed, 11 Apr 2018, Joe Perches wrote:
> > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > We already have some 500 bools-in-structs
> >
> > I got at least triple that only in include/
> > so I expect there are at probably an order
> > of magnitude more than 500 in the kernel.
> >
> > I suppose some cocci script could count the
> > actual number of instances. A regex can not.
>
> I got 12667.
Could you please post the cocci script?
> I'm not sure to understand the issue. Will using a bitfield help if there
> are no other bitfields in the structure?
IMO, not really.
The primary issue is described by Linus here:
https://lkml.org/lkml/2017/11/21/384
I personally do not find a significant issue with
uncontrolled sizes of bool in kernel structs as
all of the kernel structs are transitory and not
written out to storage.
I suppose bool bitfields are also OK, but for the
RMW required.
Using unsigned int :1 bitfield instead of bool :1
has the negative of truncation so that the uint
has to be set with !! instead of a simple assign.
At least with gcc 5.4.0, a number of structures become larger with
unsigned int :1. bool:1 seems to mostly solve this problem. The
structure
ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger
with
both approaches.
[ZJ] Hopefully, this could make it better in your environment.
IMHO, this is just for double check.
diff --git a/drivers/gpio/gpio-ich.c b/drivers/gpio/gpio-ich.c
index 4f6d643..b46e170 100644
--- a/drivers/gpio/gpio-ich.c
+++ b/drivers/gpio/gpio-ich.c
@@ -70,6 +70,18 @@ static const u8 avoton_reglen[3] = {
#define ICHX_READ(reg, base_res) inl((reg) + (base_res)->start)
struct ichx_desc {
+ /* GPO_BLINK is available on this chipset */
+ bool uses_gpe0:1;
+
+ /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
*/
+bool uses_gpe0:1;
+
+/*
+ * Some chipsets don't let reading output values on GPIO_LVL
register
+ * this option allows driver caching written output values
+ */
+bool use_outlvl_cache:1;
+
/* Max GPIO pins the chipset can have */
uint ngpio;
@@ -77,24 +89,12 @@ struct ichx_desc {
const u8 (*regs)[3];
const u8 *reglen;
- /* GPO_BLINK is available on this chipset */
- bool have_blink;
-
- /* Whether the chipset has GPIO in GPE0_STS in the PM IO region
*/
- bool uses_gpe0;
-
/* USE_SEL is bogus on some chipsets, eg 3100 */
u32 use_sel_ignore[3];
/* Some chipsets have quirks, let these use their own
request/get */
int (*request)(struct gpio_chip *chip, unsigned offset);
int (*get)(struct gpio_chip *chip, unsigned offset);
-
- /*
-* Some chipsets don't let reading output values on GPIO_LVL
register
-* this option allows driver caching written output values
-*/
- bool use_outlvl_cache;
};
ZJ
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 11 Apr 2018, Joe Perches wrote: > On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote: > > On Wed, 11 Apr 2018, Joe Perches wrote: > > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > > > We already have some 500 bools-in-structs > > > > > > I got at least triple that only in include/ > > > so I expect there are at probably an order > > > of magnitude more than 500 in the kernel. > > > > > > I suppose some cocci script could count the > > > actual number of instances. A regex can not. > > > > I got 12667. > > Could you please post the cocci script? > > > I'm not sure to understand the issue. Will using a bitfield help if there > > are no other bitfields in the structure? > > IMO, not really. > > The primary issue is described by Linus here: > https://lkml.org/lkml/2017/11/21/384 > > I personally do not find a significant issue with > uncontrolled sizes of bool in kernel structs as > all of the kernel structs are transitory and not > written out to storage. > > I suppose bool bitfields are also OK, but for the > RMW required. > > Using unsigned int :1 bitfield instead of bool :1 > has the negative of truncation so that the uint > has to be set with !! instead of a simple assign. At least with gcc 5.4.0, a number of structures become larger with unsigned int :1. bool:1 seems to mostly solve this problem. The structure ichx_desc, defined in drivers/gpio/gpio-ich.c seems to become larger with both approaches. julia
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, 12 Apr 2018 14:08:32 +0200 Peter Zijlstra wrote: > > It'd be _much_ nicer if vger.kernel.org stored every email > > it sent and had a search mechanism available rather than > > relying on external systems. > > People are looking at that afaik. I have linux-kernel mboxes going back to October 2000, which I can send to whoever-that-is if needed for importation purposes...
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, 2018-04-12 at 14:08 +0200, Peter Zijlstra wrote: > On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote: > > On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > > > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > > > Is there a better or recommended way to reference posts on LKML in > > > > commit > > > > messages? (I do like the idea of linking to previous discussions, > > > > results, > > > > ...) > > > > > > Yes: > > > > > > https://lkml.kernel.org/r/$MSGID > > > > > > that has the added benefit that it immediately includes the msg-id, so > > > even if you don't have tubes, you can search for it in your local > > > mailboxes. > > > > > > Also, since we (kernel.org) control the redirection (currently > > > marc.info) we can always point it to a life archive. > > > > Message IDs are not useful unless you subscribe and > > keep your emails. > > I happen to do so.. As do I for mailing list threads I reply to, but keeping all email threads locally is not not practicable on limited storage systems. > > It'd be _much_ nicer if vger.kernel.org stored every email > > it sent and had a search mechanism available rather than > > relying on external systems. > > People are looking at that afaik. Looking and doing are unfortunately different. Back when gmane died a couple years ago, I made the same suggestion to webmas...@kernel.org, postmas...@kernel.org and cc'd lkml https://lkml.org/lkml/2016/8/3/484 Never heard back. Maybe it's the proper time now to revisit this.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, Apr 12, 2018 at 05:01:37AM -0700, Joe Perches wrote: > On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > > Is there a better or recommended way to reference posts on LKML in commit > > > messages? (I do like the idea of linking to previous discussions, results, > > > ...) > > > > Yes: > > > > https://lkml.kernel.org/r/$MSGID > > > > that has the added benefit that it immediately includes the msg-id, so > > even if you don't have tubes, you can search for it in your local > > mailboxes. > > > > Also, since we (kernel.org) control the redirection (currently > > marc.info) we can always point it to a life archive. > > Message IDs are not useful unless you subscribe and > keep your emails. I happen to do so.. > It'd be _much_ nicer if vger.kernel.org stored every email > it sent and had a search mechanism available rather than > relying on external systems. People are looking at that afaik.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, 2018-04-12 at 13:50 +0200, Peter Zijlstra wrote: > On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > > Is there a better or recommended way to reference posts on LKML in commit > > messages? (I do like the idea of linking to previous discussions, results, > > ...) > > Yes: > > https://lkml.kernel.org/r/$MSGID > > that has the added benefit that it immediately includes the msg-id, so > even if you don't have tubes, you can search for it in your local > mailboxes. > > Also, since we (kernel.org) control the redirection (currently > marc.info) we can always point it to a life archive. Message IDs are not useful unless you subscribe and keep your emails. It'd be _much_ nicer if vger.kernel.org stored every email it sent and had a search mechanism available rather than relying on external systems.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
Andrea Parri writes: > On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote: >> >> * Peter Zijlstra wrote: >> >> > I still have room in my /dev/null mailbox for pure checkpatch patches. >> > >> > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) >> > >> > Yes, we really should not use lkml.org for references. Sadly google >> > displays it very prominently when you search for something. >> >> lkml.org is nice in emails that have a short expected life time and >> relevance - >> but it probably shouldn't be used for permanent references such as kernel >> messages, code comments and Git log entries. > > Is there a better or recommended way to reference posts on LKML in commit > messages? (I do like the idea of linking to previous discussions, results, > ...) I like lkml.kernel.org, for example a link to your message would be: https://lkml.kernel.org/r/20180412093521.GA6427@andrea BTW, I think it would be a good idea to add a hostname to your Message-Id. -- Kalle Valo
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, Apr 12, 2018 at 11:35:21AM +0200, Andrea Parri wrote: > Is there a better or recommended way to reference posts on LKML in commit > messages? (I do like the idea of linking to previous discussions, results, > ...) Yes: https://lkml.kernel.org/r/$MSGID that has the added benefit that it immediately includes the msg-id, so even if you don't have tubes, you can search for it in your local mailboxes. Also, since we (kernel.org) control the redirection (currently marc.info) we can always point it to a life archive.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
Hi, On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote: > > * Peter Zijlstra wrote: > > > I still have room in my /dev/null mailbox for pure checkpatch patches. > > > > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > > > Yes, we really should not use lkml.org for references. Sadly google > > displays it very prominently when you search for something. > > lkml.org is nice in emails that have a short expected life time and relevance > - > but it probably shouldn't be used for permanent references such as kernel > messages, code comments and Git log entries. Is there a better or recommended way to reference posts on LKML in commit messages? (I do like the idea of linking to previous discussions, results, ...) Andrea > > Thanks, > > Ingo
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, Apr 11, 2018 at 11:42:20PM -0700, Joe Perches wrote: > I personally do not find a significant issue with > uncontrolled sizes of bool in kernel structs as > all of the kernel structs are transitory and not > written out to storage. People that care about cache locality, false sharing and other such things really care about structure layout. Growing a structure into another cacheline can be a significant performance hit -- cache misses hurt.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, Apr 12, 2018 at 09:47:19AM +0200, Ingo Molnar wrote: > lkml.org is nice in emails that have a short expected life time and relevance > - I like lkml.org's archive (although it's not without its problems), but the site suffers from serious availability issues -- it is down a lot, which is _really_ tedious.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
* Peter Zijlstra wrote: > I still have room in my /dev/null mailbox for pure checkpatch patches. > > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > Yes, we really should not use lkml.org for references. Sadly google > displays it very prominently when you search for something. lkml.org is nice in emails that have a short expected life time and relevance - but it probably shouldn't be used for permanent references such as kernel messages, code comments and Git log entries. Thanks, Ingo
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 11 Apr 2018, Joe Perches wrote:
> On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote:
> > On Wed, 11 Apr 2018, Joe Perches wrote:
> > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote:
> > > > We already have some 500 bools-in-structs
> > >
> > > I got at least triple that only in include/
> > > so I expect there are at probably an order
> > > of magnitude more than 500 in the kernel.
> > >
> > > I suppose some cocci script could count the
> > > actual number of instances. A regex can not.
> >
> > I got 12667.
>
> Could you please post the cocci script?
Sure.
julia
Command line:
spatch.opt boolinstruct.cocci -j 40 --very-quiet --no-includes
--include-headers /run/shm/linux-next --use-idutils
This was tested on:
struct foo {
bool a;
bool b,c;
int r;
};
struct {
bool a;
bool b,c;
int r;
} x;
--
@initialize:ocaml@
@@
let ctr = ref 0
@r@
identifier i,x;
position p;
@@
struct i {
...
bool x@p;
...
}
@script:ocaml@
_p << r.p;
@@
ctr := !ctr + 1
@s@
identifier x;
position p;
@@
struct {
...
bool x@p;
...
}
@script:ocaml@
_p << s.p;
@@
ctr := !ctr + 1
@finalize:ocaml@
ctrs << merge.ctr;
@@
ctr := 0;
List.iter (function c -> ctr := !c + !ctr) ctrs;
Printf.printf "%d\n" !ctr
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Thu, 2018-04-12 at 08:22 +0200, Julia Lawall wrote: > On Wed, 11 Apr 2018, Joe Perches wrote: > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > > We already have some 500 bools-in-structs > > > > I got at least triple that only in include/ > > so I expect there are at probably an order > > of magnitude more than 500 in the kernel. > > > > I suppose some cocci script could count the > > actual number of instances. A regex can not. > > I got 12667. Could you please post the cocci script? > I'm not sure to understand the issue. Will using a bitfield help if there > are no other bitfields in the structure? IMO, not really. The primary issue is described by Linus here: https://lkml.org/lkml/2017/11/21/384 I personally do not find a significant issue with uncontrolled sizes of bool in kernel structs as all of the kernel structs are transitory and not written out to storage. I suppose bool bitfields are also OK, but for the RMW required. Using unsigned int :1 bitfield instead of bool :1 has the negative of truncation so that the uint has to be set with !! instead of a simple assign.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 11 Apr 2018, Joe Perches wrote: > (Adding Julia Lawall) > > On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > > We already have some 500 bools-in-structs > > I got at least triple that only in include/ > so I expect there are at probably an order > of magnitude more than 500 in the kernel. > > I suppose some cocci script could count the > actual number of instances. A regex can not. I got 12667. I'm not sure to understand the issue. Will using a bitfield help if there are no other bitfields in the structure? julia > > > and the owners of that code will > > be wondering whether they should change them, and whether they should > > apply those remove-bool-in-struct patches which someone sent them. > > Which is why the warning is --strict only > > > So... can we please get some clarity here? > > > > ... > > > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > > > hm, Linus suggests that instead of using > > > > bool mybool; > > > > we should use > > > > unsigned mybool:1; > > > > However that introduces the risk that alterations of mybool will use > > nonatomic rmw operations. > > > > unsigned myboolA:1; > > unsigned myboolB:1; > > > > so > > > > foo->myboolA = 1; > > > > could scribble on concurrent alterations of foo->myboolB. I think. > > Without barriers, that could happen anyway. > > To me, the biggest problem with conversions > from bool to bitfield is logical. ie: > > unsigned int.singlebitfield = 4; > > is not the same result as > > bool = 4; > > because of implicit truncation vs boolean conversion > so a direct change of bool use in structs to unsigned > would also require logic analysis. > >
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, Apr 11, 2018 at 09:29:59AM -0700, Andrew Morton wrote: > > OK. I guess. But I'm not really seeing some snappy description which > helps people understand why checkpatch is warning about this. "Results in architecture dependent layout." is the best short sentence I can come up with. > We already have some 500 bools-in-structs and the owners of that code > will be wondering whether they should change them, and whether they > should apply those remove-bool-in-struct patches which someone sent > them. I still have room in my /dev/null mailbox for pure checkpatch patches. > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) Yes, we really should not use lkml.org for references. Sadly google displays it very prominently when you search for something. > hm, Linus suggests that instead of using > > bool mybool; > > we should use > > unsigned mybool:1; > > However that introduces the risk that alterations of mybool will use > nonatomic rmw operations. > > unsigned myboolA:1; > unsigned myboolB:1; > > so > > foo->myboolA = 1; > > could scribble on concurrent alterations of foo->myboolB. I think. So that is true of u8 on Alpha I guess that risk is also present if myboolA and myboolB were `bool', > too. The compiler could do any old thing with them including, perhaps, > using a single-bit bitfield(?). The smallest addressable type in C is a byte, so while _Bool may be larger than a byte, it cannot be smaller. Otherwise we could not write: _Bool var; _Boll *ptr = &var; Which is something that comes apart with bitfields.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
(Adding Julia Lawall) On Wed, 2018-04-11 at 09:29 -0700, Andrew Morton wrote: > We already have some 500 bools-in-structs I got at least triple that only in include/ so I expect there are at probably an order of magnitude more than 500 in the kernel. I suppose some cocci script could count the actual number of instances. A regex can not. > and the owners of that code will > be wondering whether they should change them, and whether they should > apply those remove-bool-in-struct patches which someone sent them. Which is why the warning is --strict only > So... can we please get some clarity here? > ... > > (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) > > hm, Linus suggests that instead of using > > bool mybool; > > we should use > > unsigned mybool:1; > > However that introduces the risk that alterations of mybool will use > nonatomic rmw operations. > > unsigned myboolA:1; > unsigned myboolB:1; > > so > > foo->myboolA = 1; > > could scribble on concurrent alterations of foo->myboolB. I think. Without barriers, that could happen anyway. To me, the biggest problem with conversions from bool to bitfield is logical. ie: unsigned int.singlebitfield = 4; is not the same result as bool = 4; because of implicit truncation vs boolean conversion so a direct change of bool use in structs to unsigned would also require logic analysis.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Wed, 11 Apr 2018 10:15:02 +0200 Peter Zijlstra wrote: > On Tue, Apr 10, 2018 at 03:00:11PM -0700, Andrew Morton wrote: > > On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches wrote: > > > > > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote: > > > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches wrote: > > > > > > > > > A struct with a bool member can have different sizes on various > > > > > architectures because neither bool size nor alignment is standardized. > > > > > > > > What's wrong with bools in structs? > > > > > > See above. > > > > Yeah, but so what? `long' has different sizes on different > > architectures too. > > Right, so we have ILP32/LP64 for all our 32/64 bit archs respectively. > So only 2 possible variations to consider, and if you know your bitness > you know your layout. > > (+- some really unfortunate alignment exceptions, the worst of which > Arnd recently removed, hooray!) > > But neither says anything about sizeof(_Bool), and the standard leaves > it undefined and only mandates it is large enough to store either 0 or > 1 (and I suspect this vagueness is because there are architectures that > either have no byte addressibility or it's more expensive than word > addressibility). > > Typically GCC chooses a single byte to represent _Bool, but there are no > guarantees. This means that when you care about structure layout (as we > all really should) things go wobbly when you use _Bool. > > If GCC were to guarantee a 1 byte _Bool for all Linux ABIs we could > reconsider. OK. I guess. But I'm not really seeing some snappy description which helps people understand why checkpatch is warning about this. We already have some 500 bools-in-structs and the owners of that code will be wondering whether they should change them, and whether they should apply those remove-bool-in-struct patches which someone sent them. So... can we please get some clarity here? ... (ooh, https://lkml.org/lkml/2017/11/21/384 is working this morning) hm, Linus suggests that instead of using bool mybool; we should use unsigned mybool:1; However that introduces the risk that alterations of mybool will use nonatomic rmw operations. unsigned myboolA:1; unsigned myboolB:1; so foo->myboolA = 1; could scribble on concurrent alterations of foo->myboolB. I think. I guess that risk is also present if myboolA and myboolB were `bool', too. The compiler could do any old thing with them including, perhaps, using a single-bit bitfield(?).
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Tue, Apr 10, 2018 at 03:00:11PM -0700, Andrew Morton wrote: > On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches wrote: > > > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote: > > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches wrote: > > > > > > > A struct with a bool member can have different sizes on various > > > > architectures because neither bool size nor alignment is standardized. > > > > > > What's wrong with bools in structs? > > > > See above. > > Yeah, but so what? `long' has different sizes on different > architectures too. Right, so we have ILP32/LP64 for all our 32/64 bit archs respectively. So only 2 possible variations to consider, and if you know your bitness you know your layout. (+- some really unfortunate alignment exceptions, the worst of which Arnd recently removed, hooray!) But neither says anything about sizeof(_Bool), and the standard leaves it undefined and only mandates it is large enough to store either 0 or 1 (and I suspect this vagueness is because there are architectures that either have no byte addressibility or it's more expensive than word addressibility). Typically GCC chooses a single byte to represent _Bool, but there are no guarantees. This means that when you care about structure layout (as we all really should) things go wobbly when you use _Bool. If GCC were to guarantee a 1 byte _Bool for all Linux ABIs we could reconsider.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Tue, 10 Apr 2018 14:53:51 -0700 Joe Perches wrote: > On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote: > > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches wrote: > > > > > A struct with a bool member can have different sizes on various > > > architectures because neither bool size nor alignment is standardized. > > > > What's wrong with bools in structs? > > See above. Yeah, but so what? `long' has different sizes on different architectures too.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Tue, 2018-04-10 at 14:39 -0700, Andrew Morton wrote: > On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches wrote: > > > A struct with a bool member can have different sizes on various > > architectures because neither bool size nor alignment is standardized. > > What's wrong with bools in structs? See above.
Re: [PATCH] checkpatch: Add a --strict test for structs with bool member definitions
On Tue, 10 Apr 2018 11:19:54 -0700 Joe Perches wrote:
> A struct with a bool member can have different sizes on various
> architectures because neither bool size nor alignment is standardized.
>
> So emit a message on the use of bool in structs only in .h files and
> not .c files.
>
> There is the real possibility that this test could have a false positive
> when a bool is declared as an automatic, so limit the test to .h files
> where the only false positive is for declarations in static inline functions.
What's wrong with bools in structs? The changelog should be
self-contained, please. At least add a link in the changelog (of the
lkml.kernel.org/r/MSGID variety), but a link in the changelog is risky
because the reader may be offline or the server may be down.
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6257,6 +6257,13 @@ sub process {
>"Avoid using bool as bitfield. Prefer bool
> bitfields as unsigned int or u<8|16|32>\n" . $herecurr);
> }
>
> +# check for bool use in .h files
> + if ($realfile =~ /\.h$/ &&
> + $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
> + CHK("BOOL_MEMBER",
> + "Avoid using bool structure members because of
> possible alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n"; .
> $herecurr);
And... the server is down. Am unable to understand or review this patch!
> + }
> +
> # check for semaphores initialized locked
> if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
> WARN("CONSIDER_COMPLETION",
