Message-ID: <CAHk-=wiq=E0fwJLFpCc3wPY_9BPZF3dbdqGgVoOmK9Ykj5JEeg@mail.gmail.com>
Date: Sat, 26 Apr 2025 10:42:40 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Josh Poimboeuf <jpoimboe@...nel.org>, Harry Wentland <harry.wentland@....com>, 
	Leo Li <sunpeng.li@....com>, Alex Deucher <alexander.deucher@....com>, 
	Christian König <christian.koenig@....com>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <nick.desaulniers+lkml@...il.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>
Cc: "the arch/x86 maintainers" <x86@...nel.org>, dri-devel <dri-devel@...ts.freedesktop.org>, 
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: clang and drm issue: objtool warnings from clang build

So with clang, I get these drm build warnings

  drivers/gpu/drm/amd/amdgpu/../display/dc/basics/fixpt31_32.o:
     warning: objtool: dc_fixpt_recip() falls through to next function
dc_fixpt_sinc()

  drivers/gpu/drm/amd/amdgpu/../display/dc/sspl/spl_fixpt31_32.o:
     warning: objtool: spl_fixpt_recip() falls through to next
function spl_fixpt_sinc()

and the warnings seem real. I ignored them because it wasn't entirely
obvious what was going on and I was looking at other things, but today
I looked at why these happen.

What is going on is that the *_fixpt_recip() function has a

        SPL_ASSERT(arg.value);

which results in basically a test for 'arg.value' being zero and a WARN_ON()

Then - through inlining - we get from *_fixpt_recip() to

  spl_fixpt_from_fraction ->
    spl_complete_integer_division_u64()

here we have *another* check for the divisor not being zero:

        SPL_ASSERT(divisor);

and then it goes on to inline the code:

      spl_div64_u64_rem() ->
        div64_u64_rem()

which does

        *remainder = dividend % divisor;
        return dividend / divisor;

so now what has happened is that clang sees that when it inlines those
things, it basically has a path with two warnings, following a divide
by a value known to be the constant zero.

And that makes clang just stop generating any code at all, and the asm
looks like this:

spl_fixpt_recip:                        # @spl_fixpt_recip
# %bb.0:
        callq   __fentry__
        testq   %rdi, %rdi
        je      .LBB3_10
  ...
.LBB3_10:
        #APP
     .. disgusting unreadable for WARN_ON() on line 199 ..
        #NO_APP
        #APP
     .. disgusting unreadable for WARN_ON() on line 32 ..
        #NO_APP
.Lfunc_end3:

because clang has decided - correctly - that this path now divides by
zero. Notice how it just falls off at .Lfunc_end3 with no actual
divide, and no other sign of "you have failed at life".

However, this is actually problematic for the kernel: yes, the warning
hopefully helps show what is wrong, but because clang has now
generated code that basically jumps to random code, you get random
security issues if the warnign ever triggers (and there are
configurations where the warning is just disabled).

So there are two problems here:

 - the drm code should not "assert" crap. Do proper error handling, or
don't. But don't have a random "this is a known bad value" and then
fall back to using that value anyway.

We had something similar some time ago, where there was a drm
assertion without error handling, which caused the compiler to see
that there was a static path where the invalid value was used, and
then caused other problems. I forget the details, and gmail search
isn't helping me

But I *really* think that clang silently just generating known bad
code for invalida operations like this is very very dangerous, and is
a bug in clang.

So I'm cc'ing drm and clang people (and x86 people) in the hope that
we can fix both of these things.

Can we *please* get a flag for clang that it doesn't just stop
generating code because it has decided some path is unreachable or
undefined? Add a TRAP instruction, for Chrissake! PLEASE!

 Maybe such a flag already exists, and the kernel just doesn't know
about it. This whole "do random things for undefined behavior" is a
bug, dammit.

And yes, the drm code shouldn't be doing this. Adding warnings for
things that will oops is actually counter-productive. It generates
extra code, but this is an example of it actually causing *more*
problems. We'd have been better off with a nice clean divide-by-zero
oops than with a warning followed by random behavior.

               Linus

Powered by blists - more mailing lists