Re: [PATCH v6 9/9] drm/amd/display: Mark dc_fixpt_from_fraction() noinline

[Nathan Chancellor]

On Sun, Dec 22, 2024 at 12:27:47PM +0800, Tiezhu Yang wrote:
> On 12/21/2024 03:40 PM, Xi Ruoyao wrote:
> > On Fri, 2024-12-20 at 15:34 -0700, Nathan Chancellor wrote:
> > > > Now, the thing is, these ASSERT()s are checking for divide-by-zero, I
> > > > suspect clang figured that out and invokes UB on us and just stops
> > > > code-gen.
> > > 
> > > Yeah, I think your analysis is spot on, as this was introduced by a
> > > change in clang from a few months ago according to my bisect:
> > > 
> > > https://github.com/llvm/llvm-project/commit/37932643abab699e8bb1def08b7eb4eae7ff1448
> > > 
> > > Since the ASSERT does not do anything to prevent the divide by zero (it
> > > just flags it with WARN_ON) and the rest of the code doesn't either, I
> > > assume that the codegen stops as soon as it encounters the unreachable
> > > that change created from the path where divide by zero would occur via
> > > 
> > >   dc_fixpt_recip() ->
> > >     dc_fixpt_from_fraction() ->
> > >       complete_integer_division_u64() ->
> > >         div64_u64_rem()
> > > 
> > > Shouldn't callers of division functions harden them against dividing by
> > > zero?
> > 
> > Yes I think it'd be the correct solution.
> 
> Thank you all. Do you mean like this?
> 
> --- >8 ---
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> index 88d3f9d7dd55..848d8e67304a 100644
> --- a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> +++ b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> @@ -79,11 +79,13 @@ struct fixed31_32 dc_fixpt_from_fraction(long long
> numerator, long long denomina
>         unsigned long long arg2_value = arg2_negative ? -denominator :
> denominator;
> 
>         unsigned long long remainder;
> +       unsigned long long res_value;
> 
>         /* determine integer part */
> 
> -       unsigned long long res_value = complete_integer_division_u64(
> -               arg1_value, arg2_value, &remainder);
> +       ASSERT(arg2_value);
> +
> +       res_value = complete_integer_division_u64(arg1_value, arg2_value,
> &remainder);
> 
>         ASSERT(res_value <= LONG_MAX);
> 
> @@ -214,8 +216,6 @@ struct fixed31_32 dc_fixpt_recip(struct fixed31_32 arg)
>          * Good idea to use Newton's method
>          */
> 
> -       ASSERT(arg.value);
> -
>         return dc_fixpt_from_fraction(
>                 dc_fixpt_one.value,
>                 arg.value);
> 
> With the above changes, there is no "falls through" objtool warning
> compiled with both clang 19 and the latest mainline clang 20.

I am somewhat surprised that changes anything because the ASSERT is not
stopping control flow so I would expect the same problem as before. I
guess it does not happen perhaps due to inlining differences? I looked
at this code briefly when I sent my initial message and I was not sure
where such a check should exist. It does not look like these functions
really do any sort of error handling.

> If you are OK with it, I will send a separate formal patch to handle
> this issue after doing some more testing.

It may still be worth doing this to get some initial thoughts from the
AMD DRM folks.

Cheers,
Nathan