diff mbox series

[v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare

Message ID 20200211061338.23666-1-natechancellor@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] drm/i915: Disable -Wtautological-constant-out-of-range-compare | expand

Commit Message

Nathan Chancellor Feb. 11, 2020, 6:13 a.m. UTC
A recent commit in clang added -Wtautological-compare to -Wall, which is
enabled for i915 so we see the following warning:

../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
result of comparison of constant 576460752303423487 with expression of
type 'unsigned int' is always false
[-Wtautological-constant-out-of-range-compare]
        if (unlikely(remain > N_RELOC(ULONG_MAX)))
            ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~

This warning only happens on x86_64 but that check is relevant for
32-bit x86 so we cannot remove it. -Wtautological-compare on a whole has
good warnings but this one is not really relevant for the kernel because
of all of the different configurations that are used to build the
kernel. When -Wtautological-compare is enabled for the kernel, this
option will remain disabled so do that for i915 now.

Link: https://github.com/ClangBuiltLinux/linux/issues/778
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2: https://lore.kernel.org/lkml/20200211050808.29463-1-natechancellor@gmail.com/

* Fix patch application due to basing on a local tree that had
  -Wuninitialized turned on. Can confirm that patch applies on
  latest -next now.

 drivers/gpu/drm/i915/Makefile | 1 +
 1 file changed, 1 insertion(+)

Comments

Michel Dänzer Feb. 11, 2020, 9:41 a.m. UTC | #1
On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> A recent commit in clang added -Wtautological-compare to -Wall, which is
> enabled for i915 so we see the following warning:
> 
> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> result of comparison of constant 576460752303423487 with expression of
> type 'unsigned int' is always false
> [-Wtautological-constant-out-of-range-compare]
>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> 
> This warning only happens on x86_64 but that check is relevant for
> 32-bit x86 so we cannot remove it.

That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
in both cases, and remain is a 32-bit value in both cases. How can it be
larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
Nathan Chancellor Feb. 11, 2020, 8:39 p.m. UTC | #2
On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> > A recent commit in clang added -Wtautological-compare to -Wall, which is
> > enabled for i915 so we see the following warning:
> > 
> > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> > result of comparison of constant 576460752303423487 with expression of
> > type 'unsigned int' is always false
> > [-Wtautological-constant-out-of-range-compare]
> >         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > 
> > This warning only happens on x86_64 but that check is relevant for
> > 32-bit x86 so we cannot remove it.
> 
> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
> in both cases, and remain is a 32-bit value in both cases. How can it be
> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
> 

Hi Michel,

Can't this condition be true when UINT_MAX == ULONG_MAX? clang does not
warn on a 32-bit x86 build from what I remember. Honestly, my
understanding of overflow is pretty shoddy, this is mostly based on what
I have heard from others.

I sent a patch trying to remove that check but had it rejected:

https://lore.kernel.org/lkml/20191123195321.41305-1-natechancellor@gmail.com/

Cheers,
Nathan
Michel Dänzer Feb. 12, 2020, 8:52 a.m. UTC | #3
On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>>> enabled for i915 so we see the following warning:
>>>
>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
>>> result of comparison of constant 576460752303423487 with expression of
>>> type 'unsigned int' is always false
>>> [-Wtautological-constant-out-of-range-compare]
>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>>>
>>> This warning only happens on x86_64 but that check is relevant for
>>> 32-bit x86 so we cannot remove it.
>>
>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
>> in both cases, and remain is a 32-bit value in both cases. How can it be
>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
>>
> 
> Hi Michel,
> 
> Can't this condition be true when UINT_MAX == ULONG_MAX?

Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.


Anyway, this suggests a possible better solution:

#if UINT_MAX == ULONG_MAX
	if (unlikely(remain > N_RELOC(ULONG_MAX)))
		return -EINVAL;
#endif


Or if that can't be used for some reason, something like

	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
		return -EINVAL;

should silence the warning.


Either of these should be better than completely disabling the warning
for the whole file.
Nathan Chancellor Feb. 12, 2020, 5:07 p.m. UTC | #4
On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> > On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
> >> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> >>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> >>> enabled for i915 so we see the following warning:
> >>>
> >>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> >>> result of comparison of constant 576460752303423487 with expression of
> >>> type 'unsigned int' is always false
> >>> [-Wtautological-constant-out-of-range-compare]
> >>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> This warning only happens on x86_64 but that check is relevant for
> >>> 32-bit x86 so we cannot remove it.
> >>
> >> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
> >> in both cases, and remain is a 32-bit value in both cases. How can it be
> >> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
> >>
> > 
> > Hi Michel,
> > 
> > Can't this condition be true when UINT_MAX == ULONG_MAX?
> 
> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
> 
> 
> Anyway, this suggests a possible better solution:
> 
> #if UINT_MAX == ULONG_MAX
> 	if (unlikely(remain > N_RELOC(ULONG_MAX)))
> 		return -EINVAL;
> #endif
> 
> 
> Or if that can't be used for some reason, something like
> 
> 	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
> 		return -EINVAL;
> 
> should silence the warning.

