Messages in this thread | | | Subject | Re: [PATCH 1/2] media: v4l2-core: Shifting signed 32-bit value by 31 bits error | From | Shuah Khan <> | Date | Tue, 11 Jun 2019 16:27:58 -0600 |
| |
On 6/11/19 2:50 PM, Hans Verkuil wrote: > On 6/11/19 9:42 PM, Shuah Khan wrote: >> On 6/6/19 12:33 AM, Hans Verkuil wrote: >>> On 6/6/19 5:22 AM, Randy Dunlap wrote: >>>> On 6/5/19 2:53 PM, Shuah Khan wrote: >>>>> Fix the following cppcheck error: >>>>> >>>>> Checking drivers/media/v4l2-core/v4l2-ioctl.c ... >>>>> [drivers/media/v4l2-core/v4l2-ioctl.c:1370]: (error) Shifting signed 32-bit value by 31 bits is undefined behaviour >>>>> >>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org> >>>>> --- >>>>> drivers/media/v4l2-core/v4l2-ioctl.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> index 6859bdac86fe..333e387bafeb 100644 >>>>> --- a/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c >>>>> @@ -1364,7 +1364,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt) >>>>> (char)((fmt->pixelformat >> 8) & 0x7f), >>>>> (char)((fmt->pixelformat >> 16) & 0x7f), >>>>> (char)((fmt->pixelformat >> 24) & 0x7f), >>>>> - (fmt->pixelformat & (1 << 31)) ? "-BE" : ""); >>>>> + (fmt->pixelformat & BIT(31)) ? "-BE" : ""); >>>>> break; >>>>> } >>>>> } >>>>> >>>> >>>> If this builds, I guess #define BIT(x) got pulled in indirectly >>>> since bits.h nor bitops.h is currently #included in that source file. >>>> >> >> It does build. You are right that I should have included bitops.h >> >>>> Documentation/process/submit-checklist.rst rule #1 says: >>>> 1) If you use a facility then #include the file that defines/declares >>>> that facility. Don't depend on other header files pulling in ones >>>> that you use. >>>> >>>> Please add #include <linux/bits or bitops.h> >>>> >>> >>> I'm not sure about this patch. '1 << 31' is used all over in the kernel, >>> including in public headers (e.g. media.h, videodev2.h). >>> >>> It seems arbitrary to change it only here, but not anywhere else. >>> >> >> Right. We have several places in the kernel that do that. >> >>> In this particular example for the fourcc handling I would prefer to just >>> use '1U << 31', both in v4l2-ioctl.c and videodev2.h. >>> >> >> If you would like to take the patch, I can send v2 fixing it using >> 1U << 31 - This is simpler since it doesn't nee additional includes. > > I would like to have this cleaned up in the public media APIs. Those can be > used by other compilers as well and it makes sense to me not to have > undefined behavior in those headers. >
Great. That is a good point. I will start looking at the public media APIs.
>> >>> A separate patch doing the same for MEDIA_ENT_ID_FLAG_NEXT in media.h would >>> probably be a good idea either: that way the public API at least will do >>> the right thing. >>>
Sounds good.
>> >> I should have explained it better. I wanted to start with one or two >> places first to see if it is worth our time to fix these: >> >> The full kernel cppcheck log for "Shifting signed 32-bit value by 31 >> bits is undefined behaviour" can be found at: >> >> https://drive.google.com/file/d/19Xu7UqBGJ7BpzxEp92ZQYb6F8UPrk3z3/view > > I don't think it makes sense to fix this for drivers. If gcc would do this > wrong, we'd have noticed it ages ago.
Agreed. I am not concerned about it being incorrect. More for silencing cppcheck. I do agree that it isn't of a great value to us.
> > But I think it makes sense to fix this in public headers. >
Yes. This would definitely help.
thanks, -- Shuah
| |