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

[wm4]

On Mon, 12 Oct 2015 11:16:00 -0400
Ganesh Ajjanagadde <gajjanag at mit.edu> 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.

Haven't really been following this, but how about we disable warnings
that just waste our time?