[FFmpeg-devel] [PATCH 4/5] lavu/intmath.h: Fix UB in ff_ctz_c() and ff_ctzll_c()

[Rémi Denis-Courmont]

Le 31 mai 2024 03:41:40 GMT+03:00, Michael Niedermayer <michael at niedermayer.cc> a écrit :
>On Thu, May 30, 2024 at 10:54:46AM +0300, Rémi Denis-Courmont wrote:
>> Can't we just use the compiler built-ins here? AFAIK, they (GCC, LLVM) use the same algorithm if the CPU doesn't support native CTZ. And they will pick the right instruction if CPU does have CTZ.
>> 
>> I get it that maybe it wasn't working so well 20 years ago, but we've increased compiler version requirements since then.
>
>ffmpeg is written in C not GNU-C nor LLVM-C
>so we need to have non buggy C code

What does that mean here?

We can put compilers in 3 sets:
1) GCC+Clang
2) MSVC+ICC
3) others

Note that ICC and others are *not* tested (at least in FATE). MSVC and ICC are using intrinsics (in x86/intmath.h).

The problem is, GCC and Clang use intrinsics only if fast_clz is manually selected. This is silly. That's just requiring more platform-specific code added for no reasons.

If you want to test this code, DO write a test that calls the C version always. DO NOT force all unknown or unoptimised FFmpeg platforms to use the C just because we need to keep supporting hypothetical C11-conforming C compilers.

Note that I never said that we should remove the standard code.

>A modern compiler should turn a built-in into a efficient piece of code
>but so should it recognize that efficient piece of code and turn it into
>a single instruction if such exist.

I doubt any compiler will detect that the Debujin algorithm can be replaced by a CTZ.  The *official* standard solution is to use <stdbit.h> (C23-only unfortunately), so there are no reasons why compilers should even care to deal with this by now.

>In the end with a modern compiler it shouldnt matter how you write this
>a loop, some optimized standard implementation or a built in
>the disavantage of a builtin is that it requires #ifs and checks
>for each compiler.

A modern compiler supports STDC bit-wise functions from C23, and therefore doesn't care about this.

>So we should go with the simplest/cleanest that reliably produces optimal code
>across all supported platforms (and also consider potential future platforms
>so we dont have a maintaince nightmare but something that we can write once
>and expect it will be close to optimal forever)

So what do you do about the existing x86 special cases and the fast_clz case?

AFAICT, the only way to (arguably) do that is to switch to the C23 bit manipulation functions, and provide fallback for old compilers.

That's clearly out of the scope of this patch, and I bet some people would object anyway because they'll find the names of the new functions ugly and/or too long.