I do like this one better than the former.

> 
> 
> Either of these should be better than completely disabling the warning
> for the whole file.

Normally, I would agree but I am currently planning to leave
-Wtautological-constant-out-of-range-compare disabled when I turn on
-Wtautological-compare for the whole kernel because there are plenty of
locations in the kernel where these kind of checks depend on various
kernel configuration options and the general attitude of kernel
developers is that this particular warning is not really helpful
for that reason.

I'll see if there is a general consensus before moving further since I
know i915 turns on a bunch of extra warnings from the rest of the kernel
(hence why we are in this situation).

Cheers,
Nathan
Michel Dänzer Feb. 12, 2020, 5:17 p.m. UTC | #5
On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
> On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
>> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
>>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
>>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
>>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>>>>> enabled for i915 so we see the following warning:
>>>>>
>>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
>>>>> result of comparison of constant 576460752303423487 with expression of
>>>>> type 'unsigned int' is always false
>>>>> [-Wtautological-constant-out-of-range-compare]
>>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
>>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>>>>>
>>>>> This warning only happens on x86_64 but that check is relevant for
>>>>> 32-bit x86 so we cannot remove it.
>>>>
>>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
>>>> in both cases, and remain is a 32-bit value in both cases. How can it be
>>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
>>>>
>>>
>>> Hi Michel,
>>>
>>> Can't this condition be true when UINT_MAX == ULONG_MAX?
>>
>> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
>>
>>
>> Anyway, this suggests a possible better solution:
>>
>> #if UINT_MAX == ULONG_MAX
>> 	if (unlikely(remain > N_RELOC(ULONG_MAX)))
>> 		return -EINVAL;
>> #endif
>>
>>
>> Or if that can't be used for some reason, something like
>>
>> 	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
>> 		return -EINVAL;
>>
>> should silence the warning.
> 
> I do like this one better than the former.

FWIW, one downside of this one compared to all alternatives (presumably)
is that it might end up generating actual code even on 64-bit, which
always ends up skipping the return.
Jani Nikula Feb. 13, 2020, 2:37 p.m. UTC | #6
On Wed, 12 Feb 2020, Michel Dänzer <michel@daenzer.net> wrote:
> On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
>> On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
>>> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
>>>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
>>>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
>>>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>>>>>> enabled for i915 so we see the following warning:
>>>>>>
>>>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
>>>>>> result of comparison of constant 576460752303423487 with expression of
>>>>>> type 'unsigned int' is always false
>>>>>> [-Wtautological-constant-out-of-range-compare]
>>>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
>>>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> This warning only happens on x86_64 but that check is relevant for
>>>>>> 32-bit x86 so we cannot remove it.
>>>>>
>>>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
>>>>> in both cases, and remain is a 32-bit value in both cases. How can it be
>>>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
>>>>>
>>>>
>>>> Hi Michel,
>>>>
>>>> Can't this condition be true when UINT_MAX == ULONG_MAX?
>>>
>>> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
>>>
>>>
>>> Anyway, this suggests a possible better solution:
>>>
>>> #if UINT_MAX == ULONG_MAX
>>> 	if (unlikely(remain > N_RELOC(ULONG_MAX)))
>>> 		return -EINVAL;
>>> #endif
>>>
>>>
>>> Or if that can't be used for some reason, something like
>>>
>>> 	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
>>> 		return -EINVAL;
>>>
>>> should silence the warning.
>> 
>> I do like this one better than the former.
>
> FWIW, one downside of this one compared to all alternatives (presumably)
> is that it might end up generating actual code even on 64-bit, which
> always ends up skipping the return.

I like this better than the UINT_MAX == ULONG_MAX comparison because
that creates a dependency on the type of remain.

Then again, a sufficiently clever compiler could see through the cast,
and flag the warning anyway...

