Hi Wolfram,
On Fri, Oct 05, 2018 at 06:27:28PM +0200, Wolfram Sang wrote:
>
> > May I ask how exactly you spotted the "shift-31-problem" in
> > drivers/i2c/busses/i2c-rcar.c:
> > - visual code review?
> > - static analysis, special compiler flags?
>
> This one. I run a set of static code analyziers when applying patches.
> One of them is 'cppcheck' which reported it.
Indeed, cppcheck reports w/o this patch:
[drivers/i2c/busses/i2c-rcar.c:972]: (error) Shifting signed 32-bit value by 31
bits is undefined behaviour
[drivers/i2c/busses/i2c-rcar.c:1008]: (error) Shifting signed 32-bit value by
31 bits is undefined behaviour
>
> > According to feedback from GCC community [2], with 'gcc -std=gnu89',
> > shifting into (not past) the sign bit is "defined behavior" which is why
> > UBSAN doesn't report this as an issue in Linux kernel. That makes me
>
> I see. I guess it can be argued. Yet, BIT() solves other issues as well
> ('1' vs '1u'), so this was probably a reasonable move nonetheless, plus
> we are super-super-sure about the shifting now.
>
I agree. There is no doubt that avoiding/fixing shifting into the sign
bit makes the code more portable and will lessen the pain when
switching Kbuild to C99/C11 (if ever needed). I still have open
questions, but since they go beyond i2c framework and beyond kernel
itself (as said, they originate from porting UBSan to U-Boot), I will
discuss them elsewhere.
Thanks again for the reply.
Best regards,
Eugeniu.
