| Message ID | 20240927-cocci-6-12-v1-3-a318d4e6a19d@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: static-analyzers: Fix 6.12-rc1 cocci warnings | expand |
On Fri, Sep 27, 2024 at 09:42:15AM +0000, Ricardo Ribalda wrote: > The max() macro produce nicer code and also fixes the following cocci > errors: > > drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max() > drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max() In some (rare) cases it's a bad advice. NAK. ... > - v >>= fit_shift > 0 ? fit_shift : 0; > + v >>= max(fit_shift, 0); max() with 0 is a bit unusual, esp. taking into account that the operator here is a right shift. So, what we check here is the signedness of the value to avoid not only potentially wrong calculations, but also UB. The original code is clearer for all this.
On 27/09/2024 11:42, Ricardo Ribalda wrote: > The max() macro produce nicer code and also fixes the following cocci > errors: > > drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max() > drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max() > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h > index 8ba65161f7a9..9642506d2388 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h > +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h > @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b) > int fit_shift = sFRACTION_BITS_FITTING(a) - b; > > v >>= sSHIFT; > - v >>= fit_shift > 0 ? fit_shift : 0; > + v >>= max(fit_shift, 0); Does the warning go away if you change this to: if (fit_shift > 0) v >>= fit_shift; Using 'max' for a shift is a bit weird in my opinion. Also this change was done to reduce the min/max calls, so introducing a new max call feels odd (although it should be fine). Note that I think those cocci warnings should perhaps be ignored or dropped. In part because of the huge macro expansion of min and max, but also I often find the code that is not using min or max at least as readable, if not more. Regards, Hans > > return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX); > } > @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b) > int fit_shift = uFRACTION_BITS_FITTING(a) - b; > > v >>= uSHIFT; > - v >>= fit_shift > 0 ? fit_shift : 0; > + v >>= max(fit_shift, 0); > > return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX); > } >
From: Ricardo Ribalda > Sent: 27 September 2024 10:42 > > The max() macro produce nicer code and also fixes the following cocci > errors: > > drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max() > drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max() > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h > b/drivers/staging/media/atomisp/pci/sh_css_frac.h > index 8ba65161f7a9..9642506d2388 100644 > --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h > +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h > @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b) > int fit_shift = sFRACTION_BITS_FITTING(a) - b; > > v >>= sSHIFT; IIRC right shifts of signed values are undefined. (C does not require a cpu to have a right shift that replicates the sign bit.) > - v >>= fit_shift > 0 ? fit_shift : 0; > + v >>= max(fit_shift, 0); If the shift isn't done the return value is garbage. So the code better not let it happen. In which case you might as well let the cpu generate a (different) random value - so delete the conditional. > > return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX); all three values seem to be 'int' - so no need for the _t variant and all the associated casts. > } > @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b) > int fit_shift = uFRACTION_BITS_FITTING(a) - b; > > v >>= uSHIFT; > - v >>= fit_shift > 0 ? fit_shift : 0; > + v >>= max(fit_shift, 0); > > return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX); as above, but it is just min(v, iISP_VAL_MAX) David > } > > -- > 2.46.1.824.gd892dcdcdd-goog > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi, On 29-Sep-24 11:34 PM, David Laight wrote: > From: Ricardo Ribalda >> Sent: 27 September 2024 10:42 >> >> The max() macro produce nicer code and also fixes the following cocci >> errors: >> >> drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max() >> drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max() >> >> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> >> --- >> drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h >> b/drivers/staging/media/atomisp/pci/sh_css_frac.h >> index 8ba65161f7a9..9642506d2388 100644 >> --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h >> +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h >> @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b) >> int fit_shift = sFRACTION_BITS_FITTING(a) - b; >> >> v >>= sSHIFT; > > IIRC right shifts of signed values are undefined. > (C does not require a cpu to have a right shift that replicates the > sign bit.) > >> - v >>= fit_shift > 0 ? fit_shift : 0; >> + v >>= max(fit_shift, 0); > > If the shift isn't done the return value is garbage. > So the code better not let it happen. > In which case you might as well let the cpu generate a (different) > random value - so delete the conditional. Given the history of this code I would no be surprised if some weird corner case actually relies on the check, so NACK for dropping the conditional. > >> >> return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX); > > all three values seem to be 'int' - so no need for the _t variant > and all the associated casts. sDIGIT_FITTING() originally was a macro with a bunch of max() + min() calls nested leading to it expanding to a lot of code after running it through the pre-processor. When converting this to a static online to choice was made to with clamp_t() to avoid the overhead of the extra type checks in regular clamp(). Regards, Hans > >> } >> @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b) >> int fit_shift = uFRACTION_BITS_FITTING(a) - b; >> >> v >>= uSHIFT; >> - v >>= fit_shift > 0 ? fit_shift : 0; >> + v >>= max(fit_shift, 0); >> >> return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX); > > as above, but it is just min(v, iISP_VAL_MAX) > > David > >> } >> >> -- >> 2.46.1.824.gd892dcdcdd-goog >> > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales)
diff --git a/drivers/staging/media/atomisp/pci/sh_css_frac.h b/drivers/staging/media/atomisp/pci/sh_css_frac.h index 8ba65161f7a9..9642506d2388 100644 --- a/drivers/staging/media/atomisp/pci/sh_css_frac.h +++ b/drivers/staging/media/atomisp/pci/sh_css_frac.h @@ -37,7 +37,7 @@ static inline int sDIGIT_FITTING(int v, int a, int b) int fit_shift = sFRACTION_BITS_FITTING(a) - b; v >>= sSHIFT; - v >>= fit_shift > 0 ? fit_shift : 0; + v >>= max(fit_shift, 0); return clamp_t(int, v, sISP_VAL_MIN, sISP_VAL_MAX); } @@ -47,7 +47,7 @@ static inline unsigned int uDIGIT_FITTING(unsigned int v, int a, int b) int fit_shift = uFRACTION_BITS_FITTING(a) - b; v >>= uSHIFT; - v >>= fit_shift > 0 ? fit_shift : 0; + v >>= max(fit_shift, 0); return clamp_t(unsigned int, v, uISP_VAL_MIN, uISP_VAL_MAX); }
The max() macro produce nicer code and also fixes the following cocci errors: drivers/staging/media/atomisp/pci/sh_css_frac.h:40:17-18: WARNING opportunity for max() drivers/staging/media/atomisp/pci/sh_css_frac.h:50:17-18: WARNING opportunity for max() Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/staging/media/atomisp/pci/sh_css_frac.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)