| Message ID | 34d53778977747f19cce2abb287bb3e6@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | minmax.h: Cleanups and minor optimisations | expand |
Hi, On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity > check of the bounds in clamp(). > Gives better error coverage and one less expansion of the arguments. > > Signed-off-by: David Laight <david.laight@aculab.com> This patch triggers a build error when trying to build parisc:allmodconfig. See error message and bisect log below. I don't think there is anything wrong with the patch. The underlying problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which tries to build the affected file even though CONFIG_DRM_I915 is not enabled/supported on parisc. Copying XE maintainers for feedback/advice. Thanks, Guenter --- Building parisc:allmodconfig ... failed -------------- Error log: In file included from <command-line>: drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale': include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert' 523 | prefix ## suffix(); \ | ^~~~~~ include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert' 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) | ^~~~~~~~~~~~~~~~~~ include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' 188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ | ^~~~~~~~~~~~~~~~ include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once' 195 | __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_)) | ^~~~~~~~~~~~ include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp' 206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi) | ^~~~~~~~~~~~~~~ drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp' 48 | source_val = clamp(source_val, source_min, source_max); | ^~~~~ --- # bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117 # good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7 git bisect start 'HEAD' 'v6.13-rc7' # bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15 # bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4 # bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6 # bad: [7acb844a672defb15cf202a501815ec22c68c800] foo git bisect bad 7acb844a672defb15cf202a501815ec22c68c800 # good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field git bisect good fb2368075b135f174264071b851330649d55f9d0 # bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96 # good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page() git bisect good b04d305df1171448df5e87802c4d1f1022cc5784 # good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work() git bisect good 7e01619507058f90ab603acec482951f3c452aaa # good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions git bisect good 21b510a64c223707caa6db6176128779f0806a73 # good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b # good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp() git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2 # bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54 # bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp() git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9 # first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp()
On Sat, 18 Jan 2025 08:13:06 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > Hi, > > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity > > check of the bounds in clamp(). > > Gives better error coverage and one less expansion of the arguments. > > > > Signed-off-by: David Laight <david.laight@aculab.com> > > This patch triggers a build error when trying to build parisc:allmodconfig. > See error message and bisect log below. > > I don't think there is anything wrong with the patch. The underlying > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which > tries to build the affected file even though CONFIG_DRM_I915 is not > enabled/supported on parisc. This has appeared before. Any idea which inlined copy of scale() is causing the problem. On the face of it they all look ok. If you can reproduce it maybe try commenting out some of the calls. David > > Copying XE maintainers for feedback/advice. > > Thanks, > Guenter > > --- > Building parisc:allmodconfig ... failed > -------------- > Error log: > In file included from <command-line>: > drivers/gpu/drm/i915/display/intel_backlight.c: In function 'scale': > include/linux/compiler_types.h:542:45: error: call to '__compiletime_assert_415' declared with attribute error: clamp() low limit source_min greater than high limit source_max > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^ > include/linux/compiler_types.h:523:25: note: in definition of macro '__compiletime_assert' > 523 | prefix ## suffix(); \ > | ^~~~~~ > include/linux/compiler_types.h:542:9: note: in expansion of macro '_compiletime_assert' > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) > | ^~~~~~~~~~~~~~~~~~~ > include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert' > 39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg) > | ^~~~~~~~~~~~~~~~~~ > include/linux/minmax.h:188:9: note: in expansion of macro 'BUILD_BUG_ON_MSG' > 188 | BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ > | ^~~~~~~~~~~~~~~~ > include/linux/minmax.h:195:9: note: in expansion of macro '__clamp_once' > 195 | __clamp_once(type, val, lo, hi, __UNIQUE_ID(v_), __UNIQUE_ID(l_), __UNIQUE_ID(h_)) > | ^~~~~~~~~~~~ > include/linux/minmax.h:206:28: note: in expansion of macro '__careful_clamp' > 206 | #define clamp(val, lo, hi) __careful_clamp(__auto_type, val, lo, hi) > | ^~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/display/intel_backlight.c:48:22: note: in expansion of macro 'clamp' > 48 | source_val = clamp(source_val, source_min, source_max); > | ^~~~~ > > --- > # bad: [0907e7fb35756464aa34c35d6abb02998418164b] Add linux-next specific files for 20250117 > # good: [5bc55a333a2f7316b58edc7573e8e893f7acb532] Linux 6.13-rc7 > git bisect start 'HEAD' 'v6.13-rc7' > # bad: [195cedf4deacf84167c32b866ceac1cf4a16df15] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git > git bisect bad 195cedf4deacf84167c32b866ceac1cf4a16df15 > # bad: [e8c0711b153b0db806410d8b31ed23b590f4eab4] Merge branch 'xtensa-for-next' of git://github.com/jcmvbkbc/linux-xtensa.git > git bisect bad e8c0711b153b0db806410d8b31ed23b590f4eab4 > # bad: [81d45722d699e594c66c150c8f7a0ec2e2bc9be6] Merge branch 'for-next/perf' of git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git > git bisect bad 81d45722d699e594c66c150c8f7a0ec2e2bc9be6 > # bad: [7acb844a672defb15cf202a501815ec22c68c800] foo > git bisect bad 7acb844a672defb15cf202a501815ec22c68c800 > # good: [fb2368075b135f174264071b851330649d55f9d0] mm/damon/core: add damos_filter->allow field > git bisect good fb2368075b135f174264071b851330649d55f9d0 > # bad: [fc83c501e385753c90db7316faf9fd4158caaa96] minmax.h: remove some #defines that are only expanded once > git bisect bad fc83c501e385753c90db7316faf9fd4158caaa96 > # good: [b04d305df1171448df5e87802c4d1f1022cc5784] ocfs2: use a folio in ocfs2_map_and_dirty_page() > git bisect good b04d305df1171448df5e87802c4d1f1022cc5784 > # good: [7e01619507058f90ab603acec482951f3c452aaa] kthread: correct comments before kthread_queue_work() > git bisect good 7e01619507058f90ab603acec482951f3c452aaa > # good: [21b510a64c223707caa6db6176128779f0806a73] nilfs2: correct return value kernel-doc descriptions for ioctl functions > git bisect good 21b510a64c223707caa6db6176128779f0806a73 > # good: [6afb87f23458f2d4e4334ee5a4efb8b0d07af68b] nilfs2: handle errors that nilfs_prepare_chunk() may return > git bisect good 6afb87f23458f2d4e4334ee5a4efb8b0d07af68b > # good: [8f6d46fed0bad163e5146fea1fdff150039235b2] minmax.h: reduce the #define expansion of min(), max() and clamp() > git bisect good 8f6d46fed0bad163e5146fea1fdff150039235b2 > # bad: [7a70c678548d71e609b95dbddf2d411a02d13b54] minmax.h: move all the clamp() definitions after the min/max() ones > git bisect bad 7a70c678548d71e609b95dbddf2d411a02d13b54 > # bad: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp() > git bisect bad 37f375aab0c585388b90d1af6968454fc2769cb9 > # first bad commit: [37f375aab0c585388b90d1af6968454fc2769cb9] minmax.h: use BUILD_BUG_ON_MSG() for the lo < hi test in clamp() > >
On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote: > On Sat, 18 Jan 2025 08:13:06 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > > > Hi, > > > > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: > > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity > > > check of the bounds in clamp(). > > > Gives better error coverage and one less expansion of the arguments. > > > > > > Signed-off-by: David Laight <david.laight@aculab.com> > > > > This patch triggers a build error when trying to build parisc:allmodconfig. > > See error message and bisect log below. > > > > I don't think there is anything wrong with the patch. The underlying > > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which > > tries to build the affected file even though CONFIG_DRM_I915 is not > > enabled/supported on parisc. > > This has appeared before. > Any idea which inlined copy of scale() is causing the problem. > On the face of it they all look ok. > > If you can reproduce it maybe try commenting out some of the calls. > See diff below. All three changes are needed. No idea why the compiler would know that the values are invalid. Guenter --- diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c index fc1e517e074a..3b2c8bdfcf8d 100644 --- a/drivers/gpu/drm/i915/display/intel_backlight.c +++ b/drivers/gpu/drm/i915/display/intel_backlight.c @@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector, static u32 scale_hw_to_user(struct intel_connector *connector, u32 hw_level, u32 user_max) { +#if 0 struct intel_panel *panel = &connector->panel; return scale(hw_level, panel->backlight.min, panel->backlight.max, 0, user_max); +#else + return 0; +#endif } u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val) @@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val) drm_WARN_ON_ONCE(&i915->drm, panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0); +#if 0 val = scale(val, panel->backlight.min, panel->backlight.max, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max); +#endif return intel_backlight_invert_pwm_level(connector, val); } @@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val) intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS))) val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min); +#if 0 return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max, panel->backlight.min, panel->backlight.max); +#else + return 0; +#endif } static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)
On Sat, 18 Jan 2025 09:49:21 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote: > > On Sat, 18 Jan 2025 08:13:06 -0800 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > > > Hi, > > > > > > On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: > > > > Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity > > > > check of the bounds in clamp(). > > > > Gives better error coverage and one less expansion of the arguments. > > > > > > > > Signed-off-by: David Laight <david.laight@aculab.com> > > > > > > This patch triggers a build error when trying to build parisc:allmodconfig. > > > See error message and bisect log below. > > > > > > I don't think there is anything wrong with the patch. The underlying > > > problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which > > > tries to build the affected file even though CONFIG_DRM_I915 is not > > > enabled/supported on parisc. > > > > This has appeared before. > > Any idea which inlined copy of scale() is causing the problem. > > On the face of it they all look ok. > > > > If you can reproduce it maybe try commenting out some of the calls. > > > > See diff below. All three changes are needed. > No idea why the compiler would know that the values are invalid. Maybe it isn't even an inlining issue. Perhaps that compiler just doesn't like the function ? What happens without the 'static' (and an extra prototype)? David > > Guenter > > --- > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c > index fc1e517e074a..3b2c8bdfcf8d 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c > @@ -76,10 +76,14 @@ static u32 clamp_user_to_hw(struct intel_connector *connector, > static u32 scale_hw_to_user(struct intel_connector *connector, > u32 hw_level, u32 user_max) > { > +#if 0 > struct intel_panel *panel = &connector->panel; > > return scale(hw_level, panel->backlight.min, panel->backlight.max, > 0, user_max); > +#else > + return 0; > +#endif > } > > u32 intel_backlight_invert_pwm_level(struct intel_connector *connector, u32 val) > @@ -119,8 +123,10 @@ u32 intel_backlight_level_to_pwm(struct intel_connector *connector, u32 val) > drm_WARN_ON_ONCE(&i915->drm, > panel->backlight.max == 0 || panel->backlight.pwm_level_max == 0); > > +#if 0 > val = scale(val, panel->backlight.min, panel->backlight.max, > panel->backlight.pwm_level_min, panel->backlight.pwm_level_max); > +#endif > > return intel_backlight_invert_pwm_level(connector, val); > } > @@ -138,8 +144,12 @@ u32 intel_backlight_level_from_pwm(struct intel_connector *connector, u32 val) > intel_has_quirk(display, QUIRK_INVERT_BRIGHTNESS))) > val = panel->backlight.pwm_level_max - (val - panel->backlight.pwm_level_min); > > +#if 0 > return scale(val, panel->backlight.pwm_level_min, panel->backlight.pwm_level_max, > panel->backlight.min, panel->backlight.max); > +#else > + return 0; > +#endif > } > > static u32 lpt_get_backlight(struct intel_connector *connector, enum pipe unused)
On 1/18/25 10:09, David Laight wrote: > On Sat, 18 Jan 2025 09:49:21 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote: >>> On Sat, 18 Jan 2025 08:13:06 -0800 >>> Guenter Roeck <linux@roeck-us.net> wrote: >>> >>>> Hi, >>>> >>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: >>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity >>>>> check of the bounds in clamp(). >>>>> Gives better error coverage and one less expansion of the arguments. >>>>> >>>>> Signed-off-by: David Laight <david.laight@aculab.com> >>>> >>>> This patch triggers a build error when trying to build parisc:allmodconfig. >>>> See error message and bisect log below. >>>> >>>> I don't think there is anything wrong with the patch. The underlying >>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which >>>> tries to build the affected file even though CONFIG_DRM_I915 is not >>>> enabled/supported on parisc. >>> >>> This has appeared before. >>> Any idea which inlined copy of scale() is causing the problem. >>> On the face of it they all look ok. >>> >>> If you can reproduce it maybe try commenting out some of the calls. >>> >> >> See diff below. All three changes are needed. >> No idea why the compiler would know that the values are invalid. > > Maybe it isn't even an inlining issue. > Perhaps that compiler just doesn't like the function ? > What happens without the 'static' (and an extra prototype)? > You mean like that ? -static u32 scale(u32 source_val, + +u32 scale(u32 source_val, + u32 source_min, u32 source_max, + u32 target_min, u32 target_max); + +u32 scale(u32 source_val, u32 source_min, u32 source_max, u32 target_min, u32 target_max) It doesn't help. Worse, after that change the error is still reported even with the #if 0 elsewhere. Guenter
On Sat, 18 Jan 2025 10:36:11 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On 1/18/25 10:09, David Laight wrote: > > On Sat, 18 Jan 2025 09:49:21 -0800 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > >> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote: > >>> On Sat, 18 Jan 2025 08:13:06 -0800 > >>> Guenter Roeck <linux@roeck-us.net> wrote: > >>> > >>>> Hi, > >>>> > >>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: > >>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity > >>>>> check of the bounds in clamp(). > >>>>> Gives better error coverage and one less expansion of the arguments. > >>>>> > >>>>> Signed-off-by: David Laight <david.laight@aculab.com> > >>>> > >>>> This patch triggers a build error when trying to build parisc:allmodconfig. > >>>> See error message and bisect log below. > >>>> > >>>> I don't think there is anything wrong with the patch. The underlying > >>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which > >>>> tries to build the affected file even though CONFIG_DRM_I915 is not > >>>> enabled/supported on parisc. > >>> > >>> This has appeared before. > >>> Any idea which inlined copy of scale() is causing the problem. > >>> On the face of it they all look ok. > >>> > >>> If you can reproduce it maybe try commenting out some of the calls. > >>> > >> > >> See diff below. All three changes are needed. > >> No idea why the compiler would know that the values are invalid. > > > > Maybe it isn't even an inlining issue. > > Perhaps that compiler just doesn't like the function ? > > What happens without the 'static' (and an extra prototype)? > > > > > You mean like that ? > > -static u32 scale(u32 source_val, > + > +u32 scale(u32 source_val, > + u32 source_min, u32 source_max, > + u32 target_min, u32 target_max); > + > +u32 scale(u32 source_val, > u32 source_min, u32 source_max, > u32 target_min, u32 target_max) > > It doesn't help. Worse, after that change the error is still reported > even with the #if 0 elsewhere. Yes - that means the compiler is 'objecting' to the scale() function itself. (Without any regard for its callers.) Which should make it easy to reproduce outside the kernel build. I think Mat had a successful build with a different (older?) version of gcc for parisc. There must be something odd causing the problem - there will be other clamp() calls in the build that don't generate the error. Remember that lack of the error messages requires the compiler optimise away some code - so if the optimisation is skipped the call could be generated and the warning output (even if the call is optimised away later). Perhaps there is some obscure interaction with the WARN() statements? I don't have the required compiler (neither does godbolt). David > > Guenter >
On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > > No idea why the compiler would know that the values are invalid. It's not that the compiler knows tat they are invalid, but I bet what happens is in scale() (and possibly other places that do similar checks), which does this: WARN_ON(source_min > source_max); ... source_val = clamp(source_val, source_min, source_max); and the compiler notices that the ordering comparison in the first WARN_ON() is the same as the one in clamp(), so it basically converts the logic to if (source_min > source_max) { WARN(..); /* Do the clamp() knowing that source_min > source_max */ source_val = clamp(source_val, source_min, source_max); } else { /* Do the clamp knowing that source_min <= source_max */ source_val = clamp(source_val, source_min, source_max); } (obviously I dropped the other WARN_ON in the conversion, it wasn't relevant for this case). And now that first clamp() case is done with source_min > source_max, and it triggers that build error because that's invalid. So the condition is not statically true in the *source* code, but in the "I have moved code around to combine tests" case it now *is* statically true as far as the compiler is concerned. Linus
On 1/18/25 13:18, David Laight wrote: > On Sat, 18 Jan 2025 10:36:11 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On 1/18/25 10:09, David Laight wrote: >>> On Sat, 18 Jan 2025 09:49:21 -0800 >>> Guenter Roeck <linux@roeck-us.net> wrote: >>> >>>> On Sat, Jan 18, 2025 at 05:09:59PM +0000, David Laight wrote: >>>>> On Sat, 18 Jan 2025 08:13:06 -0800 >>>>> Guenter Roeck <linux@roeck-us.net> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> On Mon, Nov 18, 2024 at 07:13:31PM +0000, David Laight wrote: >>>>>>> Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity >>>>>>> check of the bounds in clamp(). >>>>>>> Gives better error coverage and one less expansion of the arguments. >>>>>>> >>>>>>> Signed-off-by: David Laight <david.laight@aculab.com> >>>>>> >>>>>> This patch triggers a build error when trying to build parisc:allmodconfig. >>>>>> See error message and bisect log below. >>>>>> >>>>>> I don't think there is anything wrong with the patch. The underlying >>>>>> problem seems to be that parisc:allmodconfig enables CONFIG_DRM_XE which >>>>>> tries to build the affected file even though CONFIG_DRM_I915 is not >>>>>> enabled/supported on parisc. >>>>> >>>>> This has appeared before. >>>>> Any idea which inlined copy of scale() is causing the problem. >>>>> On the face of it they all look ok. >>>>> >>>>> If you can reproduce it maybe try commenting out some of the calls. >>>>> >>>> >>>> See diff below. All three changes are needed. >>>> No idea why the compiler would know that the values are invalid. >>> >>> Maybe it isn't even an inlining issue. >>> Perhaps that compiler just doesn't like the function ? >>> What happens without the 'static' (and an extra prototype)? >>> >> >> >> You mean like that ? >> >> -static u32 scale(u32 source_val, >> + >> +u32 scale(u32 source_val, >> + u32 source_min, u32 source_max, >> + u32 target_min, u32 target_max); >> + >> +u32 scale(u32 source_val, >> u32 source_min, u32 source_max, >> u32 target_min, u32 target_max) >> >> It doesn't help. Worse, after that change the error is still reported >> even with the #if 0 elsewhere. > > Yes - that means the compiler is 'objecting' to the scale() function itself. > (Without any regard for its callers.) > Which should make it easy to reproduce outside the kernel build. > > I think Mat had a successful build with a different (older?) version of gcc for > parisc. > > There must be something odd causing the problem - there will be other clamp() > calls in the build that don't generate the error. > > Remember that lack of the error messages requires the compiler optimise away > some code - so if the optimisation is skipped the call could be generated > and the warning output (even if the call is optimised away later). > > Perhaps there is some obscure interaction with the WARN() statements? > > I don't have the required compiler (neither does godbolt). > Oh man - that was a good hint. Turns out I can only reproduce the problem with gcc 13.2 and 13.3. gcc 10.3, 11.4, and 12.4 do not generate the error. And, yes, I can "fix" the problem with - WARN_ON(source_min > source_max); + // WARN_ON(source_min > source_max); Any idea what to do ? Should I just scrap builds with gcc 13.x ? Thanks, Guenter
On 1/18/25 13:21, Linus Torvalds wrote: > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: >> >> No idea why the compiler would know that the values are invalid. > > It's not that the compiler knows tat they are invalid, but I bet what > happens is in scale() (and possibly other places that do similar > checks), which does this: > > WARN_ON(source_min > source_max); > ... > source_val = clamp(source_val, source_min, source_max); > > and the compiler notices that the ordering comparison in the first > WARN_ON() is the same as the one in clamp(), so it basically converts > the logic to > > if (source_min > source_max) { > WARN(..); > /* Do the clamp() knowing that source_min > source_max */ > source_val = clamp(source_val, source_min, source_max); > } else { > /* Do the clamp knowing that source_min <= source_max */ > source_val = clamp(source_val, source_min, source_max); > } > > (obviously I dropped the other WARN_ON in the conversion, it wasn't > relevant for this case). > > And now that first clamp() case is done with source_min > source_max, > and it triggers that build error because that's invalid. > > So the condition is not statically true in the *source* code, but in > the "I have moved code around to combine tests" case it now *is* > statically true as far as the compiler is concerned. > Yes, turns out I can reproduce the problem by adding WARN_ON() ahead of similar clamp() calls (see below). However, I can only reproduce it with gcc 13.3 for parisc. I don't see the problem with other cross compilers (I tried arm, powerpc, and loongarch64). Compilers are weird :-(. I am not sure what to do here. That kind of problem seems difficult to avoid, and I am sure we will hit it again elsewhere. Should I declare gcc 13.x off limits for parisc builds ? Guenter --- diff --git a/drivers/input/mousedev.c b/drivers/input/mousedev.c index 505c562a5daa..71c0da31a9d2 100644 --- a/drivers/input/mousedev.c +++ b/drivers/input/mousedev.c @@ -179,6 +179,7 @@ static void mousedev_abs_event(struct input_dev *dev, struct mousedev *mousedev, if (size == 0) size = xres ? : 1; + WARN_ON(min > max); value = clamp(value, min, max); mousedev->packet.x = ((value - min) * xres) / size;
On Sat, 18 Jan 2025 at 13:59, Guenter Roeck <linux@roeck-us.net> wrote: > > I am not sure what to do here. That kind of problem seems difficult > to avoid, and I am sure we will hit it again elsewhere. Should I declare > gcc 13.x off limits for parisc builds ? No, I'm sure it can happen on other architectures too. I think the only thing that makes parisc trigger this is that its WARN_ON() is slightly different from others, and uses the "__ret_warn_on" a few more times, and that just happens to make the compiler decide to simplify all those tests. Or something like that. Linus
On Sat, 18 Jan 2025 13:21:39 -0800 Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > > > > No idea why the compiler would know that the values are invalid. > > It's not that the compiler knows tat they are invalid, but I bet what > happens is in scale() (and possibly other places that do similar > checks), which does this: > > WARN_ON(source_min > source_max); > ... > source_val = clamp(source_val, source_min, source_max); > > and the compiler notices that the ordering comparison in the first > WARN_ON() is the same as the one in clamp(), so it basically converts > the logic to > > if (source_min > source_max) { > WARN(..); > /* Do the clamp() knowing that source_min > source_max */ > source_val = clamp(source_val, source_min, source_max); > } else { > /* Do the clamp knowing that source_min <= source_max */ > source_val = clamp(source_val, source_min, source_max); > } > > (obviously I dropped the other WARN_ON in the conversion, it wasn't > relevant for this case). > > And now that first clamp() case is done with source_min > source_max, > and it triggers that build error because that's invalid. > > So the condition is not statically true in the *source* code, but in > the "I have moved code around to combine tests" case it now *is* > statically true as far as the compiler is concerned. Well spotted :-) One option would be to move the WARN_ON() below the clamp() and add an OPTIMISER_HIDE_VAR(source_max) between them. Or do something more sensible than the WARN(). Perhaps return target_min on any such errors? David > > Linus >
On 1/18/25 14:11, David Laight wrote: > On Sat, 18 Jan 2025 13:21:39 -0800 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> No idea why the compiler would know that the values are invalid. >> >> It's not that the compiler knows tat they are invalid, but I bet what >> happens is in scale() (and possibly other places that do similar >> checks), which does this: >> >> WARN_ON(source_min > source_max); >> ... >> source_val = clamp(source_val, source_min, source_max); >> >> and the compiler notices that the ordering comparison in the first >> WARN_ON() is the same as the one in clamp(), so it basically converts >> the logic to >> >> if (source_min > source_max) { >> WARN(..); >> /* Do the clamp() knowing that source_min > source_max */ >> source_val = clamp(source_val, source_min, source_max); >> } else { >> /* Do the clamp knowing that source_min <= source_max */ >> source_val = clamp(source_val, source_min, source_max); >> } >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't >> relevant for this case). >> >> And now that first clamp() case is done with source_min > source_max, >> and it triggers that build error because that's invalid. >> >> So the condition is not statically true in the *source* code, but in >> the "I have moved code around to combine tests" case it now *is* >> statically true as far as the compiler is concerned. > > Well spotted :-) > > One option would be to move the WARN_ON() below the clamp() and > add an OPTIMISER_HIDE_VAR(source_max) between them. > > Or do something more sensible than the WARN(). > Perhaps return target_min on any such errors? > This helps: - WARN_ON(source_min > source_max); - WARN_ON(target_min > target_max); - /* defensive */ source_val = clamp(source_val, source_min, source_max); + WARN_ON(source_min > source_max); + WARN_ON(target_min > target_max); Guenter
On Sat, Jan 18, 2025 at 10:11 PM David Laight <david.laight.linux@gmail.com> wrote: > > On Sat, 18 Jan 2025 13:21:39 -0800 > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > No idea why the compiler would know that the values are invalid. > > > > It's not that the compiler knows tat they are invalid, but I bet what > > happens is in scale() (and possibly other places that do similar > > checks), which does this: > > > > WARN_ON(source_min > source_max); > > ... > > source_val = clamp(source_val, source_min, source_max); > > > > and the compiler notices that the ordering comparison in the first > > WARN_ON() is the same as the one in clamp(), so it basically converts > > the logic to > > > > if (source_min > source_max) { > > WARN(..); > > /* Do the clamp() knowing that source_min > source_max */ > > source_val = clamp(source_val, source_min, source_max); > > } else { > > /* Do the clamp knowing that source_min <= source_max */ > > source_val = clamp(source_val, source_min, source_max); > > } > > > > (obviously I dropped the other WARN_ON in the conversion, it wasn't > > relevant for this case). > > > > And now that first clamp() case is done with source_min > source_max, > > and it triggers that build error because that's invalid. > > > > So the condition is not statically true in the *source* code, but in > > the "I have moved code around to combine tests" case it now *is* > > statically true as far as the compiler is concerned. > > Well spotted :-) > > One option would be to move the WARN_ON() below the clamp() and > add an OPTIMISER_HIDE_VAR(source_max) between them. > > Or do something more sensible than the WARN(). Given the awful problems we've found with these macros (clamp, min, max, etc), maybe the best option is to not play these awful games in general? These macros seem footgunned to hell just as a way to try to compensate for C's lack of language features.
On Sat, 18 Jan 2025 14:58:48 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On 1/18/25 14:11, David Laight wrote: > > On Sat, 18 Jan 2025 13:21:39 -0800 > > Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > >>> > >>> No idea why the compiler would know that the values are invalid. > >> > >> It's not that the compiler knows tat they are invalid, but I bet what > >> happens is in scale() (and possibly other places that do similar > >> checks), which does this: > >> > >> WARN_ON(source_min > source_max); > >> ... > >> source_val = clamp(source_val, source_min, source_max); > >> > >> and the compiler notices that the ordering comparison in the first > >> WARN_ON() is the same as the one in clamp(), so it basically converts > >> the logic to > >> > >> if (source_min > source_max) { > >> WARN(..); > >> /* Do the clamp() knowing that source_min > source_max */ > >> source_val = clamp(source_val, source_min, source_max); > >> } else { > >> /* Do the clamp knowing that source_min <= source_max */ > >> source_val = clamp(source_val, source_min, source_max); > >> } > >> > >> (obviously I dropped the other WARN_ON in the conversion, it wasn't > >> relevant for this case). > >> > >> And now that first clamp() case is done with source_min > source_max, > >> and it triggers that build error because that's invalid. > >> > >> So the condition is not statically true in the *source* code, but in > >> the "I have moved code around to combine tests" case it now *is* > >> statically true as far as the compiler is concerned. > > > > Well spotted :-) > > > > One option would be to move the WARN_ON() below the clamp() and > > add an OPTIMISER_HIDE_VAR(source_max) between them. > > > > Or do something more sensible than the WARN(). > > Perhaps return target_min on any such errors? > > > > This helps: > > - WARN_ON(source_min > source_max); > - WARN_ON(target_min > target_max); > - > /* defensive */ > source_val = clamp(source_val, source_min, source_max); > > + WARN_ON(source_min > source_max); > + WARN_ON(target_min > target_max); That is a 'quick fix' ... Much better would be to replace the WARN() with (say): if (target_min >= target_max) return target_min; if (source_min >= source_max) return target_min + (target_max - target_min)/2; So that the return values are actually in range (in as much as one is defined). Note that the >= cpmparisons also remove a divide by zero. David > > Guenter >
On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > On Sat, 18 Jan 2025 14:58:48 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > >> On 1/18/25 14:11, David Laight wrote: >> > On Sat, 18 Jan 2025 13:21:39 -0800 >> > Linus Torvalds <torvalds@linux-foundation.org> wrote: >> > >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: >> >>> >> >>> No idea why the compiler would know that the values are invalid. >> >> >> >> It's not that the compiler knows tat they are invalid, but I bet what >> >> happens is in scale() (and possibly other places that do similar >> >> checks), which does this: >> >> >> >> WARN_ON(source_min > source_max); >> >> ... >> >> source_val = clamp(source_val, source_min, source_max); >> >> >> >> and the compiler notices that the ordering comparison in the first >> >> WARN_ON() is the same as the one in clamp(), so it basically converts >> >> the logic to >> >> >> >> if (source_min > source_max) { >> >> WARN(..); >> >> /* Do the clamp() knowing that source_min > source_max */ >> >> source_val = clamp(source_val, source_min, source_max); >> >> } else { >> >> /* Do the clamp knowing that source_min <= source_max */ >> >> source_val = clamp(source_val, source_min, source_max); >> >> } >> >> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't >> >> relevant for this case). >> >> >> >> And now that first clamp() case is done with source_min > source_max, >> >> and it triggers that build error because that's invalid. >> >> >> >> So the condition is not statically true in the *source* code, but in >> >> the "I have moved code around to combine tests" case it now *is* >> >> statically true as far as the compiler is concerned. >> > >> > Well spotted :-) >> > >> > One option would be to move the WARN_ON() below the clamp() and >> > add an OPTIMISER_HIDE_VAR(source_max) between them. >> > >> > Or do something more sensible than the WARN(). >> > Perhaps return target_min on any such errors? >> > >> >> This helps: >> >> - WARN_ON(source_min > source_max); >> - WARN_ON(target_min > target_max); >> - >> /* defensive */ >> source_val = clamp(source_val, source_min, source_max); >> >> + WARN_ON(source_min > source_max); >> + WARN_ON(target_min > target_max); > > That is a 'quick fix' ... > > Much better would be to replace the WARN() with (say): > if (target_min >= target_max) > return target_min; > if (source_min >= source_max) > return target_min + (target_max - target_min)/2; > So that the return values are actually in range (in as much as one is defined). > Note that the >= cpmparisons also remove a divide by zero. I want the loud and early warnings for clear bugs instead of "gracefully" silencing the errors only to be found through debugging user reports. BR, Jani.
On Mon, 20 Jan 2025 12:48:11 +0200 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > > On Sat, 18 Jan 2025 14:58:48 -0800 > > Guenter Roeck <linux@roeck-us.net> wrote: > > > >> On 1/18/25 14:11, David Laight wrote: > >> > On Sat, 18 Jan 2025 13:21:39 -0800 > >> > Linus Torvalds <torvalds@linux-foundation.org> wrote: > >> > > >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > >> >>> > >> >>> No idea why the compiler would know that the values are invalid. > >> >> > >> >> It's not that the compiler knows tat they are invalid, but I bet what > >> >> happens is in scale() (and possibly other places that do similar > >> >> checks), which does this: > >> >> > >> >> WARN_ON(source_min > source_max); > >> >> ... > >> >> source_val = clamp(source_val, source_min, source_max); > >> >> > >> >> and the compiler notices that the ordering comparison in the first > >> >> WARN_ON() is the same as the one in clamp(), so it basically converts > >> >> the logic to > >> >> > >> >> if (source_min > source_max) { > >> >> WARN(..); > >> >> /* Do the clamp() knowing that source_min > source_max */ > >> >> source_val = clamp(source_val, source_min, source_max); > >> >> } else { > >> >> /* Do the clamp knowing that source_min <= source_max */ > >> >> source_val = clamp(source_val, source_min, source_max); > >> >> } > >> >> > >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't > >> >> relevant for this case). > >> >> > >> >> And now that first clamp() case is done with source_min > source_max, > >> >> and it triggers that build error because that's invalid. > >> >> > >> >> So the condition is not statically true in the *source* code, but in > >> >> the "I have moved code around to combine tests" case it now *is* > >> >> statically true as far as the compiler is concerned. > >> > > >> > Well spotted :-) > >> > > >> > One option would be to move the WARN_ON() below the clamp() and > >> > add an OPTIMISER_HIDE_VAR(source_max) between them. > >> > > >> > Or do something more sensible than the WARN(). > >> > Perhaps return target_min on any such errors? > >> > > >> > >> This helps: > >> > >> - WARN_ON(source_min > source_max); > >> - WARN_ON(target_min > target_max); > >> - > >> /* defensive */ > >> source_val = clamp(source_val, source_min, source_max); > >> > >> + WARN_ON(source_min > source_max); > >> + WARN_ON(target_min > target_max); > > > > That is a 'quick fix' ... > > > > Much better would be to replace the WARN() with (say): > > if (target_min >= target_max) > > return target_min; > > if (source_min >= source_max) > > return target_min + (target_max - target_min)/2; > > So that the return values are actually in range (in as much as one is defined). > > Note that the >= cpmparisons also remove a divide by zero. > > I want the loud and early warnings for clear bugs instead of > "gracefully" silencing the errors only to be found through debugging > user reports. A user isn't going to notice a WARN() - not until you tell them to look for it. In any case even if you output a message you really want to return a 'sane' value, who knows what effect a very out of range value is going to have. David
On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > On Mon, 20 Jan 2025 12:48:11 +0200 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: >> > On Sat, 18 Jan 2025 14:58:48 -0800 >> > Guenter Roeck <linux@roeck-us.net> wrote: >> > >> >> On 1/18/25 14:11, David Laight wrote: >> >> > On Sat, 18 Jan 2025 13:21:39 -0800 >> >> > Linus Torvalds <torvalds@linux-foundation.org> wrote: >> >> > >> >> >> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: >> >> >>> >> >> >>> No idea why the compiler would know that the values are invalid. >> >> >> >> >> >> It's not that the compiler knows tat they are invalid, but I bet what >> >> >> happens is in scale() (and possibly other places that do similar >> >> >> checks), which does this: >> >> >> >> >> >> WARN_ON(source_min > source_max); >> >> >> ... >> >> >> source_val = clamp(source_val, source_min, source_max); >> >> >> >> >> >> and the compiler notices that the ordering comparison in the first >> >> >> WARN_ON() is the same as the one in clamp(), so it basically converts >> >> >> the logic to >> >> >> >> >> >> if (source_min > source_max) { >> >> >> WARN(..); >> >> >> /* Do the clamp() knowing that source_min > source_max */ >> >> >> source_val = clamp(source_val, source_min, source_max); >> >> >> } else { >> >> >> /* Do the clamp knowing that source_min <= source_max */ >> >> >> source_val = clamp(source_val, source_min, source_max); >> >> >> } >> >> >> >> >> >> (obviously I dropped the other WARN_ON in the conversion, it wasn't >> >> >> relevant for this case). >> >> >> >> >> >> And now that first clamp() case is done with source_min > source_max, >> >> >> and it triggers that build error because that's invalid. >> >> >> >> >> >> So the condition is not statically true in the *source* code, but in >> >> >> the "I have moved code around to combine tests" case it now *is* >> >> >> statically true as far as the compiler is concerned. >> >> > >> >> > Well spotted :-) >> >> > >> >> > One option would be to move the WARN_ON() below the clamp() and >> >> > add an OPTIMISER_HIDE_VAR(source_max) between them. >> >> > >> >> > Or do something more sensible than the WARN(). >> >> > Perhaps return target_min on any such errors? >> >> > >> >> >> >> This helps: >> >> >> >> - WARN_ON(source_min > source_max); >> >> - WARN_ON(target_min > target_max); >> >> - >> >> /* defensive */ >> >> source_val = clamp(source_val, source_min, source_max); >> >> >> >> + WARN_ON(source_min > source_max); >> >> + WARN_ON(target_min > target_max); >> > >> > That is a 'quick fix' ... >> > >> > Much better would be to replace the WARN() with (say): >> > if (target_min >= target_max) >> > return target_min; >> > if (source_min >= source_max) >> > return target_min + (target_max - target_min)/2; >> > So that the return values are actually in range (in as much as one is defined). >> > Note that the >= cpmparisons also remove a divide by zero. >> >> I want the loud and early warnings for clear bugs instead of >> "gracefully" silencing the errors only to be found through debugging >> user reports. > > A user isn't going to notice a WARN() - not until you tell them to look for it. > In any case even if you output a message you really want to return a 'sane' > value, who knows what effect a very out of range value is going to have. The point is, we'll catch the WARN in CI before it goes out to users. BR, Jani. > > David > >
On 1/20/25 03:21, Jani Nikula wrote: > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: >> On Mon, 20 Jan 2025 12:48:11 +0200 >> Jani Nikula <jani.nikula@linux.intel.com> wrote: >> >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: >>>> On Sat, 18 Jan 2025 14:58:48 -0800 >>>> Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>>> On 1/18/25 14:11, David Laight wrote: >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800 >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote: >>>>>> >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: >>>>>>>> >>>>>>>> No idea why the compiler would know that the values are invalid. >>>>>>> >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what >>>>>>> happens is in scale() (and possibly other places that do similar >>>>>>> checks), which does this: >>>>>>> >>>>>>> WARN_ON(source_min > source_max); >>>>>>> ... >>>>>>> source_val = clamp(source_val, source_min, source_max); >>>>>>> >>>>>>> and the compiler notices that the ordering comparison in the first >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts >>>>>>> the logic to >>>>>>> >>>>>>> if (source_min > source_max) { >>>>>>> WARN(..); >>>>>>> /* Do the clamp() knowing that source_min > source_max */ >>>>>>> source_val = clamp(source_val, source_min, source_max); >>>>>>> } else { >>>>>>> /* Do the clamp knowing that source_min <= source_max */ >>>>>>> source_val = clamp(source_val, source_min, source_max); >>>>>>> } >>>>>>> >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't >>>>>>> relevant for this case). >>>>>>> >>>>>>> And now that first clamp() case is done with source_min > source_max, >>>>>>> and it triggers that build error because that's invalid. >>>>>>> >>>>>>> So the condition is not statically true in the *source* code, but in >>>>>>> the "I have moved code around to combine tests" case it now *is* >>>>>>> statically true as far as the compiler is concerned. >>>>>> >>>>>> Well spotted :-) >>>>>> >>>>>> One option would be to move the WARN_ON() below the clamp() and >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them. >>>>>> >>>>>> Or do something more sensible than the WARN(). >>>>>> Perhaps return target_min on any such errors? >>>>>> >>>>> >>>>> This helps: >>>>> >>>>> - WARN_ON(source_min > source_max); >>>>> - WARN_ON(target_min > target_max); >>>>> - >>>>> /* defensive */ >>>>> source_val = clamp(source_val, source_min, source_max); >>>>> >>>>> + WARN_ON(source_min > source_max); >>>>> + WARN_ON(target_min > target_max); >>>> >>>> That is a 'quick fix' ... >>>> >>>> Much better would be to replace the WARN() with (say): >>>> if (target_min >= target_max) >>>> return target_min; >>>> if (source_min >= source_max) >>>> return target_min + (target_max - target_min)/2; >>>> So that the return values are actually in range (in as much as one is defined). >>>> Note that the >= cpmparisons also remove a divide by zero. >>> >>> I want the loud and early warnings for clear bugs instead of >>> "gracefully" silencing the errors only to be found through debugging >>> user reports. >> >> A user isn't going to notice a WARN() - not until you tell them to look for it. >> In any case even if you output a message you really want to return a 'sane' >> value, who knows what effect a very out of range value is going to have. > > The point is, we'll catch the WARN in CI before it goes out to users. > It isn't going to catch the divide by 0 error, and it obviously doesn't catch the build problem on parisc with gcc 13.x because the CI isn't testing it. How about disabling DRM_XE on architectures where it isn't supported, matching DRM_I915 ? Thanks, Guenter
On Mon, 20 Jan 2025 06:15:30 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > On 1/20/25 03:21, Jani Nikula wrote: > > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > >> On Mon, 20 Jan 2025 12:48:11 +0200 > >> Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> > >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > >>>> On Sat, 18 Jan 2025 14:58:48 -0800 > >>>> Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>>> On 1/18/25 14:11, David Laight wrote: > >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800 > >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote: > >>>>>> > >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > >>>>>>>> > >>>>>>>> No idea why the compiler would know that the values are invalid. > >>>>>>> > >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what > >>>>>>> happens is in scale() (and possibly other places that do similar > >>>>>>> checks), which does this: > >>>>>>> > >>>>>>> WARN_ON(source_min > source_max); > >>>>>>> ... > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> > >>>>>>> and the compiler notices that the ordering comparison in the first > >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts > >>>>>>> the logic to > >>>>>>> > >>>>>>> if (source_min > source_max) { > >>>>>>> WARN(..); > >>>>>>> /* Do the clamp() knowing that source_min > source_max */ > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> } else { > >>>>>>> /* Do the clamp knowing that source_min <= source_max */ > >>>>>>> source_val = clamp(source_val, source_min, source_max); > >>>>>>> } > >>>>>>> > >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't > >>>>>>> relevant for this case). > >>>>>>> > >>>>>>> And now that first clamp() case is done with source_min > source_max, > >>>>>>> and it triggers that build error because that's invalid. > >>>>>>> > >>>>>>> So the condition is not statically true in the *source* code, but in > >>>>>>> the "I have moved code around to combine tests" case it now *is* > >>>>>>> statically true as far as the compiler is concerned. > >>>>>> > >>>>>> Well spotted :-) > >>>>>> > >>>>>> One option would be to move the WARN_ON() below the clamp() and > >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them. > >>>>>> > >>>>>> Or do something more sensible than the WARN(). > >>>>>> Perhaps return target_min on any such errors? > >>>>>> > >>>>> > >>>>> This helps: > >>>>> > >>>>> - WARN_ON(source_min > source_max); > >>>>> - WARN_ON(target_min > target_max); > >>>>> - > >>>>> /* defensive */ > >>>>> source_val = clamp(source_val, source_min, source_max); > >>>>> > >>>>> + WARN_ON(source_min > source_max); > >>>>> + WARN_ON(target_min > target_max); > >>>> > >>>> That is a 'quick fix' ... > >>>> > >>>> Much better would be to replace the WARN() with (say): > >>>> if (target_min >= target_max) > >>>> return target_min; > >>>> if (source_min >= source_max) > >>>> return target_min + (target_max - target_min)/2; > >>>> So that the return values are actually in range (in as much as one is defined). > >>>> Note that the >= cpmparisons also remove a divide by zero. > >>> > >>> I want the loud and early warnings for clear bugs instead of > >>> "gracefully" silencing the errors only to be found through debugging > >>> user reports. > >> > >> A user isn't going to notice a WARN() - not until you tell them to look for it. > >> In any case even if you output a message you really want to return a 'sane' > >> value, who knows what effect a very out of range value is going to have. > > > > The point is, we'll catch the WARN in CI before it goes out to users. > > > > It isn't going to catch the divide by 0 error, and it obviously doesn't > catch the build problem on parisc with gcc 13.x because the CI isn't > testing it. > > How about disabling DRM_XE on architectures where it isn't supported, > matching DRM_I915 ? That'll just bite back later. As Linus spotted the compiler is just 'optimising' some code paths. It could happen for any architecture including x64. The repeated tests are basically slightly odd, although you might only expect them to be present in debug builds. An alternative would be to replace the clamp() with: if (source_val <= source_min) return target_min; if (source_val >= source_max) return target_max; David > > Thanks, > Guenter >
On Mon, Jan 20, 2025 at 06:41:43PM +0000, David Laight wrote: > On Mon, 20 Jan 2025 06:15:30 -0800 > Guenter Roeck <linux@roeck-us.net> wrote: > > On 1/20/25 03:21, Jani Nikula wrote: > > > On Mon, 20 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > > >> On Mon, 20 Jan 2025 12:48:11 +0200 > > >> Jani Nikula <jani.nikula@linux.intel.com> wrote: > > >>> On Sun, 19 Jan 2025, David Laight <david.laight.linux@gmail.com> wrote: > > >>>> On Sat, 18 Jan 2025 14:58:48 -0800 > > >>>> Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>> On 1/18/25 14:11, David Laight wrote: > > >>>>>> On Sat, 18 Jan 2025 13:21:39 -0800 > > >>>>>> Linus Torvalds <torvalds@linux-foundation.org> wrote: > > >>>>>>> On Sat, 18 Jan 2025 at 09:49, Guenter Roeck <linux@roeck-us.net> wrote: > > >>>>>>>> > > >>>>>>>> No idea why the compiler would know that the values are invalid. > > >>>>>>> > > >>>>>>> It's not that the compiler knows tat they are invalid, but I bet what > > >>>>>>> happens is in scale() (and possibly other places that do similar > > >>>>>>> checks), which does this: > > >>>>>>> > > >>>>>>> WARN_ON(source_min > source_max); > > >>>>>>> ... > > >>>>>>> source_val = clamp(source_val, source_min, source_max); > > >>>>>>> > > >>>>>>> and the compiler notices that the ordering comparison in the first > > >>>>>>> WARN_ON() is the same as the one in clamp(), so it basically converts > > >>>>>>> the logic to > > >>>>>>> > > >>>>>>> if (source_min > source_max) { > > >>>>>>> WARN(..); > > >>>>>>> /* Do the clamp() knowing that source_min > source_max */ > > >>>>>>> source_val = clamp(source_val, source_min, source_max); > > >>>>>>> } else { > > >>>>>>> /* Do the clamp knowing that source_min <= source_max */ > > >>>>>>> source_val = clamp(source_val, source_min, source_max); > > >>>>>>> } > > >>>>>>> > > >>>>>>> (obviously I dropped the other WARN_ON in the conversion, it wasn't > > >>>>>>> relevant for this case). > > >>>>>>> > > >>>>>>> And now that first clamp() case is done with source_min > source_max, > > >>>>>>> and it triggers that build error because that's invalid. > > >>>>>>> > > >>>>>>> So the condition is not statically true in the *source* code, but in > > >>>>>>> the "I have moved code around to combine tests" case it now *is* > > >>>>>>> statically true as far as the compiler is concerned. > > >>>>>> > > >>>>>> Well spotted :-) > > >>>>>> > > >>>>>> One option would be to move the WARN_ON() below the clamp() and > > >>>>>> add an OPTIMISER_HIDE_VAR(source_max) between them. > > >>>>>> > > >>>>>> Or do something more sensible than the WARN(). > > >>>>>> Perhaps return target_min on any such errors? > > >>>>>> > > >>>>> > > >>>>> This helps: > > >>>>> > > >>>>> - WARN_ON(source_min > source_max); > > >>>>> - WARN_ON(target_min > target_max); > > >>>>> - > > >>>>> /* defensive */ > > >>>>> source_val = clamp(source_val, source_min, source_max); > > >>>>> > > >>>>> + WARN_ON(source_min > source_max); > > >>>>> + WARN_ON(target_min > target_max); > > >>>> > > >>>> That is a 'quick fix' ... > > >>>> > > >>>> Much better would be to replace the WARN() with (say): > > >>>> if (target_min >= target_max) > > >>>> return target_min; > > >>>> if (source_min >= source_max) > > >>>> return target_min + (target_max - target_min)/2; > > >>>> So that the return values are actually in range (in as much as one is defined). > > >>>> Note that the >= cpmparisons also remove a divide by zero. > > >>> > > >>> I want the loud and early warnings for clear bugs instead of > > >>> "gracefully" silencing the errors only to be found through debugging > > >>> user reports. > > >> > > >> A user isn't going to notice a WARN() - not until you tell them to look for it. > > >> In any case even if you output a message you really want to return a 'sane' > > >> value, who knows what effect a very out of range value is going to have. > > > > > > The point is, we'll catch the WARN in CI before it goes out to users. > > > > It isn't going to catch the divide by 0 error, and it obviously doesn't > > catch the build problem on parisc with gcc 13.x because the CI isn't > > testing it. > > > > How about disabling DRM_XE on architectures where it isn't supported, > > matching DRM_I915 ? > > That'll just bite back later. > As Linus spotted the compiler is just 'optimising' some code paths. > It could happen for any architecture including x64. > The repeated tests are basically slightly odd, although you might only > expect them to be present in debug builds. > > An alternative would be to replace the clamp() with: > if (source_val <= source_min) > return target_min; > if (source_val >= source_max) > return target_max; Excuse me if I am missing something, but clamp() has a warning inside it, correct? Why do wee need an additional warning on top of that? P.S. However, I agree that ideally clamp() should work independently on the caller to use WARN*() or other similar stuff.
On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Excuse me if I am missing something, but clamp() has a warning inside it, correct? > Why do we need an additional warning on top of that? Note: the warning in clamp() only finds compile-time obvious wrong uses. It's really meant to notice the trivial case where you clam with constants and just got the order wrong, so you do something silly like res = clamp(in, 15, 1); but it does also end up catching slightly more complex things where the compiler can figure out the range of the clamping. The build problem then comes from the compiler doing various *other* code movem,ent and optimization too, and - like in this case - finds an error path where the clamping is done "wrong". I think the real issue in the i915 driver is that it does that WARN_ON(), but then it just happily continues anyway. So if the i915 driver instead did if (WARN_ON(..)) return invalid value; none of this would ever have happened. Linus
On 1/20/25 11:14, Linus Torvalds wrote: > On Mon, 20 Jan 2025 at 10:55, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> >> Excuse me if I am missing something, but clamp() has a warning inside it, correct? >> Why do we need an additional warning on top of that? > > Note: the warning in clamp() only finds compile-time obvious wrong uses. > > It's really meant to notice the trivial case where you clam with > constants and just got the order wrong, so you do something silly like > > res = clamp(in, 15, 1); > > but it does also end up catching slightly more complex things where > the compiler can figure out the range of the clamping. > > The build problem then comes from the compiler doing various *other* > code movem,ent and optimization too, and - like in this case - finds > an error path where the clamping is done "wrong". > > I think the real issue in the i915 driver is that it does that > WARN_ON(), but then it just happily continues anyway. > > So if the i915 driver instead did > > if (WARN_ON(..)) return invalid value; > > none of this would ever have happened. > I'll take a stab and send a patch combining your and David's suggestions. Guenter
diff --git a/include/linux/minmax.h b/include/linux/minmax.h index 6f7ea669d305..91aa1b90c1bb 100644 --- a/include/linux/minmax.h +++ b/include/linux/minmax.h @@ -106,8 +106,7 @@ __auto_type uval = (val); \ __auto_type ulo = (lo); \ __auto_type uhi = (hi); \ - static_assert(__builtin_choose_expr(__is_constexpr((lo) > (hi)), \ - (lo) <= (hi), true), \ + BUILD_BUG_ON_MSG(statically_true(ulo > uhi), \ "clamp() low limit " #lo " greater than high limit " #hi); \ BUILD_BUG_ON_MSG(!__types_ok3(uval, ulo, uhi), \ "clamp("#val", "#lo", "#hi") signedness error"); \
Use BUILD_BUG_ON_MSG(statically_true(ulo > uhi), ...) for the sanity check of the bounds in clamp(). Gives better error coverage and one less expansion of the arguments. Signed-off-by: David Laight <david.laight@aculab.com> --- include/linux/minmax.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)