[FFmpeg-devel] [PATCH 1/5] lavu/common.h: Fix UB in av_clipl_int32_c()

[Rémi Denis-Courmont]

Le torstaina 30. toukokuuta 2024, 19.48.13 EEST Tomas Härdin a écrit :
> > > Are you saying that UB is acceptable? You know the compiler is free
> > > to
> > > assume signed arithmetic doesn't overflow, right? If so then what
> > > other
> > > UB might we accept?
> > 
> > He did not say that... He said we should switch to a better
> > implementation rather than trying to fix the existing potentially
> > buggy one.
> 
> I have a fix for demonstrable UB and Rémi is problematizing it.

Andreas made cosmetic arguments against this patch before I had even seen the 
patch, forget comment on it.

> It is not a "theoretical" UB - that's not how UB works.

It is a *theoretical* UB if you can not prove that it leads to misbehaviour in 
any *practical* use. In theory, all UB is *potentially* fatal. Emphasis on 
potentially.

So yes, while all UB instances are bad and deserve fixing, they are not all 
equally bad nor urgent. UB that is proven to lead to remote code execution is 
way worse than theoretical UB that has only been proven in literature, and is 
not known or even seriously suspected to lead to broken optimisations.

> Any compiler doing
> basic value analysis will find it, and is therefore free to do whatever
> it wants, for example deleting all calls to av_clipl_int32_c().

That is formally true. But it is also formally true that, by that same logic, 
since there is most certainly some UB instance left elsewhere in the codebase, 
the entirety of libavutil could be elided by the compiler. In other words, in 
theory, FFmpeg does not work at all. Does that mean that we should give up on 
the project here and now?

> We could certainly replace some of these functions with intrinsics, but
> that's not what this patchset is about.

I am not sure what is your point because nobody said that av_clipl_int32_c() 
should be replaced by intrinsics.

> I don't know what set of compilers we support.

That is irrelevant since all C99, C11 and C23 compilers support the proposed 
substitute code as long as <stdint.h> defines int32_t.

> I don't know what intrinsics they support.

Also irrelevant.

> Am I to be compelled to figure that out, and provide the necessary
> intrinsics for all of them?

No, and you are the only person to have made an implication to the contrary as 
far as *this* patch is concerned.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/