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

[Tomas Härdin]

tor 2024-05-30 klockan 12:42 -0300 skrev James Almer:
> On 5/30/2024 12:32 PM, Tomas Härdin wrote:
> > tor 2024-05-30 klockan 17:28 +0300 skrev Rémi Denis-Courmont:
> > > 
> > > 
> > > Le 30 mai 2024 17:07:21 GMT+03:00, "Tomas Härdin"
> > > <git at haerdin.se> a
> > > écrit :
> > > > > We should depend on punning as long as it conforms to the
> > > > > standard.
> > > > 
> > > > My mistake, I forgot type punning is allowed in C. It's UB in
> > > > C++
> > > > 
> > > > > > The standard compliant way
> > > > > > is to use memcpy()
> > > > > 
> > > > > That's way worse than union in terms of how proactively the
> > > > > compiler
> > > > > needs to optimise, and both approaches are as confirming.
> > > > 
> > > > A good compiler will do the same thing
> > > 
> > > True, and I don't care very much about memcpy vs union, as they
> > > both
> > > rely on matching representation. AFAIR, FFmpeg tends to use
> > > unions
> > > though.
> > > 
> > > > 
> > > > Maybe I can get the riscv version covered by Eva as well.
> > > > That's
> > > > beyond
> > > > the scope of this patchset
> > > 
> > > IMHO, this specific patch (and the following one) are beating
> > > dead
> > > horses. Sure there may be theoretical UB in the current code, but
> > > if
> > > there is a *better* implementation, better switch to that than
> > > bike
> > > shedding the fix for the UB.
> > 
> > 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. It is
not a "theoretical" UB - that's not how UB works. 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().

We could certainly replace some of these functions with intrinsics, but
that's not what this patchset is about. I don't know what set of
compilers we support. I don't know what intrinsics they support. Am I
to be compelled to figure that out, and provide the necessary
intrinsics for all of them?

This may all seem trivial, and it is, but this patchset is also a test
balloon. Line struggle is important. What I see is the stalling of
fixes of *known broken code*. That is not encouraging.

/Tomas