| Message ID | 20230718-nsecs_to_jiffies_timeout-constant-logical-operand-v1-2-36ed8fc8faea@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout() | expand |
On 18/07/2023 22:44, Nathan Chancellor wrote: > A proposed update to clang's -Wconstant-logical-operand to warn when the > left hand side is a constant shows the following instance in > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > such as CONFIG_HZ=300: > > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] > 189 | if (NSEC_PER_SEC % HZ && > | ~~~~~~~~~~~~~~~~~ ^ > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation > 189 | if (NSEC_PER_SEC % HZ && > | ^~ > | & > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning > 1 warning generated. > > Turn this into an explicit comparison against zero to make the > expression a boolean to make it clear this should be a logical check, > not a bitwise one. So -Wconstant-logical-operand only triggers when it is a constant but not zero constant? Why does that make sense is not a kludge to avoid too much noise? Personally, it all feels a bit over the top as a warning, since code in both cases should optimise away. And we may end up papering over it if it becomes a default. Then again this patch IMO does make the code more readable, so I am happy to take this one via our tree. Or either give ack to bring it in via drm-misc-next: Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Let me know which route works best. Regards, Tvrtko > Link: https://reviews.llvm.org/D142609 > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > index 4a33ad2d122b..d4b918fb11ce 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c > @@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, > static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) > { > /* nsecs_to_jiffies64() does not guard against overflow */ > - if (NSEC_PER_SEC % HZ && > + if ((NSEC_PER_SEC % HZ) != 0 && > div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) > return MAX_JIFFY_OFFSET; > >
On Thu, Jul 20, 2023 at 09:43:05AM +0100, Tvrtko Ursulin wrote: > > On 18/07/2023 22:44, Nathan Chancellor wrote: > > A proposed update to clang's -Wconstant-logical-operand to warn when the > > left hand side is a constant shows the following instance in > > nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, > > such as CONFIG_HZ=300: > > > > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] > > 189 | if (NSEC_PER_SEC % HZ && > > | ~~~~~~~~~~~~~~~~~ ^ > > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation > > 189 | if (NSEC_PER_SEC % HZ && > > | ^~ > > | & > > drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning > > 1 warning generated. > > > > Turn this into an explicit comparison against zero to make the > > expression a boolean to make it clear this should be a logical check, > > not a bitwise one. > > So -Wconstant-logical-operand only triggers when it is a > constant but not zero constant? Why does that make sense is not > a kludge to avoid too much noise? Yes, the warning purposefully does not trigger when the constant is a 1 or 0 (as those are usually indicative of an intentional logical operation): https://github.com/llvm/llvm-project/blob/dfdfd306cfaf54fbc43e2d5eb36489dac3eb9976/clang/lib/Sema/SemaExpr.cpp#L13917-L13919 In this case, it is 100, so I kind of understand why this might be ambiguous to the compiler. > Personally, it all feels a bit over the top as a warning, > since code in both cases should optimise away. And we may end I do not necessarily disagree, as you can see from the differential review that I linked in the message, but I also understand it is a fine line to tread when writing compiler warnings between wanting to catch as many potential problems as possible and having too much noise for developers to sift through. I think this is erring on the side of caution. > up papering over it if it becomes a default. diagtool tree tells me this warning is already on by default. > Then again this patch IMO does make the code more readable, so I think so too. > I am happy to take this one via our tree. Or either give ack to > bring it in via drm-misc-next: > > Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Let me know which route works best. Thanks for the feedback! Either route is fine with me but if the v3d patch is going to go in via drm-misc-next, it seems like it would not be too much trouble to push this one with it. Cheers, Nathan
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c index 4a33ad2d122b..d4b918fb11ce 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c @@ -186,7 +186,7 @@ i915_gem_object_wait(struct drm_i915_gem_object *obj, static inline unsigned long nsecs_to_jiffies_timeout(const u64 n) { /* nsecs_to_jiffies64() does not guard against overflow */ - if (NSEC_PER_SEC % HZ && + if ((NSEC_PER_SEC % HZ) != 0 && div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ) return MAX_JIFFY_OFFSET;
A proposed update to clang's -Wconstant-logical-operand to warn when the left hand side is a constant shows the following instance in nsecs_to_jiffies_timeout() when NSEC_PER_SEC is not a multiple of HZ, such as CONFIG_HZ=300: drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: warning: use of logical '&&' with constant operand [-Wconstant-logical-operand] 189 | if (NSEC_PER_SEC % HZ && | ~~~~~~~~~~~~~~~~~ ^ drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: use '&' for a bitwise operation 189 | if (NSEC_PER_SEC % HZ && | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_wait.c:189:24: note: remove constant to silence this warning 1 warning generated. Turn this into an explicit comparison against zero to make the expression a boolean to make it clear this should be a logical check, not a bitwise one. Link: https://reviews.llvm.org/D142609 Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- drivers/gpu/drm/i915/gem/i915_gem_wait.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)