BR,
Jani.
Nathan Chancellor Feb. 13, 2020, 9:48 p.m. UTC | #7
On Thu, Feb 13, 2020 at 04:37:15PM +0200, Jani Nikula wrote:
> On Wed, 12 Feb 2020, Michel Dänzer <michel@daenzer.net> wrote:
> > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
> >> On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
> >>> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> >>>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
> >>>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> >>>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> >>>>>> enabled for i915 so we see the following warning:
> >>>>>>
> >>>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> >>>>>> result of comparison of constant 576460752303423487 with expression of
> >>>>>> type 'unsigned int' is always false
> >>>>>> [-Wtautological-constant-out-of-range-compare]
> >>>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >>>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> >>>>>>
> >>>>>> This warning only happens on x86_64 but that check is relevant for
> >>>>>> 32-bit x86 so we cannot remove it.
> >>>>>
> >>>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
> >>>>> in both cases, and remain is a 32-bit value in both cases. How can it be
> >>>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
> >>>>>
> >>>>
> >>>> Hi Michel,
> >>>>
> >>>> Can't this condition be true when UINT_MAX == ULONG_MAX?
> >>>
> >>> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
> >>>
> >>>
> >>> Anyway, this suggests a possible better solution:
> >>>
> >>> #if UINT_MAX == ULONG_MAX
> >>> 	if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >>> 		return -EINVAL;
> >>> #endif
> >>>
> >>>
> >>> Or if that can't be used for some reason, something like
> >>>
> >>> 	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
> >>> 		return -EINVAL;
> >>>
> >>> should silence the warning.
> >> 
> >> I do like this one better than the former.
> >
> > FWIW, one downside of this one compared to all alternatives (presumably)
> > is that it might end up generating actual code even on 64-bit, which
> > always ends up skipping the return.
> 
> I like this better than the UINT_MAX == ULONG_MAX comparison because
> that creates a dependency on the type of remain.
> 
> Then again, a sufficiently clever compiler could see through the cast,
> and flag the warning anyway...

Would you prefer a patch that adds that cast rather than silencing the
warning outright? It does appear to work for clang.

Cheers,
Nathan
Jani Nikula Feb. 13, 2020, 10:05 p.m. UTC | #8
On Thu, 13 Feb 2020, Nathan Chancellor <natechancellor@gmail.com> wrote:
> On Thu, Feb 13, 2020 at 04:37:15PM +0200, Jani Nikula wrote:
>> On Wed, 12 Feb 2020, Michel Dänzer <michel@daenzer.net> wrote:
>> > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
>> >> On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
>> >>> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
>> >>>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
>> >>>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
>> >>>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>> >>>>>> enabled for i915 so we see the following warning:
>> >>>>>>
>> >>>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
>> >>>>>> result of comparison of constant 576460752303423487 with expression of
>> >>>>>> type 'unsigned int' is always false
>> >>>>>> [-Wtautological-constant-out-of-range-compare]
>> >>>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
>> >>>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>> >>>>>>
>> >>>>>> This warning only happens on x86_64 but that check is relevant for
>> >>>>>> 32-bit x86 so we cannot remove it.
>> >>>>>
>> >>>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
>> >>>>> in both cases, and remain is a 32-bit value in both cases. How can it be
>> >>>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
>> >>>>>
>> >>>>
>> >>>> Hi Michel,
>> >>>>
>> >>>> Can't this condition be true when UINT_MAX == ULONG_MAX?
>> >>>
>> >>> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
>> >>>
>> >>>
>> >>> Anyway, this suggests a possible better solution:
>> >>>
>> >>> #if UINT_MAX == ULONG_MAX
>> >>> 	if (unlikely(remain > N_RELOC(ULONG_MAX)))
>> >>> 		return -EINVAL;
>> >>> #endif
>> >>>
>> >>>
>> >>> Or if that can't be used for some reason, something like
>> >>>
>> >>> 	if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
>> >>> 		return -EINVAL;
>> >>>
>> >>> should silence the warning.
>> >> 
>> >> I do like this one better than the former.
>> >
>> > FWIW, one downside of this one compared to all alternatives (presumably)
>> > is that it might end up generating actual code even on 64-bit, which
>> > always ends up skipping the return.
>> 
>> I like this better than the UINT_MAX == ULONG_MAX comparison because
>> that creates a dependency on the type of remain.
>> 
>> Then again, a sufficiently clever compiler could see through the cast,
>> and flag the warning anyway...
>
> Would you prefer a patch that adds that cast rather than silencing the
> warning outright? It does appear to work for clang.

I'd take the cast.

If that fails for whatever reason, per-file

CFLAGS_gem/i915_gem_execbuffer.o = $(call cc-disable-warning, tautological-constant-out-of-range-compare)

over subdir-ccflags-y would be preferrable I think.

BR,
Jani.



