[FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros

[Michael Niedermayer]

Hi

On Sun, May 26, 2024 at 02:59:35AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Hi
> > 
> > On Fri, May 24, 2024 at 11:58:21PM +0200, Andreas Rheinhardt wrote:
> >> Useful to let the compiler and static analyzers know that
> >> something is unreachable without adding an av_assert
> >> (which would be either dead for the compiler or add runtime
> >> overhead) for this.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
> >> ---
> >> I can add more macros if it is desired to differentiate between
> >> ASSERT_LEVEL == 1 and ASSERT_LEVEL > 1.
> >>
> >>  doc/APIchanges       |  3 +++
> >>  libavutil/avassert.h | 33 +++++++++++++++++++++++++++++++++
> >>  2 files changed, 36 insertions(+)
> >>
> >> diff --git a/doc/APIchanges b/doc/APIchanges
> >> index 60f056b863..5a3ae37999 100644
> >> --- a/doc/APIchanges
> >> +++ b/doc/APIchanges
> >> @@ -2,6 +2,9 @@ The last version increases of all libraries were on 2024-03-07
> >>  
> >>  API changes, most recent first:
> >>  
> >> +2024-05-24 - xxxxxxxxxx - lavu 59.xx.100 - avassert.h
> >> +  Add av_unreachable and av_assume() macros.
> >> +
> >>  2024-05-23 - xxxxxxxxxx - lavu 59.20.100 - channel_layout.h
> >>    Add av_channel_layout_ambisonic_order().
> >>  
> >> diff --git a/libavutil/avassert.h b/libavutil/avassert.h
> >> index 1895fb7551..41e29c7687 100644
> >> --- a/libavutil/avassert.h
> >> +++ b/libavutil/avassert.h
> >> @@ -31,6 +31,7 @@
> >>  #ifdef HAVE_AV_CONFIG_H
> >>  #   include "config.h"
> >>  #endif
> >> +#include "attributes.h"
> >>  #include "log.h"
> >>  #include "macros.h"
> >>  
> >> @@ -68,6 +69,38 @@
> >>  #define av_assert2_fpu() ((void)0)
> >>  #endif
> >>  
> >> +/**
> >> + * Asserts that are used as compiler optimization hints depending
> >> + * upon ASSERT_LEVEL and NBDEBUG.
> >> + *
> >> + * Undefined behaviour occurs if execution reaches a point marked
> >> + * with av_unreachable or if a condition used with av_assume()
> >> + * is false.
> >> + *
> >> + * The condition used with av_assume() should not have side-effects
> >> + * and should be visible to the compiler.
> >> + */
> > 
> > this feels wrong
> > 
> > We have 3 assert functions
> > 
> > one for security relevant code or other things we always want to check and not play around
> > 
> > one for speed relevant code where we dont want to check in production code. But may want
> > to do checks if we are debuging.
> > 
> > and one for the cases between
> > 
> > 
> > What is an assert ? Its a statement about a condition that is true unless the code
> > is broken. Its never correct to use an assert to check for a condition that is known
> > to be false for some input.
> > So a assert really already is either
> > 
> > A. Check, print, abort
> > or
> > B. undefined if false
> > 
> > But if an assert already is "undefined if false" then what you add is not
> > usefull, just add the compiler specific "assume" code to the disabled asserts
> 
> 1. So you want me to change the disabled asserts into a "if (!(cond))
> __builtin_unreachable();" (like dav1d does)? This is problematic,
> because asserts (as they are used right now) contain no requirement at
> all that the condition be visible to the compiler;


> it may contain
> function calls that the compiler cannot elide (unless it is an LTO
> compiler, but even they stop at library boundaries).

would that break anything ?


> While we could of
> course look through our own asserts and change them, we must not simply
> do so for our users.

the feature can be delayed outside our code until the next API bump


> (The PutBits API has checks for the buffer being too small:
>             av_log(NULL, AV_LOG_ERROR, "Internal error, put_bits buffer
> too small\n");
>             av_assert2(0);
> If the av_assert2 is changed into an __builtin_unreachable() in case the
> latter assert is disabled, then this defeats the purpose of this check.

indeed but finding this now, gives the oppertunity to improve this code


> This shows that all our asserts need to be checked and be potentially
> changed to be consistent with using them as optimization hints.)

maybe but introducing a redundant "assert" isnt avoiding that.
You still either check everything or you dont check
and you either use unchecked cases for optimization or only checked cases.
Its just more assert like macros


> 
> 2. It is useful, it is just a different usecase: Here the focus is not
> on correctness, but on telling the compiler something that is presumed
> to be beneficial for performance.
> 

> > This would also keep the API simpler
> 
> IMO using av_unreachable instead of av_assertX(0) expresses the intent
> better (so for me the current usage feels wrong). Same for av_assume
> (see 2. for the intent).

I dont agree

if we do add av_unreachable() we will for debuging need a way to
disabled it and print something, same as av_assert*() already does
so it really is a av_assert*() just with a different name

about the name, well

we already use

av_assert0(!"reached");

and various variations of that.
The advantage is that the API is simpler, there are fewer things one needs to remember
to use it.

Its "most pretty for core developers who work on this every day" vs.
    "easy to use for developers rarely interacting with FFmpeg"

The more our API grows, the harder it becomes for outsiders to learn and use
in this respect i dont think increasing the 3 av_assert*() with 2+ more macros is a
good idea more so because they need a way to be turned off which one too needs
to remember, ...

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240526/081e7bab/attachment.sig>