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
