Re: [Outreachy kernel] [RESEND PATCH 2/2] staging: vboxvideo: Use unsigned int instead bool

[Himanshu Jha <himanshujha199640@gmail.com>]

Hi Sasha,

On Fri, Oct 26, 2018 at 04:42:25PM -0400, Sasha Levin wrote:
> On Fri, Oct 26, 2018 at 04:04:45PM -0300, Shayenne da Luz Moura wrote:
> > This change was suggested by checkpath.pl. Use unsigned int with bitfield
> > allocate only one bit to the boolean variable.
> > 
> > CHECK: Avoid using bool structure members because of possible alignment
> > issues
> > 
> > Signed-off-by: Shayenne da Luz Moura <shayenneluzmoura@gmail.com>
> > ---
> > drivers/staging/vboxvideo/vbox_drv.h        | 14 +++++++-------
> > drivers/staging/vboxvideo/vboxvideo_guest.h |  2 +-
> > 2 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/vboxvideo/vbox_drv.h b/drivers/staging/vboxvideo/vbox_drv.h
> > index 594f84272957..7d3e329a6b1c 100644
> > --- a/drivers/staging/vboxvideo/vbox_drv.h
> > +++ b/drivers/staging/vboxvideo/vbox_drv.h
> > @@ -81,7 +81,7 @@ struct vbox_private {
> > 	u8 __iomem *vbva_buffers;
> > 	struct gen_pool *guest_pool;
> > 	struct vbva_buf_ctx *vbva_info;
> > -	bool any_pitch;
> > +	unsigned int any_pitch:1;
> > 	u32 num_crtcs;
> > 	/** Amount of available VRAM, including space used for buffers. */
> > 	u32 full_vram_size;
> 
> Using bitfields for booleans in these cases is less efficient than just
> using "regular" booleans for two reasons:
> 
> 1. It will use the same amount of space. Due to alignment requirements,
> the compiler can't squeeze in anything into the 7 bits that are now
> "free". Each member, unless it's another bitfield, must start at a whole
> byte.

Agreed!

FYI original thread of discussion: https://lkml.org/lkml/2017/11/21/207

As Steve says:

"Thus, changing:

 int  a : 1;
 int  b : 1;
 int  c : 1;
 int  d : 1;

to

 bool a;
 bool b;
 bool c;
 bool d;

at best increases the size required from 1 byte to 4 bytes, and at
worse, it increases it from one byte to 16 bytes."

In the above cases, we have all bitfields members and no non-bitfields
members.

But before playing with these bitfields there are some points to be
noted:
https://port70.net/~nsz/c/c11/n1570.html#J.3.9

Implementation Defined
----------------------

* Whether a ''plain'' int bit-field is treated as a signed int 
  bit-field or as an unsigned int bit-field.

eg: `int foo:3` may have a range from 0..7 or -4..3

So, changing `int foo:3` to `unsigned int foo:3` might be a sane
change to remove the ambiguity and make sure range is 0..7.

Also, such an change can also handle unsigned overflow
or better said "wrapping".

* Whether a bit-field can straddle a storage-unit boundary.

So, you can't guess what could be the possible unless you're familiar
or tested the change for different arch.

* The alignment of non-bit-field members of structures.

...

The "possible alignement issues" in CHECK report is difficult to figure
out by just doing a glance analysis. :)

Linus also suggested to use bool as the base type i.e., `bool x:1` but
again sizeof(_Bool) is implementation defined ranging from 1-4 bytes.

And since this issue is added to checkpatch now, very likely there would
be blast of patches sent on the same.

Not everyone who sends checkpatch would be able to disassemble or test
on different implementations.

But if anyone is interested check this:
https://godbolt.org/


-- 
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology