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 9E1024B001 for ; Sun, 26 May 2024 00:59:50 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D599A68D4C1; Sun, 26 May 2024 03:59:47 +0300 (EEST) Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05olkn2026.outbound.protection.outlook.com [40.92.90.26]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 2330268D1C6 for ; Sun, 26 May 2024 03:59:41 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=XdDtNZzm0KoB/ZdyL8U72dyp9AXHecPmed57Cx5n9/SGbM57Di4N3F951DOZTsr+IkUmk8aKTcZlTM+kRn+gL+nqzRG2kZ7IGM6Q7YJHQj4Bc0hfkZ2CVTS0vY5TK2FWerV3vgNfDHOWoVmu1qit7eeapCygzeLwrnZb1TAuYzpTlBkCWeUU7iSYEpRidDi2Y4ssajBUP/+N6tgZaZOrKJk9bBSfm0zsJVaBC8ei/7+vr6VOuN6aKHSGHEeAG/h32K4s70tyEXEUeq5ItmJYi3j1gl6qiYsNkVV/alMU7aP89ylHPZzZzxFpOXueCHpGsoWC/XjpzDve5fuq6JJKnA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=a6DF+vsJY+Ses3TdAaCk6Iv6zHip5PUpNe03wsGYLnA=; b=HAR9ZIuuaN/xLt/weXP8NxGdwSSgyyWps3FfJ5dzLMrQJLRuOJoZLFEu2D4z2Cfh4V4qXfQSo/CdmIJdxKnuWYV9xRsr2Gonj1Fb6ROBg4EWGdtSvjchJLkOPmBkKCYxJ9MfjzpupZNR7ywihyMeTfo///o4GuCYt3ucb06KSZXNkGmng+hfUT/mdFHFriFUd/ZC6Kl95ouue1O32NZjjsKPI9Sdd4OOYlopKnXpjQ0rgrxDsxG1pgK4rqA3hygclsYNexlelc2md89WrcnXPaA2Pl26oTQXuqcFKLe1hJCbAky/iQIW3zVGm2KDhPQXfncDon32orJsfk1rCfCiRA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=a6DF+vsJY+Ses3TdAaCk6Iv6zHip5PUpNe03wsGYLnA=; b=OKAz23bzGiUc6k6j4NJv8u2Lms2BK3f5eqDFHTsgfwZX98WilgEkVFPnkfOKeABqXH4MAelMtvzLNRgQerASm5XVo54NQvixjqshNox69IwT5aeRzqTua5qupPKppkQDDCSC0befFNIlmlgVlCrH3DXNNWJHx4owMbiVeb/wDxHBe7TUzd1c0qvvbXdmdfCvIGxjdgf32REJq7rOZgmLffVqDTZe/z1vuKSY8fxyOHlUIoO16VLkF3xUg50EsH+82Kzym+vpIV5budcycI/hgHyHUHir8aqA29T/iq6ou7OPpfYz4+AbusYWgIzMUYQIT8icJR30bBmzC2Pw+6Cl/w== Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) by PAXP250MB0565.EURP250.PROD.OUTLOOK.COM (2603:10a6:102:289::5) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7611.22; Sun, 26 May 2024 00:59:38 +0000 Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::384d:40d4:ecb7:1c9]) by AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::384d:40d4:ecb7:1c9%4]) with mapi id 15.20.7611.016; Sun, 26 May 2024 00:59:38 +0000 Message-ID: Date: Sun, 26 May 2024 02:59:35 +0200 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20240526002239.GG2821752@pb2> Content-Language: en-US From: Andreas Rheinhardt In-Reply-To: <20240526002239.GG2821752@pb2> X-TMN: [V0+bqFUDxym3BWZbfyrCRxnn8ExCAT6eA5v9RsXma3o=] X-ClientProxiedBy: ZR0P278CA0163.CHEP278.PROD.OUTLOOK.COM (2603:10a6:910:45::10) To AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: AS8P250MB0744:EE_|PAXP250MB0565:EE_ X-MS-Office365-Filtering-Correlation-Id: 7225aa0d-1866-41af-6b86-08dc7d1f1da7 X-Microsoft-Antispam: BCL:0;ARA:14566002|461199019|440099019|3412199016; X-Microsoft-Antispam-Message-Info: e0zIHk78LBDaVzI6QZluR3viHuY3y89kxMOnsVHMWouVl54sU3XHB7NRC7Y8epAeZ7kqL97AN4roAcRyuSN2bDhy/T8vTpprv8uCxETTZRUA5RKhDpyD5DK5AoSrQmQDN24czQsRhoExhTZh244QlCDrWh7TPTl1RmfY4H1OH/kf6szls+RlLWqDPg0Giutcm3ZJmKlCmJW8+JSPdxgd4Mv2fQi63HS6PLGyk5mSZED7Kz6C2Tfy8A2c0npesz4JRcgnjt8vRl5Wd/NRj/zSxqXIa+cnltw8fdK4lmuuPoOVjTgcf5vial9sM1zOyAOxFFTaXFASErFlpkp9z0bqafqe7oV2WjBaMyUq6S4qfT8NzF2MYmTNliHRoCUWSdKgnmhyYenT4JnJax6Igadgk7slpH5tWOwjcPHuVzegqR6LMqjgvX7uL8HhJ9Ff4I40A2TJsws2jqnQIq0XtcJOOdsvn9giClvjZLGF8ad3y22bqjwUuULpxXIrmizfUe5NcDmcv+7myeUeL3siRu0QQcvijejCVjDdfgPzBPJl6quqkxJ4FNOKe1sOp6t8ZTme X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?YW9oOEZleHA2NWdiVmE4c2dSNHpJUjdKcXQvUnc0VXRtaTdJVGxlem5wbFdQ?= =?utf-8?B?d2JDM3RYQUM1T2J1U3RVQUZDQXJKU2MyZkFVUzh5dG9qWUZDanRLT21mSG1D?= =?utf-8?B?T0RMZStFZXFGcVVnL3ppbFNaYk5lb0Fyb2lES2RpK3V0cmVSUDZob29SSXpM?= =?utf-8?B?UGJRbGFEejN1M1k2YlZIeC9UUnM1cG9JMkt4d0VvQU54MW5KRmY1cUNicWw3?= =?utf-8?B?VjRaRHFaaGNIUTllSkVsOHBDWW55UTdqTEcwOS81RVYyWlc1UGlRWGJqcEpD?= =?utf-8?B?eng5ZjdIRkpYeXlMdXhPcWNtYmpsQjdFeUFwRGR2YkthUTRMVmRrRHN4elMw?= =?utf-8?B?R28vZmxoZHJGZGw5N3VKR0RYcmh0a0FOZnVtdVZiNk50ZDVuOWFuSExDVkl0?= =?utf-8?B?aFNueGRlSmt5Ym5jaDBlakRXYnljN250dGJGWlhPMHNJQVlPT0Y3dUdMdXFs?= =?utf-8?B?ODMvdjZkdEdqSmpIcUE0MHVVQ2VKVDhOcXBLV3NVSEZiWjduRE9US0JqalhT?= =?utf-8?B?Q213SjBHVTBmTk9Lbm9WemRIYXpBRjRzVU1BblhscTRPOUMxeGczNVUwc082?= =?utf-8?B?SVRoRkpveEhBa1A5ekpZenNtUUdIK1AxeXNqV0doKzM4L1VidEJncEVXL3lZ?= =?utf-8?B?bXNGSGxuTjE3bXdqZzRkOWlkOFNzRmptWDFmL1JlVmRMdUNOVWRBZzFJNys5?= =?utf-8?B?amlHZmtVL1pXNGRMdVA4SXdjZ2djRHEzMEVFbHk5NEtLSEZDL0ZRVmcyUDhC?= =?utf-8?B?cVNIVEdXNTRUYzZ4T1BBcVU0SlY4VE9rMXZxZjBoNGlWaFV6NnBHclZKS2Mz?= =?utf-8?B?dk5yaE1ZODVJdGJwOEZaMVpXeDd1ZUYzUFFrTlF1eTRValBTMWdmK2Zid1hs?= =?utf-8?B?Smhsek0xdnVwa09QRjV6aUpTWmxqZXIveTNDQ1VpUWw4Z2VnWk1qcnFpMzBj?= =?utf-8?B?YUFDQ2hOa1E0YTE2SDBxUi9BS1dJYmduSHRudlYxUTMwRG4yN01oNDA1d0w1?= =?utf-8?B?bmlFeGNqcnBmMUlQOW85WCtXUVpwTTJkaGRxMjBLZXZmdFZJUEdnOERXT2tL?= =?utf-8?B?RUw3WmQrMzlaRG5KdnU3YUdOV1dWUldaNFlhVC81bmZJajU2S2JQOThPcTBC?= =?utf-8?B?ajBvL2JvaHVMNlR6TFVGRk1Eb2F6dlpJMDNnRGhiZU5ZTmtDVVZlK1V2U1J5?= =?utf-8?B?WnRwb0hoUjIzZWx3Y0RSaUxYMlZZT0lRQ1pzNzErOWNGMzF6eW9kc2RXenFI?= =?utf-8?B?YnVMYXBMeUwrWmQ1bS94SGRGaThTUWVjVFJTelNWNWJveHpvM25ERCsrQ2N3?= =?utf-8?B?RkNxcFNWcVA0bk4zREJoa25LR2FTTXhNQUNvWlAyeHlRV1hjWlYwaTlIOUlM?= =?utf-8?B?bmxoaGhNTjJSZ0lNRnU5akhxSWVyNHBCeVQ2SENCT1Y2TWEzMXI5ZC9DQVhE?= =?utf-8?B?QWVkLzZKNTUxYVhtOTZlQ1ZYbTgweHhDS3hMaDhxd210eGMzalF1VlZzK3JL?= =?utf-8?B?SVpPSzBNRXlHVFhWVjFEZDN1ZEQrbUtKKzd6UWpFWDlMSGFSblEwR0FPZDht?= =?utf-8?B?cUdmS2dtTndBRktJbndiNVJXaFlxYWlIK2x5TW9NNmxySnI1U0I2ZWU0TS9k?= =?utf-8?B?M21hQVdqeitjTjA2TUNOK0kxTHd1b1gzVzhjQTlRMnViMGxVQ0UvOCs1Y3hY?= =?utf-8?B?aTFaWWJWUDBlUGF1c3pON2Jpa0lVYUo4bE4rV3RRVXBBUWNGTmcvZ3VBPT0=?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7225aa0d-1866-41af-6b86-08dc7d1f1da7 X-MS-Exchange-CrossTenant-AuthSource: AS8P250MB0744.EURP250.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 May 2024 00:59:38.0326 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: PAXP250MB0565 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: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: 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 >> --- >> 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 _______________________________________________ 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".