[PATCH] D142609: [Clang] Fix -Wconstant-logical-operand when LHS is a constant

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

In D142609#4511579 <https://reviews.llvm.org/D142609#4511579>, @nickdesaulniers wrote:

> In D142609#4507696 <https://reviews.llvm.org/D142609#4507696>, @nathanchance wrote:
>
>> but I see a new one along a similar line as those:
>>
>> https://elixir.bootlin.com/linux/v6.5-rc2/source/drivers/gpu/drm/v3d/v3d_drv.h#L343
>
> I only see 4 instances of `NSEC_PER_SEC % HZ` in mainline. 2 already use the C preprocessor (inconsistently), and 2 don't. I think the 2 that don't could be converted to use the preprocessor, then that issue goes away. So I think we can clean that up on the kernel side.

Is this diff what you had in mind?

  diff --git a/drivers/gpu/drm/i915/gem/i915_gem_wait.c b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
  index 4a33ad2d122b..d7dd93da2511 100644
  --- a/drivers/gpu/drm/i915/gem/i915_gem_wait.c
  +++ b/drivers/gpu/drm/i915/gem/i915_gem_wait.c
  @@ -186,9 +186,10 @@ 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 &&
  -           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
  +#if (NSEC_PER_SEC % HZ) != 0
  +       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                  return MAX_JIFFY_OFFSET;
  +#endif
  
          return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
   }
  diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
  index b74b1351bfc8..af1470ee477d 100644
  --- a/drivers/gpu/drm/v3d/v3d_drv.h
  +++ b/drivers/gpu/drm/v3d/v3d_drv.h
  @@ -340,9 +340,10 @@ struct v3d_submit_ext {
   static inline unsigned long nsecs_to_jiffies_timeout(const u64 n)
   {
          /* nsecs_to_jiffies64() does not guard against overflow */
  -       if (NSEC_PER_SEC % HZ &&
  -           div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
  +#if (NSEC_PER_SEC % HZ) != 0
  +       if (div_u64(n, NSEC_PER_SEC) >= MAX_JIFFY_OFFSET / HZ)
                  return MAX_JIFFY_OFFSET;
  +#endif
  
          return min_t(u64, MAX_JIFFY_OFFSET, nsecs_to_jiffies64(n) + 1);
   }

Not sure how the kernel maintainers will like that, they generally do not like to use preprocessing for hiding warnings. Alternatively, we could just turn the checks into booleans, which is what I tested earlier.

  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;
  diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
  index b74b1351bfc8..7f664a4b2a75 100644
  --- a/drivers/gpu/drm/v3d/v3d_drv.h
  +++ b/drivers/gpu/drm/v3d/v3d_drv.h
  @@ -340,7 +340,7 @@ struct v3d_submit_ext {
   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;

I don't really have a strong preference either way but warnings that show up with certain configuration values and not others are annoying for things like randconfig testing, although I suppose that is always the nature of certain warnings...

> @nathanchance were there many other instances of warnings triggered by this patch beyond that?

No, those were the only two instances that I saw across my whole build matrix with the current version of the patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142609/new/

https://reviews.llvm.org/D142609