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

[Andreas Rheinhardt]

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). While we could of
course look through our own asserts and change them, we must not simply
do so for our users.
(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.
This shows that all our asserts need to be checked and be potentially
changed to be consistent with using them as optimization hints.)

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).

- Andreas