| Message ID | AS8P250MB0744D0CB066E751A0A393A368FF52@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/12] avutil/avassert: Add av_unreachable and av_assume() macros | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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@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 This would also keep the API simpler thx [..]
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@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
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@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 [...]
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. + */ +#if defined(ASSERT_LEVEL) && ASSERT_LEVEL > 0 || !defined(HAVE_AV_CONFIG_H) && !defined(NDEBUG) +#define av_unreachable av_assert0(0) +#define av_assume(cond) av_assert0(cond) +#elif AV_GCC_VERSION_AT_LEAST(4, 5) || AV_HAS_BUILTIN(__builtin_unreachable) +#define av_unreachable __builtin_unreachable() +#if AV_HAS_BUILTIN(__builtin_assume) +#define av_assume(cond) __builtin_assume(cond) +#else +#define av_assume(cond) do { \ + if (!(cond)) \ + __builtin_unreachable(); \ +} while (0) +#endif +#elif defined(_MSC_VER) +#define av_unreachable __assume(0) +#define av_assume(cond) __assume(cond) +#else +#define av_unreachable +#define av_assume(cond) +#endif + /** * Assert that floating point operations can be executed. *
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@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(+)