A recent commit in clang added -Wtautological-compare to -Wall, which is
enabled for i915 after -Wtautological-compare is disabled for the rest
of the kernel so we see the following warning on x86_64:
../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
# define unlikely(x) __builtin_expect(!!(x), 0)
^
1 warning generated.
It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
account for the case where this file is built for 32-bit x86, where
ULONG_MAX == UINT_MAX and this check is still relevant.
Cast remain to unsigned long, which keeps the generated code the same
(verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
the warning is silenced so we can catch more potential issues in the
future.
Link: https://github.com/ClangBuiltLinux/linux/issues/778
Suggested-by: Michel Dänzer <[email protected]>
Signed-off-by: Nathan Chancellor <[email protected]>
---
Round 3 :)
Previous threads/patches:
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 60c984e10c4a..47f4d8ab281e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1430,7 +1430,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct i915_vma *vma)
urelocs = u64_to_user_ptr(entry->relocs_ptr);
remain = entry->relocation_count;
- if (unlikely(remain > N_RELOC(ULONG_MAX)))
+ if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
return -EINVAL;
/*
--
2.25.0
On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
> A recent commit in clang added -Wtautological-compare to -Wall, which is
> enabled for i915 after -Wtautological-compare is disabled for the rest
> of the kernel so we see the following warning on x86_64:
>
> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
> ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
> # define unlikely(x) __builtin_expect(!!(x), 0)
> ^
> 1 warning generated.
>
> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
> account for the case where this file is built for 32-bit x86, where
> ULONG_MAX == UINT_MAX and this check is still relevant.
>
> Cast remain to unsigned long, which keeps the generated code the same
> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
> the warning is silenced so we can catch more potential issues in the
> future.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/778
> Suggested-by: Michel Dänzer <[email protected]>
> Signed-off-by: Nathan Chancellor <[email protected]>
Works for me as a workaround,
Reviewed-by: Jani Nikula <[email protected]>
> ---
>
> Round 3 :)
>
> Previous threads/patches:
>
> https://lore.kernel.org/lkml/[email protected]/
> https://lore.kernel.org/lkml/[email protected]/
>
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 60c984e10c4a..47f4d8ab281e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1430,7 +1430,7 @@ static int eb_relocate_vma(struct i915_execbuffer *eb, struct i915_vma *vma)
>
> urelocs = u64_to_user_ptr(entry->relocs_ptr);
> remain = entry->relocation_count;
> - if (unlikely(remain > N_RELOC(ULONG_MAX)))
> + if (unlikely((unsigned long)remain > N_RELOC(ULONG_MAX)))
> return -EINVAL;
>
> /*
--
Jani Nikula, Intel Open Source Graphics Center
Quoting Jani Nikula (2020-02-14 06:36:15)
> On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
> > A recent commit in clang added -Wtautological-compare to -Wall, which is
> > enabled for i915 after -Wtautological-compare is disabled for the rest
> > of the kernel so we see the following warning on x86_64:
> >
> > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
> > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
> > # define unlikely(x) __builtin_expect(!!(x), 0)
> > ^
> > 1 warning generated.
> >
> > It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
> > account for the case where this file is built for 32-bit x86, where
> > ULONG_MAX == UINT_MAX and this check is still relevant.
> >
> > Cast remain to unsigned long, which keeps the generated code the same
> > (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
> > the warning is silenced so we can catch more potential issues in the
> > future.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/778
> > Suggested-by: Michel Dänzer <[email protected]>
> > Signed-off-by: Nathan Chancellor <[email protected]>
>
> Works for me as a workaround,
But the whole point was that the compiler could see that it was
impossible and not emit the code. Doesn't this break that?
-Chris
On Fri, 14 Feb 2020, Chris Wilson <[email protected]> wrote:
> Quoting Jani Nikula (2020-02-14 06:36:15)
>> On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
>> > A recent commit in clang added -Wtautological-compare to -Wall, which is
>> > enabled for i915 after -Wtautological-compare is disabled for the rest
>> > of the kernel so we see the following warning on x86_64:
>> >
>> > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
>> > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>> > ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
>> > # define unlikely(x) __builtin_expect(!!(x), 0)
>> > ^
>> > 1 warning generated.
>> >
>> > It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
>> > account for the case where this file is built for 32-bit x86, where
>> > ULONG_MAX == UINT_MAX and this check is still relevant.
>> >
>> > Cast remain to unsigned long, which keeps the generated code the same
>> > (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
>> > the warning is silenced so we can catch more potential issues in the
>> > future.
>> >
>> > Link: https://github.com/ClangBuiltLinux/linux/issues/778
>> > Suggested-by: Michel Dänzer <[email protected]>
>> > Signed-off-by: Nathan Chancellor <[email protected]>
>>
>> Works for me as a workaround,
>
> But the whole point was that the compiler could see that it was
> impossible and not emit the code. Doesn't this break that?
It seems that goal and the warning are fundamentally incompatible.
Back to the original patch?
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
On Fri, Feb 14, 2020 at 08:32:19AM +0000, Chris Wilson wrote:
> Quoting Jani Nikula (2020-02-14 06:36:15)
> > On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
> > > A recent commit in clang added -Wtautological-compare to -Wall, which is
> > > enabled for i915 after -Wtautological-compare is disabled for the rest
> > > of the kernel so we see the following warning on x86_64:
> > >
> > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
> > > ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > > ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
> > > # define unlikely(x) __builtin_expect(!!(x), 0)
> > > ^
> > > 1 warning generated.
> > >
> > > It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
> > > account for the case where this file is built for 32-bit x86, where
> > > ULONG_MAX == UINT_MAX and this check is still relevant.
> > >
> > > Cast remain to unsigned long, which keeps the generated code the same
> > > (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
> > > the warning is silenced so we can catch more potential issues in the
> > > future.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/778
> > > Suggested-by: Michel D?nzer <[email protected]>
> > > Signed-off-by: Nathan Chancellor <[email protected]>
> >
> > Works for me as a workaround,
>
> But the whole point was that the compiler could see that it was
> impossible and not emit the code. Doesn't this break that?
> -Chris
As noted in the commit message, I ran diff <(objdump -Dr) <(objdump -Dr)
on objects files compiled with and without the patch with clang and gcc
for x86_64 and gcc for i386 (i386 does not build with clang) and there
was zero difference aside from the file names.
At the end of the day, I do not really care how the warning get fixed,
just that it does since it is the only one on x86_64 defconfig.
Cheers,
Nathan
On 2020-02-14 12:49 p.m., Jani Nikula wrote:
> On Fri, 14 Feb 2020, Chris Wilson <[email protected]> wrote:
>> Quoting Jani Nikula (2020-02-14 06:36:15)
>>> On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
>>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
>>>> enabled for i915 after -Wtautological-compare is disabled for the rest
>>>> of the kernel so we see the following warning on x86_64:
>>>>
>>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
>>>> ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
>>>> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
>>>> # define unlikely(x) __builtin_expect(!!(x), 0)
>>>> ^
>>>> 1 warning generated.
>>>>
>>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
>>>> account for the case where this file is built for 32-bit x86, where
>>>> ULONG_MAX == UINT_MAX and this check is still relevant.
>>>>
>>>> Cast remain to unsigned long, which keeps the generated code the same
>>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
>>>> the warning is silenced so we can catch more potential issues in the
>>>> future.
>>>>
>>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778
>>>> Suggested-by: Michel Dänzer <[email protected]>
>>>> Signed-off-by: Nathan Chancellor <[email protected]>
>>>
>>> Works for me as a workaround,
>>
>> But the whole point was that the compiler could see that it was
>> impossible and not emit the code. Doesn't this break that?
>
> It seems that goal and the warning are fundamentally incompatible.
Not really:
if (sizeof(remain) >= sizeof(unsigned long) &&
unlikely(remain > N_RELOC(ULONG_MAX)))
return -EINVAL;
In contrast to the cast, this doesn't generate any machine code on 64-bit:
https://godbolt.org/z/GmUE4S
but still generates the same code on 32-bit:
https://godbolt.org/z/hAoz8L
--
Earthling Michel Dänzer | https://redhat.com
Libre software enthusiast | Mesa and X developer
On Fri, Feb 14, 2020 at 7:36 AM Michel Dänzer <[email protected]> wrote:
>
> On 2020-02-14 12:49 p.m., Jani Nikula wrote:
> > On Fri, 14 Feb 2020, Chris Wilson <[email protected]> wrote:
> >> Quoting Jani Nikula (2020-02-14 06:36:15)
> >>> On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
> >>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> >>>> enabled for i915 after -Wtautological-compare is disabled for the rest
> >>>> of the kernel so we see the following warning on x86_64:
> >>>>
> >>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
> >>>> ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> >>>> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
> >>>> # define unlikely(x) __builtin_expect(!!(x), 0)
> >>>> ^
> >>>> 1 warning generated.
> >>>>
> >>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
> >>>> account for the case where this file is built for 32-bit x86, where
> >>>> ULONG_MAX == UINT_MAX and this check is still relevant.
> >>>>
> >>>> Cast remain to unsigned long, which keeps the generated code the same
> >>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
> >>>> the warning is silenced so we can catch more potential issues in the
> >>>> future.
> >>>>
> >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778
> >>>> Suggested-by: Michel Dänzer <[email protected]>
> >>>> Signed-off-by: Nathan Chancellor <[email protected]>
> >>>
> >>> Works for me as a workaround,
> >>
> >> But the whole point was that the compiler could see that it was
> >> impossible and not emit the code. Doesn't this break that?
> >
> > It seems that goal and the warning are fundamentally incompatible.
>
> Not really:
>
> if (sizeof(remain) >= sizeof(unsigned long) &&
> unlikely(remain > N_RELOC(ULONG_MAX)))
> return -EINVAL;
>
> In contrast to the cast, this doesn't generate any machine code on 64-bit:
>
> https://godbolt.org/z/GmUE4S
>
> but still generates the same code on 32-bit:
>
> https://godbolt.org/z/hAoz8L
Exactly.
This check is only a tautology when `sizeof(long) == sizeof(int)` (ie.
ILP32 platforms, like 32b x86), notice how BOTH GCC AND Clang generate
exactly the same code: https://godbolt.org/z/6ShrDM
Both compilers eliminate the check when `-m32` is not set, and
generate the exact same check otherwise. How about:
```
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index d3f4f28e9468..25b9d3f3ad57 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -1415,8 +1415,10 @@ static int eb_relocate_vma(struct
i915_execbuffer *eb, struct eb_vma *ev)
urelocs = u64_to_user_ptr(entry->relocs_ptr);
remain = entry->relocation_count;
+#ifndef CONFIG_64BIT
if (unlikely(remain > N_RELOC(ULONG_MAX)))
return -EINVAL;
+#endif
/*
* We must check that the entire relocation array is safe
```
We now have 4 proposed solutions:
1. https://lore.kernel.org/lkml/[email protected]/
2. https://lore.kernel.org/lkml/[email protected]/
3. https://lore.kernel.org/lkml/[email protected]/
4. my diff above
Let's please come to a resolution on this.
--
Thanks,
~Nick Desaulniers
On Mon, Mar 16, 2020 at 02:41:23PM -0700, Nick Desaulniers wrote:
> On Fri, Feb 14, 2020 at 7:36 AM Michel D?nzer <[email protected]> wrote:
> >
> > On 2020-02-14 12:49 p.m., Jani Nikula wrote:
> > > On Fri, 14 Feb 2020, Chris Wilson <[email protected]> wrote:
> > >> Quoting Jani Nikula (2020-02-14 06:36:15)
> > >>> On Thu, 13 Feb 2020, Nathan Chancellor <[email protected]> wrote:
> > >>>> A recent commit in clang added -Wtautological-compare to -Wall, which is
> > >>>> enabled for i915 after -Wtautological-compare is disabled for the rest
> > >>>> of the kernel so we see the following warning on x86_64:
> > >>>>
> > >>>> ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1433: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)))
> > >>>> ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > >>>> ../include/linux/compiler.h:78:42: note: expanded from macro 'unlikely'
> > >>>> # define unlikely(x) __builtin_expect(!!(x), 0)
> > >>>> ^
> > >>>> 1 warning generated.
> > >>>>
> > >>>> It is not wrong in the case where ULONG_MAX > UINT_MAX but it does not
> > >>>> account for the case where this file is built for 32-bit x86, where
> > >>>> ULONG_MAX == UINT_MAX and this check is still relevant.
> > >>>>
> > >>>> Cast remain to unsigned long, which keeps the generated code the same
> > >>>> (verified with clang-11 on x86_64 and GCC 9.2.0 on x86 and x86_64) and
> > >>>> the warning is silenced so we can catch more potential issues in the
> > >>>> future.
> > >>>>
> > >>>> Link: https://github.com/ClangBuiltLinux/linux/issues/778
> > >>>> Suggested-by: Michel D?nzer <[email protected]>
> > >>>> Signed-off-by: Nathan Chancellor <[email protected]>
> > >>>
> > >>> Works for me as a workaround,
> > >>
> > >> But the whole point was that the compiler could see that it was
> > >> impossible and not emit the code. Doesn't this break that?
> > >
> > > It seems that goal and the warning are fundamentally incompatible.
> >
> > Not really:
> >
> > if (sizeof(remain) >= sizeof(unsigned long) &&
> > unlikely(remain > N_RELOC(ULONG_MAX)))
> > return -EINVAL;
> >
> > In contrast to the cast, this doesn't generate any machine code on 64-bit:
> >
> > https://godbolt.org/z/GmUE4S
> >
> > but still generates the same code on 32-bit:
> >
> > https://godbolt.org/z/hAoz8L
>
> Exactly.
>
> This check is only a tautology when `sizeof(long) == sizeof(int)` (ie.
> ILP32 platforms, like 32b x86), notice how BOTH GCC AND Clang generate
> exactly the same code: https://godbolt.org/z/6ShrDM
>
> Both compilers eliminate the check when `-m32` is not set, and
> generate the exact same check otherwise. How about:
> ```
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index d3f4f28e9468..25b9d3f3ad57 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1415,8 +1415,10 @@ static int eb_relocate_vma(struct
> i915_execbuffer *eb, struct eb_vma *ev)
>
> urelocs = u64_to_user_ptr(entry->relocs_ptr);
> remain = entry->relocation_count;
> +#ifndef CONFIG_64BIT
> if (unlikely(remain > N_RELOC(ULONG_MAX)))
> return -EINVAL;
> +#endif
>
> /*
> * We must check that the entire relocation array is safe
> ```
>
> We now have 4 proposed solutions:
> 1. https://lore.kernel.org/lkml/[email protected]/
> 2. https://lore.kernel.org/lkml/[email protected]/
> 3. https://lore.kernel.org/lkml/[email protected]/
> 4. my diff above
> Let's please come to a resolution on this.
This is the only warning on an x86_64 defconfig build. Apologies if we
are being too persistent or nagging but we need guidance from the i915
maintainers on which solution they would prefer so it can be picked up.
I understand you all are busy and I appreciate the work you all do but
I do not want this to fall between the cracks because it is annoying to
constantly see this warning.
Cheers,
Nathan
On Thu, 26 Mar 2020, Nathan Chancellor <[email protected]> wrote:
> This is the only warning on an x86_64 defconfig build. Apologies if we
> are being too persistent or nagging but we need guidance from the i915
> maintainers on which solution they would prefer so it can be picked up.
> I understand you all are busy and I appreciate the work you all do but
> I do not want this to fall between the cracks because it is annoying to
> constantly see this warning.
Apologies for the delay. As I replied first thing in this thread, this
works for me. Thanks for the patch, pushed to drm-intel-next-queued.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center