From patchwork Sun May 12 12:49:00 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adam Richter X-Patchwork-Id: 13086 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 57519447C1D for ; Sun, 12 May 2019 15:55:24 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3C47F680921; Sun, 12 May 2019 15:55:24 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-vs1-f68.google.com (mail-vs1-f68.google.com [209.85.217.68]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 305666804B6 for ; Sun, 12 May 2019 15:55:17 +0300 (EEST) Received: by mail-vs1-f68.google.com with SMTP id d128so6365082vsc.10 for ; Sun, 12 May 2019 05:55:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=H73vmU8SFAfUDVC2WPK4Pg792x+hZNkAL4jVaryUWBs=; b=t32f4mrBn5OpN71ZG2j6bjMRG3cKn8zd4M+lxKeXMEfSLsp/PeG9nGpR2pNjD/w2Vn o9i22rx3/FDFz45EzYYK2vMhG1BhhTnS/LE/XOakt94Jbm0LWeEDll1QfcWDQwOljmfy VR2H9zkw8P5ga4mS4dAI9amur+fVu2zLCXzszU9O+83LU4oAk++r9gNCx6wD3pdYxPx+ waRQ/B0HEVhW5RvgDoxb5oV4Zz189eymFgIrasoGkZLEAYa1KXrl84aWXcEWxc9zt2kP ifg4CsVS7/Zq2/ov8a3tDOpNbDK5YDQq4crCA9JIb+4WxsOUSoNrZzNO5awZ0CD9kV1N maKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=H73vmU8SFAfUDVC2WPK4Pg792x+hZNkAL4jVaryUWBs=; b=OOfNyGI2EJfgyKNhqWUVJjQRdaour8lmKF9PRjB5ssNcz0Oi9csd/5+IBSrw9+l+nW 9XaK/B1AY6QOuZJGU1yDOr3QzIsQU89ATWLOdQkRG5SGse8XGJBbs7T4cq+iZVKLUFMO w+PLY+2c7CX5MHBs7z7oFR2WN0VFfv0cMEiZ4LVwW/bF4vU17pvtBX0QNYJ8vXORUL6A i1sZhekTZOB8rY4LdKPy3W4yKq10QAAZ/MCNeL4zYszBHMf6tMvckV2hcljWP46Epcxd 3TVjSW+uF15WMYgahWfRMjBatyFX8OHOjWDeu3aggBvc3/xH6h2aI0MmNjF8wybf4kJp LOYw== X-Gm-Message-State: APjAAAU7IM5Cxb9iXbIALD6UIIesQkeAui6NUaciRJ+Mv6lMs17nmybl AKD/38YRlG1YHjwvSh42lfICzSvgvgf0UakgyPsT/OWQiWAS+w== X-Google-Smtp-Source: APXvYqzmNkzdhX0KEigMmpXaxDXbaYcYayKstRby6i+bSeFNe5KfdvDDyTw0yoJvFtxSCqNdf/3VFGhNXMdFsHKWw9Q= X-Received: by 2002:a67:e98e:: with SMTP id b14mr5749710vso.145.1557665351326; Sun, 12 May 2019 05:49:11 -0700 (PDT) MIME-Version: 1.0 From: Adam Richter Date: Sun, 12 May 2019 05:49:00 -0700 Message-ID: To: ffmpeg-devel@ffmpeg.org Subject: [FFmpeg-devel] [PATCH] libavformat: Separate assertions of the form "av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise diagnostics. X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" This is the first of what I expect to be several patches to convert assertions of the form "assert(a && b)" to "assert(a); assert(b)". Here are some reasons why I think this would be an improvement. This lengthy argument is not included in the patch attachment. 1. Assertion failures are often sporadic, and users who report them may not be in a position to efficiently narrow them down further, so it is important to get as much precision from each assertion failure as possible. 2. It is a more efficient use of developers time when a bug report arrives if the problem has been narrowed down that much more. A new bug report may initially attract more interest, so, if the problem has been narrowed down that much more, it may increase the chance that developers may invest the time to try to resolve the problem, and also reduce unnecessary traffic on the developer mailing list about possible causes of the bug that separating the assertion was able to rule out. 3. It's often more readable, sometimes eliminating parentheses or changing multi-line conditions to separate single line conditions. 4. When using a debugger to step over an assertion failure in the first part of the statement, the second part is still tested. 5. Providing separate likelihood hints to the compiler in the form of separate assert statements does not require the compiler to be quite as smart to recognize that it should optimize both branches, although I do not know if that makes a difference for any compiler commonly used to compile X (that is, I suspect that they are all smart enough to realize is that "a && b" is likely true, then "a" is likely true and "b" is likely true). I have confirmed that the resulting tree built without any apparent complaints about the assert statements and that "make fate" completed with a zero exit code. I have not done any other tests though. Thanks in advance for considering this patch. Adam From edb58a5ee8030ec66c04736a025d2a44e7322ba3 Mon Sep 17 00:00:00 2001 From: Adam Richter Date: Sun, 12 May 2019 03:41:49 -0700 Subject: [PATCH] libavformat: Separate assertions of the form "av_assertN(a && b)" to "av_assertN(a); av_assertN(b)" for more precise diagnostics. Signed-off-by: Adam Richter --- libavformat/au.c | 3 ++- libavformat/avienc.c | 3 ++- libavformat/aviobuf.c | 3 ++- libavformat/matroskaenc.c | 6 ++++-- libavformat/mov.c | 3 ++- libavformat/rtmppkt.c | 3 ++- libavformat/utils.c | 9 +++++---- 7 files changed, 19 insertions(+), 11 deletions(-) diff --git a/libavformat/au.c b/libavformat/au.c index cb48e67feb..76f23de56d 100644 --- a/libavformat/au.c +++ b/libavformat/au.c @@ -177,7 +177,8 @@ static int au_read_header(AVFormatContext *s) bps = 2; } else { const uint8_t bpcss[] = {4, 0, 3, 5}; - av_assert0(id >= 23 && id < 23 + 4); + av_assert0(id >= 23); + av_assert0(id < 23 + 4); ba = bpcss[id - 23]; bps = bpcss[id - 23]; } diff --git a/libavformat/avienc.c b/libavformat/avienc.c index ac0f04c354..e96eb27e5e 100644 --- a/libavformat/avienc.c +++ b/libavformat/avienc.c @@ -797,7 +797,8 @@ static int avi_write_packet(AVFormatContext *s, AVPacket *pkt) int pal_size = 1 << par->bits_per_coded_sample; int pc_tag, i; - av_assert0(par->bits_per_coded_sample >= 0 && par->bits_per_coded_sample <= 8); + av_assert0(par->bits_per_coded_sample >= 0); + av_assert0(par->bits_per_coded_sample <= 8); if ((pb->seekable & AVIO_SEEKABLE_NORMAL) && avist->pal_offset) { int64_t cur_offset = avio_tell(pb); diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 5a33f82950..e71846e0e8 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -195,7 +195,8 @@ static void flush_buffer(AVIOContext *s) void avio_w8(AVIOContext *s, int b) { - av_assert2(b>=-128 && b<=255); + av_assert2(b >= -128); + av_assert2(b <= 255); *s->buf_ptr++ = b; if (s->buf_ptr >= s->buf_end) flush_buffer(s); diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c index cef504fa05..51b6d1b16f 100644 --- a/libavformat/matroskaenc.c +++ b/libavformat/matroskaenc.c @@ -589,7 +589,8 @@ static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra tracks[j].has_cue = 0; for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) { int tracknum = entry[j].stream_idx; - av_assert0(tracknum>=0 && tracknum= 0); + av_assert0(tracknum < num_tracks); if (tracks[tracknum].has_cue && s->streams[tracknum]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE) continue; tracks[tracknum].has_cue = 1; @@ -605,7 +606,8 @@ static int64_t mkv_write_cues(AVFormatContext *s, mkv_cues *cues, mkv_track *tra tracks[j].has_cue = 0; for (j = 0; j < cues->num_entries - i && entry[j].pts == pts; j++) { int tracknum = entry[j].stream_idx; - av_assert0(tracknum>=0 && tracknum= 0); + av_assert0(tracknum < num_tracks); if (tracks[tracknum].has_cue && s->streams[tracknum]->codecpar->codec_type != AVMEDIA_TYPE_SUBTITLE) continue; tracks[tracknum].has_cue = 1; diff --git a/libavformat/mov.c b/libavformat/mov.c index 78f692872b..c45ea416fc 100644 --- a/libavformat/mov.c +++ b/libavformat/mov.c @@ -3267,7 +3267,8 @@ static void fix_index_entry_timestamps(AVStream* st, int end_index, int64_t end_ int64_t* frame_duration_buffer, int frame_duration_buffer_size) { int i = 0; - av_assert0(end_index >= 0 && end_index <= st->nb_index_entries); + av_assert0(end_index >= 0); + av_assert0(end_index <= st->nb_index_entries); for (i = 0; i < frame_duration_buffer_size; i++) { end_ts -= frame_duration_buffer[frame_duration_buffer_size - 1 - i]; st->index_entries[end_index - 1 - i].timestamp = end_ts; diff --git a/libavformat/rtmppkt.c b/libavformat/rtmppkt.c index 1eeae17337..04b651d45e 100644 --- a/libavformat/rtmppkt.c +++ b/libavformat/rtmppkt.c @@ -501,7 +501,8 @@ int ff_amf_tag_size(const uint8_t *data, const uint8_t *data_end) ret = amf_tag_skip(&gb); if (ret < 0 || bytestream2_get_bytes_left(&gb) <= 0) return -1; - av_assert0(bytestream2_tell(&gb) >= 0 && bytestream2_tell(&gb) <= data_end - data); + av_assert0(bytestream2_tell(&gb) >= 0); + av_assert0(bytestream2_tell(&gb) <= data_end - data); return bytestream2_tell(&gb); } diff --git a/libavformat/utils.c b/libavformat/utils.c index a63d71b0f4..efd1b17588 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -163,10 +163,11 @@ void av_format_inject_global_side_data(AVFormatContext *s) int ff_copy_whiteblacklists(AVFormatContext *dst, const AVFormatContext *src) { - av_assert0(!dst->codec_whitelist && - !dst->format_whitelist && - !dst->protocol_whitelist && - !dst->protocol_blacklist); + av_assert0(!dst->codec_whitelist); + av_assert0(!dst->format_whitelist); + av_assert0(!dst->protocol_whitelist); + av_assert0(!dst->protocol_blacklist); + dst-> codec_whitelist = av_strdup(src->codec_whitelist); dst->format_whitelist = av_strdup(src->format_whitelist); dst->protocol_whitelist = av_strdup(src->protocol_whitelist); -- 2.21.0