From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 2F1F448DA6 for ; Sun, 26 May 2024 17:55:16 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 0E2DA68C1BA; Sun, 26 May 2024 20:55:14 +0300 (EEST) Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 360DD68C1BA for ; Sun, 26 May 2024 20:55:07 +0300 (EEST) Received: by mail.gandi.net (Postfix) with ESMTPSA id 53F24240002 for ; Sun, 26 May 2024 17:55:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=niedermayer.cc; s=gm1; t=1716746106; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=4TwUfF+WH1ncvTVpsltCdbVSvf/cGV1KXYf6b492Ywg=; b=P6ip7Tj/RrW+Lic4aMsWpX8msV7Fu+J5Xt9Zz6hP2h/z5MKYOprqz+PMrwDl/d9lA82x2P Dp5stoqL2//IMJGAWd3DQRfo4zjBCNom6ypf6JKtaA7PVilNzW4zDl/M/lmAPy6dFFW6zg Ie1QMeH/elNtsNCuLs1Vf4CnXg4PXPd7I55iJWXWvA+aY/4CmohYQOSatswE0+jKaVcEGh Wm0zj4r6x08P43XzPnnV/hB8/3MXMHN8X9bkikoj4rSEU0Mz2YU0BfTQjLFo+eMJ9Jplou U2Dl++9dQZPjVa87cOpvMd+Tykmg0Oa8Ui4DRBWZPLkuh8mMothEVq5eNa2EUQ== Date: Sun, 26 May 2024 19:55:05 +0200 From: Michael Niedermayer To: FFmpeg development discussions and patches Message-ID: <20240526175505.GJ2821752@pb2> References: <20240526002239.GG2821752@pb2> MIME-Version: 1.0 In-Reply-To: X-GND-Sasl: michael@niedermayer.cc Subject: Re: [FFmpeg-devel] [PATCH 01/12] avutil/avassert: Add av_unreachable and av_assume() macros X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: multipart/mixed; boundary="===============3357416555852635824==" Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: --===============3357416555852635824== Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ktyc2UzNUDZzGzis" Content-Disposition: inline --ktyc2UzNUDZzGzis Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi On Sun, May 26, 2024 at 02:59:35AM +0200, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Hi > >=20 > > 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 > >> --- > >> I can add more macros if it is desired to differentiate between > >> ASSERT_LEVEL =3D=3D 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 20= 24-03-07 > >> =20 > >> API changes, most recent first: > >> =20 > >> +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(). > >> =20 > >> 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" > >> =20 > >> @@ -68,6 +69,38 @@ > >> #define av_assert2_fpu() ((void)0) > >> #endif > >> =20 > >> +/** > >> + * 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. > >> + */ > >=20 > > this feels wrong > >=20 > > We have 3 assert functions > >=20 > > one for security relevant code or other things we always want to check = and not play around > >=20 > > one for speed relevant code where we dont want to check in production c= ode. But may want > > to do checks if we are debuging. > >=20 > > and one for the cases between > >=20 > >=20 > > What is an assert ? Its a statement about a condition that is true unle= ss 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 > >=20 > > A. Check, print, abort > > or > > B. undefined if false > >=20 > > But if an assert already is "undefined if false" then what you add is n= ot > > usefull, just add the compiler specific "assume" code to the disabled a= sserts >=20 > 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 >=20 > 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. >=20 > > This would also keep the API simpler >=20 > 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 mac= ros is a good idea more so because they need a way to be turned off which one too ne= eds to remember, ... thx [...] --=20 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. --ktyc2UzNUDZzGzis Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iF0EABEKAB0WIQSf8hKLFH72cwut8TNhHseHBAsPqwUCZlN3cwAKCRBhHseHBAsP q8gTAKCNAWUtjR8I03+HOJj9mizFHkXnPQCeKFfZzlKbUI0LcVMZuTjowzy39hs= =OS1Y -----END PGP SIGNATURE----- --ktyc2UzNUDZzGzis-- --===============3357416555852635824== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". --===============3357416555852635824==--