Re: [Intel-gfx] [PATCH 2/2] drm/i915: Avoid -Wconstant-logical-operand in nsecs_to_jiffies_timeout()

[Nathan Chancellor <nathan@kernel.org>]

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