>
> Cheers,
> Nathan
Nick Desaulniers Feb. 13, 2020, 10:43 p.m. UTC | #9
On Wed, Feb 12, 2020 at 9:17 AM Michel Dänzer <michel@daenzer.net> wrote:
>
> On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
> > On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
> >> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> >>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
> >>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> >>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> >>>>> enabled for i915 so we see the following warning:
> >>>>>
> >>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> >>>>> result of comparison of constant 576460752303423487 with expression of
> >>>>> type 'unsigned int' is always false
> >>>>> [-Wtautological-constant-out-of-range-compare]
> >>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> >>>>>
> >>>>> This warning only happens on x86_64 but that check is relevant for
> >>>>> 32-bit x86 so we cannot remove it.
> >>>>
> >>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
> >>>> in both cases, and remain is a 32-bit value in both cases. How can it be
> >>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
> >>>>
> >>>
> >>> Hi Michel,
> >>>
> >>> Can't this condition be true when UINT_MAX == ULONG_MAX?
> >>
> >> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
> >>
> >>
> >> Anyway, this suggests a possible better solution:
> >>
> >> #if UINT_MAX == ULONG_MAX
> >>      if (unlikely(remain > N_RELOC(ULONG_MAX)))
> >>              return -EINVAL;
> >> #endif
> >>
> >>
> >> Or if that can't be used for some reason, something like
> >>
> >>      if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
> >>              return -EINVAL;
> >>
> >> should silence the warning.
> >
> > I do like this one better than the former.
>
> FWIW, one downside of this one compared to all alternatives (presumably)
> is that it might end up generating actual code even on 64-bit, which
> always ends up skipping the return.

The warning is pointing out that the conditional is always false,
which is correct on 64b.  The check is only active for 32b.
https://godbolt.org/z/oQrgT_
The cast silences the warning for 64b.  (Note that GCC and Clang also
generate precisely the same instruction sequences in my example, just
GCC doesn't warn on such tautologies).
Nathan Chancellor Feb. 13, 2020, 11:27 p.m. UTC | #10
On Thu, Feb 13, 2020 at 02:43:21PM -0800, Nick Desaulniers wrote:
> On Wed, Feb 12, 2020 at 9:17 AM Michel Dänzer <michel@daenzer.net> wrote:
> >
> > On 2020-02-12 6:07 p.m., Nathan Chancellor wrote:
> > > On Wed, Feb 12, 2020 at 09:52:52AM +0100, Michel Dänzer wrote:
> > >> On 2020-02-11 9:39 p.m., Nathan Chancellor wrote:
> > >>> On Tue, Feb 11, 2020 at 10:41:48AM +0100, Michel Dänzer wrote:
> > >>>> On 2020-02-11 7:13 a.m., Nathan Chancellor wrote:
> > >>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> > >>>>> enabled for i915 so we see the following warning:
> > >>>>>
> > >>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> > >>>>> result of comparison of constant 576460752303423487 with expression of
> > >>>>> type 'unsigned int' is always false
> > >>>>> [-Wtautological-constant-out-of-range-compare]
> > >>>>>         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> > >>>>>             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > >>>>>
> > >>>>> This warning only happens on x86_64 but that check is relevant for
> > >>>>> 32-bit x86 so we cannot remove it.
> > >>>>
> > >>>> That's suprising. AFAICT N_RELOC(ULONG_MAX) works out to the same value
> > >>>> in both cases, and remain is a 32-bit value in both cases. How can it be
> > >>>> larger than N_RELOC(ULONG_MAX) on 32-bit (but not on 64-bit)?
> > >>>>
> > >>>
> > >>> Hi Michel,
> > >>>
> > >>> Can't this condition be true when UINT_MAX == ULONG_MAX?
> > >>
> > >> Oh, right, I think I was wrongly thinking long had 64 bits even on 32-bit.
> > >>
> > >>
> > >> Anyway, this suggests a possible better solution:
> > >>
> > >> #if UINT_MAX == ULONG_MAX
> > >>      if (unlikely(remain > N_RELOC(ULONG_MAX)))
> > >>              return -EINVAL;
> > >> #endif
> > >>
> > >>
> > >> Or if that can't be used for some reason, something like
> > >>
> > >>      if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
> > >>              return -EINVAL;
> > >>
> > >> should silence the warning.
> > >
> > > I do like this one better than the former.
> >
> > FWIW, one downside of this one compared to all alternatives (presumably)
> > is that it might end up generating actual code even on 64-bit, which
> > always ends up skipping the return.
> 
> The warning is pointing out that the conditional is always false,
> which is correct on 64b.  The check is only active for 32b.
> https://godbolt.org/z/oQrgT_
> The cast silences the warning for 64b.  (Note that GCC and Clang also
> generate precisely the same instruction sequences in my example, just
> GCC doesn't warn on such tautologies).

Thanks for confirming! I'll send a patch to add the cast later tonight.

Cheers,
Nathan
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index b8c5f8934dbd..159355eb43a9 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -22,6 +22,7 @@  subdir-ccflags-y += $(call cc-disable-warning, sign-compare)
 subdir-ccflags-y += $(call cc-disable-warning, sometimes-uninitialized)
 subdir-ccflags-y += $(call cc-disable-warning, initializer-overrides)
 subdir-ccflags-y += $(call cc-disable-warning, uninitialized)
+subdir-ccflags-y += $(call cc-disable-warning, tautological-constant-out-of-range-compare)
 subdir-ccflags-$(CONFIG_DRM_I915_WERROR) += -Werror
 
 # Fine grained warnings disable