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

Ganesh Ajjanagadde gajjanag at mit.edu
Mon Oct 12 18:00:39 CEST 2015


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.

>
> [...]
>
> --
> 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
>


More information about the ffmpeg-devel mailing list