[FFmpeg-devel] [PATCH] avformat/movenc: suppress -Wstrict-overflow warnings

Thu Oct 15 04:06:56 CEST 2015

On Mon, Oct 12, 2015 at 12:00 PM, Ganesh Ajjanagadde <gajjanag at mit.edu> wrote:
> On Mon, Oct 12, 2015 at 11:47 AM, Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Mon, Oct 12, 2015 at 11:16:00AM -0400, Ganesh Ajjanagadde wrote:
>>> On Mon, Oct 12, 2015 at 10:54 AM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>>> > Hi,
>>> >
>>> > On Mon, Oct 12, 2015 at 10:51 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> > wrote:
>>> >>
>>> >> On Thu, Oct 8, 2015 at 5:07 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> >> wrote:
>>> >> > On Sat, Oct 3, 2015 at 8:17 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> >> > wrote:
>>> >> >> On Tue, Sep 29, 2015 at 10:49 AM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> >> >> wrote:
>>> >> >>> On Sun, Sep 27, 2015 at 9:39 PM, Ganesh Ajjanagadde <gajjanag at mit.edu>
>>> >> >>> wrote:
>>> >> >>>> On Sun, Sep 27, 2015 at 9:18 PM, Michael Niedermayer
>>> >> >>>> <michaelni at gmx.at> wrote:
>>> >> >>>>> On Sun, Sep 27, 2015 at 01:23:03PM -0400, Ganesh Ajjanagadde wrote:
>>> >> >>>>>> On Sun, Sep 27, 2015 at 12:58 PM, Michael Niedermayer
>>> >> >>>>>> <michaelni at gmx.at> wrote:
>>> >> >>>>>> > On Sat, Sep 26, 2015 at 10:55:26PM -0400, Ganesh Ajjanagadde
>>> >> >>>>>> > wrote:
>>> >> >>>>>> >> On Sat, Sep 26, 2015 at 10:32 PM, Ronald S. Bultje
>>> >> >>>>>> >> <rsbultje at gmail.com> wrote:
>>> >> >>>>>> >> > Hi,
>>> >> >>>>>> >> >
>>> >> >>>>>> >> > On Sat, Sep 26, 2015 at 7:19 PM, Ganesh Ajjanagadde
>>> >> >>>>>> >> > <gajjanag at mit.edu>
>>> >> >>>>>> >> > wrote:
>>> >> >>>>>> >> >
>>> >> >>>>>> >> >> On Sat, Sep 26, 2015 at 7:11 PM, Michael Niedermayer
>>> >> >>>>>> >> >> <michaelni at gmx.at>
>>> >> >>>>>> >> >> wrote:
>>> >> >>>>>> >> >> > On Fri, Sep 18, 2015 at 05:15:50PM -0400, Ganesh
>>> >> >>>>>> >> >> > Ajjanagadde wrote:
>>> >> >>>>>> >> >> >> This patch results in identical behavior of movenc, and
>>> >> >>>>>> >> >> >> suppresses
>>> >> >>>>>> >> >> -Wstrict-overflow
>>> >> >>>>>> >> >> >> warnings observed in GCC 5.2.
>>> >> >>>>>> >> >> >> I have manually checked that all usages are safe, and
>>> >> >>>>>> >> >> >> overflow
>>> >> >>>>>> >> >> possibility does
>>> >> >>>>>> >> >> >> not exist with this expression rewrite.
>>> >> >>>>>> >> >> >>
>>> >> >>>>>> >> >> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>>> >> >>>>>> >> >> >> ---
>>> >> >>>>>> >> >> >>  libavformat/movenc.c | 2 +-
>>> >> >>>>>> >> >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> >> >>>>>> >> >> >>
>>> >> >>>>>> >> >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>>> >> >>>>>> >> >> >> index af03d1e..6e4a1a6 100644
>>> >> >>>>>> >> >> >> --- a/libavformat/movenc.c
>>> >> >>>>>> >> >> >> +++ b/libavformat/movenc.c
>>> >> >>>>>> >> >> >> @@ -854,7 +854,7 @@ static int
>>> >> >>>>>> >> >> >> get_cluster_duration(MOVTrack *track,
>>> >> >>>>>> >> >> int cluster_idx)
>>> >> >>>>>> >> >> >>  {
>>> >> >>>>>> >> >> >>      int64_t next_dts;
>>> >> >>>>>> >> >> >>
>>> >> >>>>>> >> >> >> -    if (cluster_idx >= track->entry)
>>> >> >>>>>> >> >> >> +    if (cluster_idx - track->entry >= 0)
>>> >> >>>>>> >> >> >
>>> >> >>>>>> >> >> > i do not understand what this fixes or why
>>> >> >>>>>> >> >> > also plese quote the actual warnings which are fixed in the
>>> >> >>>>>> >> >> > commit
>>> >> >>>>>> >> >> > message
>>> >> >>>>>> >> >>
>>> >> >>>>>> >> >> I have posted v2 with a more detailed commit message. It
>>> >> >>>>>> >> >> should be
>>> >> >>>>>> >> >> self explanatory.
>>> >> >>>>>> >> >
>>> >> >>>>>> >> >
>>> >> >>>>>> >> > Even with the new message, it's still not clear to me what's
>>> >> >>>>>> >> > being fixed.
>>> >> >>>>>> >> > What does the warning check for? What is the problem in the
>>> >> >>>>>> >> > initial
>>> >> >>>>>> >> > expression?
>>> >> >>>>>> >>
>>> >> >>>>>> >> Compilers make transformations on the statements in order to
>>> >> >>>>>> >> possibly
>>> >> >>>>>> >> get better performance when compiled with optimizations.
>>> >> >>>>>> >> However, some
>>> >> >>>>>> >> of these optimizations require assumptions in the code. In
>>> >> >>>>>> >> particular,
>>> >> >>>>>> >> the compiler is internally rewriting cluster_idx >= track->entry
>>> >> >>>>>> >> to
>>> >> >>>>>> >> cluster_idx - track->entry >= 0 internally for some reason (I am
>>> >> >>>>>> >> not
>>> >> >>>>>> >> an asm/instruction set guy, so I can't comment why it likes
>>> >> >>>>>> >> this).
>>> >> >>>>>> >> However, such a transformation is NOT always safe as integer
>>> >> >>>>>> >> arithmetic can overflow (try e.g extreme values close to
>>> >> >>>>>> >> INT_MIN,
>>> >> >>>>>> >> INT_MAX). The warning is spit out since the compiler can't be
>>> >> >>>>>> >> sure
>>> >> >>>>>> >> that this is safe, but it still wants to do it (I suspect only
>>> >> >>>>>> >> the
>>> >> >>>>>> >> -O3/-O2 level that try this, can check if you want).
>>> >> >>>>>> >
>>> >> >>>>>> > iam not sure i understand correctly but
>>> >> >>>>>> > if the compiler changes the code and then warns that what it just
>>> >> >>>>>> > did might be unsafe then the compiler is broken
>>> >> >>>>>>
>>> >> >>>>>>
>>> >> >>>>>> https://stackoverflow.com/questions/12984861/dont-understand-assuming-signed-overflow-warning
>>> >> >>>>>> - gives a detailed explanation.
>>> >> >>>>>>
>>> >> >>>>>> Some more info: this is triggered only when -finline-functions is
>>> >> >>>>>> enabled (done by default on -O3, not enabled by default on -O2).
>>> >> >>>>>> -finline-functions tries to inline stuff even when "inline" keyword
>>> >> >>>>>> is
>>> >> >>>>>> absent (like in this case).
>>> >> >>>>>> As for the warning, http://linux.die.net/man/1/gcc - search for
>>> >> >>>>>> -Wstrict-overflow. It is enabled due to -Wall, and as the man page
>>> >> >>>>>> suggests, it depends on optimization level as we can see in this
>>> >> >>>>>> example.
>>> >> >>>>>> I do consider the compiler broken in this case, but then again
>>> >> >>>>>> compilers are broken in so many different ways it is not even
>>> >> >>>>>> funny:
>>> >> >>>>>> see e.g -Warray-bounds, can't use the ISO C correct { 0 }
>>> >> >>>>>> initializer
>>> >> >>>>>> for compound data types, etc.
>>> >> >>>>>>
>>> >> >>>>>> If you don't like this, we should add a -Wnostrict-overflow either
>>> >> >>>>>> to
>>> >> >>>>>> configure, or a local enable/disable via pragmas/macros. I don't
>>> >> >>>>>> like
>>> >> >>>>>> either of these as compared to this simple workaround:
>>> >> >>>>>> 1. -Wnostrict-overflow: FFmpeg with the amount of integer
>>> >> >>>>>> arithmetic
>>> >> >>>>>> being done should benefit from this warning in general, so
>>> >> >>>>>> disabling
>>> >> >>>>>> it globally may be bad.
>>> >> >>>>>
>>> >> >>>>> how many actual bugs has Wstrict-overflow found ?
>>> >> >>>>
>>> >> >>>> No idea; maybe a good place to check is the Google fuzzing effort
>>> >> >>>> where many bugs were fixed.
>>> >> >>>
>>> >> >>> See e.g your commit: 09ef98f1ae3c8a4e08b66f41c3bd97dd7b07405f -
>>> >> >>> Wstrict-overflow is indeed useful.
>>> >> >>> I am thus convinced that we should retain it.
>>> >> >>> Given the fact that local suppression is not worth it for just 2
>>> >> >>> instances and also that the patch does not reduce readability in any
>>> >> >>> way, I think this patch and the one for xface are ok.
>>> >> >
>>> >> > Here is some more detailed digging. Please note that I am not certain
>>> >> > of the following, but someone with more asm experience could possibly
>>> >> > confirm.
>>> >> >
>>> >> > First, recall that this is only triggered with -finline-functions
>>> >> > (automatically enabled at -O3). What -finline-functions does is lets
>>> >> > the compiler inline stuff even when the "inline" keyword is not used
>>> >> > when the compiler finds it beneficial. Now when the compiler inlines
>>> >> > this, it wants to rewrite the expression
>>> >> > cluster_idx >= track->entry
>>> >> > to
>>> >> > cluster_idx - track->entry >= 0,
>>> >> > a transformation that is not always safe due to integer overflow
>>> >> > possibilities as explained in detail above. However, as I mention in
>>> >> > the commit message, I have manually audited and made sure all such
>>> >> > transformations are safe.
>>> >> >
>>> >> > Now the question is: why does it want to do that? The answer is hinted
>>> >> > at in the warning messages themselves:
>>> >> >
>>> >> > libavformat/movenc.c: In function ‘mov_flush_fragment’:
>>> >> > libavformat/movenc.c:4112:12: warning: assuming signed overflow does
>>> >> > not occur when assuming that (X - c) > X is always false
>>> >> > [-Wstrict-overflow]
>>> >> >  static int mov_flush_fragment(AVFormatContext *s)
>>> >> >             ^
>>> >> > libavformat/movenc.c:4112:12: warning: assuming signed overflow does
>>> >> > not occur when assuming that (X - c) > X is always false
>>> >> > [-Wstrict-overflow]
>>> >> > libavformat/movenc.c:857:8: warning: assuming signed overflow does not
>>> >> > occur when assuming that (X - c) > X is always false
>>> >> > [-Wstrict-overflow]
>>> >> >      if (cluster_idx >= track->entry)
>>> >> >
>>> >> > In mov_flush_fragment, there are multiple calls to
>>> >> > get_cluster_duration that are getting inlined due to
>>> >> > -finline-functions:
>>> >> > line 4132:        if (get_cluster_duration(track, track->entry - 1) !=
>>> >> > 0)
>>> >> > line 4138:         track->track_duration +=
>>> >> > get_cluster_duration(track, track->entry - 2);
>>> >> > line 4139:         track->end_pts        +=
>>> >> > get_cluster_duration(track, track->entry - 2);
>>> >> >
>>> >> > Examining these closely, the compiler can easily do a constant
>>> >> > propagation if it assumes the lack of integer overflow:
>>> >> > track->entry >= track->entry - 2 is not always true depending on
>>> >> > track->entry's value (take near INT_MIN for instance),
>>> >> > but if transformed as I indicated, it becomes a -2 >= 0 (and easily
>>> >> > optimized out) since the compiler assumes associative arithmetic on
>>> >> > integers when optimizations are enabled. Yes, illustrated above is the
>>> >> > fact that any kind of arithmetic expression rewrite (including simple
>>> >> > associative transformations) is potentially unsafe due to overflow,
>>> >> > but the compiler wants (since we use O3) and I am sure everybody here
>>> >> > likes the fact that we get optimizations for them. It wants to warn us
>>> >> > about the "crazier" arithmetic transformation regarding the >=
>>> >> > comparison.
>>> >> >
>>> >> > Overall, I still believe this patch should be applied:
>>> >> > 1. It is a clean solution - the code in no way is less readable.
>>> >> > 2. It silences the compiler.
>>> >> > 3. Alternative approaches here are far worse for IMHO little gain (e.g
>>> >> > local disable is much more noisy than the above, disabling of
>>> >> > -Wstrict-overflow altogether is a terrible idea due to e.g commit
>>> >> > 09ef98f1ae3c8a4e08b66f41c3bd97dd7b07405f).
>>> >> > 4. I no longer view it as a compiler bug - it wants to do an
>>> >> > optimization, I am sure we would like it as well, but its hands are
>>> >> > tied until we "feed" it some information that the transformation is
>>> >> > safe. Note that it is essentially impossible for compilers to do such
>>> >> > "confirmation" analysis themselves, I needed to do a nontrivial manual
>>> >> > audit myself.
>>> >>
>>> >> You still don't feel that this is the cleanest and simplest solution
>>> >> to this problem?
>>> >
>>> >
>>> > I've been passively following this. No, I honestly don't think it is. I
>>> > understand what the compiler is doing and why, I think your analysis is
>>> > pretty good.
>>> >
>>> > But no, a - b >= 0 is not better than a >= b. a >= b is exactly what's
>>> > intended here. I understand why the compiler wants to rewrite it, but this
>>> > code isn't remotely performance-sensitive, so I'd rather go for explanatory
>>> > code.
>>> >
>>> > I tend to agree with Michael that in this particular case, the warning by
>>> > the compiler suggests it's doing something it shouldn't do. Maybe we'd want
>>> > it to do it, but again, this code is not performance-sensitive, so
>>> > explanatory code is more important then.
>>>
>>> All right, then how about a compromise: I can add a small comment
>>> right above this change saying what we really mean, and give the
>>> rewrite right below it?
>>>
>>> That yields the same level of explanation, at the cost of a small
>>> increase in verbosity due to the comment. The function is anyway not
>>> too big, so it should not be  an issue.
>>>
>>
>>> But yes, if we run into this more often than 2 instances, we can come
>>> up with a more sustainable solution.
>>
>> its more than 2 here: (gcc (Ubuntu/Linaro 4.6.3-1ubuntu5) 4.6.3)
>>
>> libavformat/movenc.c:857:8: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> ./libavutil/common.h:116:8: warning: assuming signed overflow does not occur when assuming that (X + c) < X is always false [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X - c) > X is always false [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>> libavcodec/xface.c:318:27: warning: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Wstrict-overflow]
>
> 2 comments:
> 1. There are duplicates, gcc prints out one for each inline
> instantiation. You thus have 3 on your gcc, I have 2 (movenc and
> xface) on 5.2.
> 2. I am concentrating efforts on the latest GCC and Clang, for reasons
> I have explained earlier. Roughly, I have come to realize that they
> are the only 2 platforms where we can feasibly achieve essentially
> complete warning suppression.

patchv2 implementing the comment idea has been sent. Hopefully
everyone is fine with this.

>
>>
>> [...]
>>
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Those who are too smart to engage in politics are punished by being
>> governed by those who are dumber. -- Plato
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>