On Fri, Feb 14, 2020 at 7:36 AM Michel Dänzer <michel@xxxxxxxxxxx> wrote: > > On 2020-02-14 12:49 p.m., Jani Nikula wrote: > > On Fri, 14 Feb 2020, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: > >> Quoting Jani Nikula (2020-02-14 06:36:15) > >>> On Thu, 13 Feb 2020, Nathan Chancellor <natechancellor@xxxxxxxxx> 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 <michel@xxxxxxxxxxx> > >>>> Signed-off-by: Nathan Chancellor <natechancellor@xxxxxxxxx> > >>> > >>> 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/20191123195321.41305-1-natechancellor@xxxxxxxxx/ 2. https://lore.kernel.org/lkml/20200211050808.29463-1-natechancellor@xxxxxxxxx/ 3. https://lore.kernel.org/lkml/20200214054706.33870-1-natechancellor@xxxxxxxxx/ 4. my diff above Let's please come to a resolution on this. -- Thanks, ~Nick Desaulniers _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
